[module-loaders] Simplify module asset loader code path #26484
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
170
to
159
| assets_list = LoadedAssetsList.from_modules(modules) | ||
| return assets_list.assets_defs, assets_list.sources, assets_list.cacheables |
There was a problem hiding this comment.
The extra_source_assets parameter is being dropped in this refactor. To maintain the existing functionality, the source assets should be combined like this:
return assets_list.assets_defs, list(check.opt_sequence_param(extra_source_assets, 'extra_source_assets', of_type=SourceAsset)) + assets_list.sources, assets_list.cacheablesSpotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
dpeng817
force-pushed
the
dpeng817/dpeng817/simplify_module_asset_loader
branch
from
d52130a to
ea633af
Compare
dpeng817
changed the base branch from
master
to
dpeng817/delete_extra_source_assets
OwenKephart
approved these changes
Dec 14, 2024
python_modules/dagster/dagster/_core/definitions/load_assets_from_modules.py
Outdated
Show resolved
Hide resolved
| @@ -63,48 +149,12 @@ def assets_from_modules( | |||
| A tuple containing a list of assets, a list of source assets, and a list of | |||
There was a problem hiding this comment.
python_modules/dagster/dagster/_core/definitions/load_assets_from_modules.py
Outdated
Show resolved
Hide resolved
schrockn
requested changes
Dec 14, 2024
@schrockn this function is (somewhat tragically) part of the public API so we can't change the type signature |
|
Ah yeah you are right. I was thinking that in our code it would be likely be backwards compatible change to go from
The only way out is through components. |
schrockn
approved these changes
Dec 14, 2024
dpeng817
force-pushed
the
dpeng817/dpeng817/simplify_module_asset_loader
branch
from
ea633af to
ea4f4fc
Compare
dpeng817
force-pushed
the
dpeng817/dpeng817/simplify_module_asset_loader
branch
from
ea4f4fc to
e7054f4
Compare
This was referenced Dec 16, 2024
[module-loaders] Genericize object list functionality to take in all sensors, jobs, schedules
#26545
Merged
dpeng817
force-pushed
the
dpeng817/dpeng817/simplify_module_asset_loader
branch
from
e7054f4 to
c63cd60
Compare
dpeng817
changed the title
dpeng817
force-pushed
the
dpeng817/delete_extra_source_assets
branch
2 times, most recently
from
2219a42 to
2853ef1
Compare
dpeng817
force-pushed
the
dpeng817/dpeng817/simplify_module_asset_loader
branch
from
c63cd60 to
9beb0c3
Compare
dpeng817
merged commit 22860d7
into
dpeng817/delete_extra_source_assets
1 check was pending
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## 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
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## 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
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## 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
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## 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
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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