Parallelize extraction of Excel spreadsheets #2943
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current validation failures when comparing to 10-12 full dev SQL database: |
|
Losing rows in |
zaneselvans
requested changes
Oct 17, 2023
There was a problem hiding this comment.
excel.py seems very clever and concise but I'm having a hard time understanding how it works with the multiple nested layers of factories. Can we expand the docstrings and/or add some comments and argument descriptions to help clarify how they all fit together?
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2943 +/- ##
=====================================
Coverage 88.5% 88.5%
=====================================
Files 91 91
Lines 10805 10808 +3
=====================================
+ Hits 9569 9572 +3
Misses 1236 1236
☔ View full report in Codecov by Sentry. |
zaneselvans
added
dagster
e-belfer
commented
Oct 17, 2023
- Expanded on some of the docstrings and comments to try and clarify the highly abstracted nested factories that construct the Excel extraction graph asset. - Changed some of the function/object names to use extract_ rather than load_ to match the stage-names we're using elsewhere. - It turned out that not including the empty dataframes created errors, since those dataframes are being constructed such that they always contain all mapped columns, even if the particular year being extracted doesn't have every column... and if we don't end up with all of the expected columns (even if they're null) that causes problems downstream.
zaneselvans
approved these changes
Oct 18, 2023
Comment on lines
+388
to
+391
| all_data = defaultdict(list) | ||
| for dfs in paged_dfs: | ||
| for page in dfs: | ||
| all_data[page].append(dfs[page]) |
There was a problem hiding this comment.
I switched this back to not checking for the empty dataframes because it turns out we need them to ensure all of the mapped columns are present downstream. Otherwise the fast tests and other subsets of the data fail because they don't contain columns that are only reported in the earlier years.
| return all_data | ||
|
|
||
|
|
||
| def year_extractor_factory( |
There was a problem hiding this comment.
On this and a couple of other names, I switched from load to extract so they match the sense in which we're using those words to describe the different ETL stages.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Overview
Taking over #2673. This builds on top of #2644 to add parallel data loading to EIA 861 and EIA 923 datasets. All of the logic introduced in #2644 has been factored out to
excel.pyso it can be re-used across each of the datasets.This is more work towards #2385, but doesn't quite fix it because the EIA 860m data isn't being parallel loaded yet. That's done a bit differently to other datasets in this PR at the moment, so I think best to tackle that in a follow up PR.
PR Checklist
dev).