[pythonic resources][fix] fix setup/teardown behavior w/ nested pythonic resources #19852
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| @@ -399,6 +399,7 @@ def _is_cm_resource_cls(cls: Type["ConfigurableResourceFactory"]) -> bool: | |||
| return ( | |||
| cls.yield_for_execution != ConfigurableResourceFactory.yield_for_execution | |||
| or cls.teardown_after_execution != ConfigurableResourceFactory.teardown_after_execution | |||
| or len(_get_resource_param_fields(cls)) > 0 | |||
There was a problem hiding this comment.
does this equate to "if there are any resources as params, assume they are context managers"? Is that the case because they always are in the base class?
edit: not sure my question actually makes sense but i think it points towards putting a comment here explaining whats going on
There was a problem hiding this comment.
It basically equates to "if this resource has any other nested resources, let's assume that they're context managers, and treat this resource as a context manager"
This boolean affects a few things:
-
The underlying resource function for the
ResourceDefinitioncreated from thisConfigurableResource- it's a CM function ifTrue, not a CM ifFalse(the source of the bug this PR addresses). This is largely opaque to the user, so biasing towardsTrueis low risk. -
A few utility methods, like
from_resource_context- this method is typically used for migrations from function-style resources which construct the Pythonic-style resource. These for the most part shouldn't use nesting:class FancyDbResource(ConfigurableResource): conn_string: str @resource(config_schema=FancyDbResource.to_config_schema()) def fancy_db_resource(context: InitResourceContext) -> FancyDbResource: return FancyDbResource.from_resource_context(context)
There was a problem hiding this comment.
got it - think its worth a comment
| @@ -399,6 +399,7 @@ def _is_cm_resource_cls(cls: Type["ConfigurableResourceFactory"]) -> bool: | |||
| return ( | |||
| cls.yield_for_execution != ConfigurableResourceFactory.yield_for_execution | |||
| or cls.teardown_after_execution != ConfigurableResourceFactory.teardown_after_execution | |||
| or len(_get_resource_param_fields(cls)) > 0 | |||
There was a problem hiding this comment.
got it - think its worth a comment
benpankow
force-pushed
the
benpankow/fix-setup-teardown-nesting-cm
branch
from
51f761f to
a9d4637
Compare
|
Deploy preview for dagster-university ready! ✅ Preview Built with commit a9d4637. |
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit a9d4637. |
|
Thank you ❤️ |
…nic resources (#19852) ## Summary Addresses #19277 and #14112. When nested in a non-context-manager resource (e.g. no `teardown_after_execution` or `yield_for_execution` override), context manager resources were not properly opened before step execution and closed afterwards, meaning their teardown logic was invoked immediately. This PR treats all resources with nested resources (which may or may not be context managers) as context managers as a safeguard to prevent this from happening. ## Test Plan Unit tests of the nested-yield and nested-teardown case, both which previously failed.
Summary
Addresses #19277 and #14112.
When nested in a non-context-manager resource (e.g. no
teardown_after_executionoryield_for_executionoverride), context manager resources were not properly opened before step execution and closed afterwards, meaning their teardown logic was invoked immediately.This PR treats all resources with nested resources (which may or may not be context managers) as context managers as a safeguard to prevent this from happening.
Test Plan
Unit tests of the nested-yield and nested-teardown case, both which previously failed.