[module-loaders] Genericize object list utils #26535
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
This was referenced Dec 17, 2024
|
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. |
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
7ec766f to
f2e7f3d
Compare
This was referenced Dec 17, 2024
[module-loaders] Genericize object list functionality to take in all sensors, jobs, schedules
#26545
Merged
dpeng817
force-pushed
the
dpeng817/key_iterator_removal
branch
from
8831b0b to
db2f79a
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
f2e7f3d to
a67444d
Compare
dpeng817
force-pushed
the
dpeng817/key_iterator_removal
branch
from
db2f79a to
091517b
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
a67444d to
fa8dff1
Compare
dpeng817
changed the title
OwenKephart
previously requested changes
Dec 18, 2024
| @@ -9,8 +9,11 @@ | |||
| from dagster._core.definitions.cacheable_assets import CacheableAssetsDefinition | |||
| from dagster._core.definitions.source_asset import SourceAsset | |||
|
|
|||
| LoadableAssetTypes = Union[AssetsDefinition, AssetSpec, SourceAsset, CacheableAssetsDefinition] | |||
| KeyScopedAssetObjects = (AssetsDefinition, AssetSpec, SourceAsset) | |||
| LoadableAssetObject = Union[AssetsDefinition, AssetSpec, SourceAsset, CacheableAssetsDefinition] | |||
There was a problem hiding this comment.
I think Object -> Def in all of these names would make more sense
| @@ -46,17 +47,17 @@ def from_modules(cls, modules: Iterable[ModuleType]) -> "LoadedAssetsList": | |||
| ) | |||
|
|
|||
| @cached_property | |||
| def flat_object_list(self) -> Sequence[LoadableAssetTypes]: | |||
| def flat_object_list(self) -> Sequence[LoadableDagsterObject]: | |||
There was a problem hiding this comment.
e.g.
def dlat_defs_list(self) -> Sequence[LoadableDagsterDef]:
dpeng817
force-pushed
the
dpeng817/key_iterator_removal
branch
from
091517b to
49c6d6b
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
fa8dff1 to
d8e8a8f
Compare
dpeng817
force-pushed
the
dpeng817/key_iterator_removal
branch
from
49c6d6b to
51512e6
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
d8e8a8f to
01f281a
Compare
dpeng817
force-pushed
the
dpeng817/key_iterator_removal
branch
from
51512e6 to
3553a63
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
01f281a to
f7eb224
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
f1016a1 to
2b7cdbb
Compare
dpeng817
changed the base branch from
dpeng817/key_iterator_removal
to
dpeng817/split_load_file
dpeng817
commented
Dec 19, 2024
There was a problem hiding this comment.
@OwenKephart so I'd like to push back for the asset case; it's very hard to reason about assets since there is AssetsDefinition, SourceAsset and AssetSpec, and those can all have the same values. I think that that SourceAsset and AssetSpec falling into a defs_list is a bit strange, but maybe that's saying more about the naming of those APIs than anything else.
I don't feel that strongly about it but I'd request that if you really want the name change that we do it in a follow up; turns into an absolutely nightmarish rebase (changes like every line of the object_list.py file)
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
9e56100 to
143aa7b
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
2b7cdbb to
d2db161
Compare
|
I do like But this isn't extremely publicly facing and we are in a release freeze zone, so I think landing + follow up for name changes is reasonable. (you have my verbal stamp. i will let owen approve in case of any strong reason not to) |
|
I'm fine with making the change in a follow up |
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
143aa7b to
67ee4a3
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
d2db161 to
6bc53ed
Compare
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
67ee4a3 to
370277b
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
6bc53ed to
a1e7c18
Compare
OwenKephart
requested changes
Dec 19, 2024
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
370277b to
349e3bd
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
a1e7c18 to
e9d0bba
Compare
|
We're aligned @OwenKephart - I'll make the naming changes in a followup |
OwenKephart
approved these changes
Dec 19, 2024
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
349e3bd to
a10d824
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
e9d0bba to
b1f2469
Compare
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
a10d824 to
b5065bf
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
b1f2469 to
d4430a7
Compare
dpeng817
force-pushed
the
dpeng817/split_load_file
branch
from
b5065bf to
41ec0ab
Compare
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
d4430a7 to
3f706f1
Compare
Base automatically changed from
dpeng817/split_load_file
to
dpeng817/delete_extra_source_assets
December 19, 2024 16:52
dpeng817
force-pushed
the
dpeng817/genericize_typehints_and_methods
branch
from
3f706f1 to
73810c7
Compare
dpeng817
merged commit 0405939
into
dpeng817/delete_extra_source_assets
1 check was pending
dpeng817
added a commit
that referenced
this pull request
Dec 19, 2024
## Summary & Motivation The object list utils were previously defining behavior mostly in terms of assets. Genericize the utilities to instead refer to "DagsterObjects" where appropriate; in preparation for handling other types of dagster definitions. ## 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
The object list utils were previously defining behavior mostly in terms of assets. Genericize the utilities to instead refer to "DagsterObjects" where appropriate; in preparation for handling other types of dagster definitions.
How I Tested These Changes
Existing tests