[core-api][experimental] direct resource passing #26688
Merged
Conversation
This file contains hidden or 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 23, 2024
This was referenced Dec 23, 2024
|
adding @dpeng817 to this one. i'm a little indifferent to GA vs not, so want second opinions here. if we do end up making it public, i think we'll need to add docstring for resource_defs and io_manager_defs. not seeing them currently |
OwenKephart
force-pushed
the
12-23-_core-api_experimental_exclude_subruns
branch
from
85477ee to
29d9049
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
6b3e956 to
19f02c5
Compare
Merged
OwenKephart
force-pushed
the
12-23-_core-api_experimental_exclude_subruns
branch
from
29d9049 to
8124d05
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
19f02c5 to
7412f63
Compare
OwenKephart
changed the base branch from
12-23-_core-api_experimental_exclude_subruns
to
12-23-_core-api_experimental_multi_partition_mappings
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
0a1a10b to
7a9da34
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
7412f63 to
dc7528f
Compare
dpeng817
previously requested changes
Jan 2, 2025
There was a problem hiding this comment.
I actually disagree with this.
I don't think we document direct resource passing anywhere outside of api docs, and it has known problems (specifically key collisions). There might be a few places we use it internally, but in general I think it doesn't fit with how the framework has evolved (you specify resources at the op level but assets at the spec level).
I think if we wanted to make it truly public we would at the very least need to resolve the key collisions problem (having scoped resources be a true concept within dagster) but that's unlikely to be prioritized any time soon.
|
@dpeng817 i'm convinced -- updated |
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
7a9da34 to
7d36f6f
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
dc7528f to
05ee63b
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
7d36f6f to
ac5903a
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
05ee63b to
1a4c938
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
ac5903a to
b1fa256
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
1a4c938 to
e318778
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
b1fa256 to
42fd940
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
e318778 to
759a075
Compare
maximearmstrong
approved these changes
Feb 5, 2025
OwenKephart
force-pushed
the
12-23-_core-api_experimental_multi_partition_mappings
branch
from
42fd940 to
0736e7c
Compare
OwenKephart
force-pushed
the
12-23-_core-api_experimental_direct_resource_passing
branch
from
759a075 to
f2b0f84
Compare
## Summary & Motivation decision: experimental -> GA reason: this has existed for a long time, we need to support it for the new asset selection syntax, and there's no reason that we would not want to support this behavior docs exist: yes (API only, which is ok) ## How I Tested These Changes ## Changelog > Insert changelog entry or delete this section.
OwenKephart
merged commit aa4ba14
into
12-23-_core-api_experimental_multi_partition_mappings
1 check was pending
OwenKephart
deleted the
12-23-_core-api_experimental_direct_resource_passing
branch
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
decision: experimental -> public (could be convinced otherwise)
reason: this has existed for a long time, and even if it isn't directly recommended in most cases at the moment, we're moving to a world where more locally-scoped resource definitions are the norm and so this seems fair to support.
docs exist: api only
How I Tested These Changes
Changelog