-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Remove tables from settings #2286
Conversation
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.
Mostly just deleting code with this one.
"epacems": {"tables"}, | ||
} | ||
) | ||
).datasets.dict() |
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.
Get rid of those nasty excludes!
@@ -484,7 +484,7 @@ def define_sqlite_db( | |||
Returns: | |||
None: the effects of the function are stored inside sqlite_meta | |||
""" | |||
for table in ferc1_to_sqlite_settings.tables: | |||
for table in DBF_TABLES_FILENAMES.keys(): |
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.
Just use all tables
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.
Love a PR that deletes code!
We'll have to remember to remove the tables attribute from the FERC 714 settings once it's integrated into the dagster DAG #2266. I could see us eventually removing most of the code in settings.py in favor or using dagster config types for validating years and partitions and using the DAG to specify dataset dependencies.
Same with eia861, I think |
PR Overview
This PR removes tables from the settings models/yaml files, as discussed in #2272.
As @bendnorman mentioned in the PR linked above, it looks like we could use dagster's multi-asset subsetting to allow us to select subsets of tables, but I'm not sure if it's worth the time/energy. Open to other's thoughts on this though. Perhaps it's something we could leave to the future if we have time.
PR Checklist
dev
)