Untangle eia_transform / harvesting / bga multi-asset #2450
Conversation
zaneselvans
added
eia923
There was a problem hiding this comment.
Left some pointers / questions for both of you @bendnorman @cmgosnell
I still need to fix the ownership_eia860 issue, and I'd also like to:
- Get rid of the BGA wrapper
- Get rid of "keep_cols" and just never drop any columns from the dataframes being harvested (but if this breaks anything I will give up immediately and walk away)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #2450 +/- ##
=====================================
Coverage 86.7% 86.7%
=====================================
Files 81 81
Lines 9447 9453 +6
=====================================
+ Hits 8192 8203 +11
+ Misses 1255 1250 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
I ran the integration tests on the full DB and got some errors: pytest --live-dbs --etl-settings src/pudl/package_data/settings/etl_full.yml test/integrationEdit: these errors were cropping up because I hadn't yet merged in the settings fixes from #2424. All good now! |
|
Ah yeah you're right the harvesting notebook probably needs to be updated. Should we think about adding some/all of the devtools notebooks to be run automatically as part of the CI? |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Updated harvesting debug notebook added. |
* Got rid of some unnecessary settings error checking which is now handled by the EiaSettings class upstream. * Incorporated `_restrict_years()` into the loop at the beginning of `boiler_generator_assn_eia860()` rather than doing it piecemeal later on. * Changed `_restrict_years()` to depend directly on an `EiaSettings` object rather than lists of years.
| with nb_path.open() as f: | ||
| nb = nbformat.read(f, as_version=4) | ||
| ep = ExecutePreprocessor(timeout=600, kernel_name="python3") | ||
| _ = ep.preprocess(nb, resources={"Application": {"log_level": 5}}) |
There was a problem hiding this comment.
I cannot for the life of me figure out how to turn off the DEBUG level output coming from this test. The Application log_level thing is supposed to do it I think, but apparently I am wrong.
| "devtools/inspect-assets.ipynb", | ||
| "devtools/debug-eia-etl.ipynb", | ||
| "devtools/debug-ferc1-etl.ipynb", | ||
| "devtools/debug-harvesting.ipynb", |
There was a problem hiding this comment.
These all run pretty quickly now that they're relying on assets generated by Dagster for the most part, and they're prone to getting out of sync w/ the codebase, so I thought it made sense to add them into the tests finally. And also I just wanted to understand how to do that, since @katie-lamb and I are going to try and run some notebooks in CI for the record-linkage project.
PR Overview
Owing to our early pattern of passing around huge dictionaries of all the dataframes and mutating them inside functions, we had kind of a hairball of implied dependencies within the
eia_transformasset, which took care of harvesting and generating the boiler-generator association table. This PR breaks that asset into several pieces which can run in parallel, and reduces the inter-asset dependencies significantly.PR Checklist
dev).MoveThere were other circular imports so this didn't work.occurrence_consistencyintopudl.helpersand try to auto-generate the list of clean assets.EiaSettingsclass.col_dfsif not debug_restrict_years()in dictionary comprehension at beginning of BGA processclean_*_dfsrather thaneia_transformed_dfslegacy name for multi-DF input assets.Enumto define validentitytypes.io_manager_keyis being passed in to all asset factories as a parameter.pudl.transform.eiadevtoolsnotebooks into the integration tests.