Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[pythonic resources] Improve nested resource evaluation logic #14807

Merged
merged 3 commits into from
Jun 27, 2023
Merged

[pythonic resources] Improve nested resource evaluation logic #14807

merged 3 commits into from
Jun 27, 2023

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented Jun 14, 2023 •
edited
Loading

Summary

This PR attempts to address a couple shortcomings with the Pythonic resource system which are exposed in #14751.

First, the logic to determine whether a field on a resource represents a nested resource or a config field is not a good heuristic currently. Right now, the check only assesses whether the value is a resource type - but this fails in the case that a user wants to provide a raw value (ie an instance of IOManager). This PR updates this check to inspect the annotation on the field, and treat it as a resource only if the annotation indicates as much.

Second, the initialization logic for nested resources (coerce_to_resource) doesn't properly handle raw values such as a plain IOManager. This PR updates the logic in coerce_to_resource to call out to wrap_resources_for_execution, which can successfully deal with these cases.

Example

The following resource previously would not accept a raw IOManager as input, only a resource which produces one:

class MyResourceNeedsIOManager(ConfigurableIOManager):
    base_io_manager: ResourceDependency[IOManager]


@io_manager
def my_io_manager(context) -> IOManager:
    pass

# previously OK, since my_io_manager is a resource
MyResourceNeedsIOManager(base_io_manager=my_io_manager)


class MyIOManager(IOManager):
    pass

# previously errors, since MyIOManager is not a resource
MyResourceNeedsIOManager(base_io_manager=MyIOManager())

The new code will coerce MyIOManager to a resource by wrapping it, since it is annotated as a ResourceDependency. This gives the user more flexibility when specifying resource-resource dependencies to input raw values or mocks.

Test Plan

Existing unit tests succeed. New unit tests of nesting behavior with raw values (such as an IOManager).

@benpankow
Copy link
Member Author

benpankow commented Jun 14, 2023 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benpankow benpankow force-pushed the benpankow/fix-nested-resource-logic branch from ddceb2c to 68ad4e6 Compare June 21, 2023 21:44
@github-actions
Copy link

Deploy preview for dagster ready!

Preview available at https://dagster-2yzw6yc1w-elementl.vercel.app

Direct link to changed pages:

@github-actions
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-aiz5rzt1f-elementl.vercel.app

Built with commit 68ad4e6.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

github-actions bot commented Jun 21, 2023 •
edited
Loading

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-md1jqaeni-elementl.vercel.app

Built with commit 929c237.
This pull request is being automatically deployed with vercel-action

@benpankow benpankow mentioned this pull request Jun 21, 2023
Open
@schrockn
Copy link
Member

It would be helpful if you included examples in the PR summary that illustrate the behavior change, especially since you are moving around the tests (instead of doing it in place in which case one could see behavior change as a diff itself)

schrockn
schrockn requested changes Jun 22, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly about documentation. Would love to see tests changed in place so that we can see behavior changes in action, and put in example in the PR summary.

python_modules/dagster/dagster/_core/execution/build_resources.py
Comment on lines +118 to +140
return (
{
resource_key: wrap_resource_for_execution(resource)
for resource_key, resource in resources.items()
}
if resources
else {}
)


def wrap_resource_for_execution(resource: Any) -> ResourceDefinition:
from dagster._config.pythonic_config import ConfigurableResourceFactory, PartialResource

resources = check.opt_mapping_param(resources, "resources", key_type=str)
resource_defs = {}
# Wrap instantiated resource values in a resource definition.
# If an instantiated IO manager is provided, wrap it in an IO manager definition.
for resource_key, resource in resources.items():
# Wrap instantiated resource values in a resource definition.
# If an instantiated IO manager is provided, wrap it in an IO manager definition.
if isinstance(resource, (ConfigurableResourceFactory, PartialResource)):
resource_defs[resource_key] = resource.get_resource_definition()
elif isinstance(resource, ResourceDefinition):
resource_defs[resource_key] = resource
elif isinstance(resource, IOManager):
resource_defs[resource_key] = IOManagerDefinition.hardcoded_io_manager(resource)
else:
resource_defs[resource_key] = ResourceDefinition.hardcoded_resource(resource)

return resource_defs
if isinstance(resource, (ConfigurableResourceFactory, PartialResource)):
return resource.get_resource_definition()
elif isinstance(resource, ResourceDefinition):
return resource
elif isinstance(resource, IOManager):
return IOManagerDefinition.hardcoded_io_manager(resource)
else:
return ResourceDefinition.hardcoded_resource(resource)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a behavior change or a refactor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor, so that we're able to wrap just a single resource in some cases.

@benpankow
Copy link
Member Author

benpankow commented Jun 22, 2023 •
edited
Loading

The existing unit tests weren't modified, just moved to the new file - the main behavior changes are highlighted in new tests in the test_nesting file (the raw_value tests).

Updated the PR description with an example - this is the case that #14751 ran into, since it was relying on IOManagers being supplied which weren't necessarily resources.

@benpankow benpankow force-pushed the benpankow/fix-nested-resource-logic branch from 68ad4e6 to 929c237 Compare June 22, 2023 20:33
@benpankow benpankow requested a review from schrockn June 22, 2023 20:33
schrockn
schrockn approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks

@benpankow benpankow merged commit 625034a into master Jun 27, 2023
@benpankow benpankow deleted the benpankow/fix-nested-resource-logic branch June 27, 2023 17:23
This was referenced Jan 17, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn approved these changes

@yuhan yuhan Awaiting requested review from yuhan

@jamiedemaria jamiedemaria Awaiting requested review from jamiedemaria

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benpankow @schrockn