-
-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIA 176 wide tables - follow-up fixes #3978
Conversation
…udl into eia176_wide_table
Also some lint changes + making these assets clearly not-persisted-yet.
8cd921b
to
602e1a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Materialized fine locally. I did notice that there's no 2023 data for EIA-176 and then found catalyst-cooperative/pudl-archiver#484.
Just one tiny naming thing -- I don't think we've ever applied the "not ready for primetime" underscore prefix to the asset group names. I get the analogy, but it feels like an extra complication that we might forget to undo, when the assets in the group are finally ready.
src/pudl/etl/__init__.py
Outdated
@@ -64,6 +64,7 @@ | |||
|
|||
|
|||
core_module_groups = { | |||
"_core_eia176": [pudl.transform.eia176], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know we haven't used the underscore prefix for any of the other asset groups. I think just having the prefix on the assets themselves is probably sufficient to indicate that they are not yet ready for use? Then the asset group will show up in a more expected place in the list of groups, alphabetically.
@@ -64,6 +64,7 @@ | |||
|
|||
|
|||
core_module_groups = { | |||
"core_eia176": [pudl.transform.eia176], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and made this 1-character change in order to get this merged in before I leave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and also re-materialized the eia176
assets locally just to make sure nothing weird happened)
Overview
Fixes a few things from the original PR (#3590):
_core_eia176__data
multi-asset now has namedOutput
s - not sure how Dagster was associating these correctly before_core_eia176_
namespace so that it's clear we don't persist them yetMy changes on top of the original PR are all in this commit
TODO in future follow-ups:
pudl_io_manager
.Documentation
Make sure to update relevant aspects of the documentation.
Tasks
Testing
How did you make sure this worked? How can a reviewer verify this?
To-do list