-
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
[core-api][experimental] direct resource passing #26688
base: 12-23-_core-api_experimental_multi_partition_mappings
Are you sure you want to change the base?
[core-api][experimental] direct resource passing #26688
Conversation
b31ab31
to
85477ee
Compare
70ceb12
to
6b3e956
Compare
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 |
85477ee
to
29d9049
Compare
6b3e956
to
19f02c5
Compare
29d9049
to
8124d05
Compare
19f02c5
to
7412f63
Compare
0a1a10b
to
7a9da34
Compare
7412f63
to
dc7528f
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.
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.
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