Document new transforms & organize imports/logging #1962
Conversation
Rather than importing all modules using absolute paths in the top-level __init__.py, manage package and sub-package imports hierarchically within all of the various __init__.py files throughout the package using relative imports, with each subpackage repsonsible for importing its own modules and subpackages into the package / subpackage namespace. This re-organization surfaced some circular dependencies resulting from the logging functions being defined in pudl.helpers (which has all kinds of other dependencies in it. I removed the absolufy-imports pre-commit hook since there's no way to turn it off in the __init__.py modules, which makes it impossible to do imports that construct a clean namespace. Also made sure that all modules use the root logger configured in pudl.logging via get_logger. There was also one lingering reference to pudl.settings.Ferc1DbfSettings that hadn't been noticed because the testes still aren't passing, which I swapped out for Ferc1Settings.
zaneselvans
added
ferc1
Codecov ReportBase: 83.4% // Head: 83.5% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## xbrl_integration #1962 +/- ##
==================================================
+ Coverage 83.4% 83.5% +0.1%
==================================================
Files 71 72 +1
Lines 7769 7761 -8
==================================================
+ Hits 6484 6486 +2
+ Misses 1285 1275 -10
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 at Codecov. |
|
@cmgosnell definitely open to cutting out text if you think it's too much. Or if there's duplication and you have a preference for the one place that the content should be (we can easily link between different parts of the docs) |
|
@zschira @cmgosnell The CI is passing! 🎉 Also the CI is taking 70-80 minutes 😭! I assume that a lot of the extra time is extracting all the different XBRL databases, now that we have updated (and much larger) For the ones that aren't actually integrated into PUDL (yet) I wonder if it would be more appropriate to test the XBRL to SQLite extraction in the |
Awesome!! As for the 70-80 minutes, I think that removing the need to extract all the XBRL every time makes sense. If we do move that testing to the extractor repo, we'll need to have access to raw XBRL filings there somehow. I'm sure we can just put a zipfile with a years worth of data in that repo or something. |
There was a problem hiding this comment.
this looks pretty good overall! I left lots of little questions and requests for either clarifications, simplifications or moving explanations closer to the code/user.
a high level question/comment... were these import fixes actually related to the docs? if they weren't, I think it would have been easier for me as a reviewer to digest one of these types of changes at a time.
No, it really shouldn't have been mixed in here. I screwed this up. The imports and the logging should have each been a separate discrete PR directly into |
|
@cmgosnell I think I've at least responded to all of your comments, but I'm not sure if it'll all be satisfying. If you want to respond in comments go for it! Or we can talk on Friday maybe? |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
* Hard-coded the steam & hydro plant types in the field definitions to avoid a cross-subpackage circular import error. This isn't an ideal solution, but it works. Need to figure out the right way to deal with subpackage-level (rather than module-level) import dependencies appropriately. * Changed hydro plant type categories to use snake_case as the steam plant type categories do. * Renamed the pudl.logging module to pudl.logging_helpers since it mysteriously started colliding (apparently?) with the Python standard library logging module (even though it hadn't previously)
__init__.pyfiles, rather than importing every module absolutely in the top level__init__.pyabsolufy-importspre-commit hook.pudl.loggingto avoid circular dependencies inpudl.helpersFerc1DbfSettingsCloses #1924