-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[module-loaders] [rfc] Include specs in asset module loaders #26524
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
open to ideas - i wonder which option is better:
- this PR - add an additional param
include_specs
to existing APIs - or brand new
load_asset_specs_from_x
hmmm 🤔 actually as im typing out this, i think this PR does feel cleaner and less of a cognitive load to remembering both load_asset_ and load_asset_specs.
OK I think this is a good step forward and it's not too risky or irreversible. Could you update the relevant docstrings to communicate this addition in a non-confusing way? (btw the current docstring is pretty busted 😓 https://docs-preview.dagster.io/api/python-api/assets#code-locations) |
1e84402
to
aa8efe8
Compare
1570691
to
203c228
Compare
Yea I think that ideally we don't introduce the cross product of these APIs - make the existing ones work sensibly, and then eventually introduce a centralized defs loader |
aa8efe8
to
0309dba
Compare
203c228
to
1fff846
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.
nice
0309dba
to
5be4d04
Compare
1fff846
to
2219a42
Compare
## Summary & Motivation This code path was highly crufty - code smell here is "iterating through big list and doing a ton of things at once". Solution is to create a lookup table of cacheable properties and imperatively call properties as you need them (stole this pattern from @schrockn when we were working on airlift stuff) I think the code is a lot clearer now. I also got rid of an error message we were devoting a lot of lines to that felt very outdated; nobody is using with_resources, and repository_def is misleading. I think we also end up catching a few hidden error states; for example, we weren't checking assetsdefs against sourceassets with the same key, sourceasset collision checking felt more like a side effect. ## How I Tested These Changes Existing tests
5be4d04
to
292e31e
Compare
2219a42
to
2b63520
Compare
Closing because all the commits from this PR are now contained within #26494 - ideally this would be merged, but gt fold failed to resolve things properly for me, and now this PR is in an unresolveable state. |
Summary & Motivation
This PR is the tablestakes approach for adding spec support to our asset module loaders. It enables asset spec loading in the code path, and gates it behind a bool so as to not change behavior for any existing users.
One intricacy here is that the loading the following module will error:
At this point in time, I think this is fine. It will just basically push users towards a pattern like this:
How I tested this
Added additional testing for spec loading case, ensured existing test behavior did not change.
Changelog
Our asset loading functions can now load
AssetSpec
objects when called with theinclude_specs
parameter. This includesload_assets_from_package_name
,load_assets_from_modules
,load_assets_from_modules
, andload_assets_from_current_module
.