Skip to content

fix(pythonic-resources): ResourceDependency with default causes RecursionError#33668

Open
gambletan wants to merge 1 commit intodagster-io:masterfrom
gambletan:fix/33650-resource-dependency-default-recursion
Open

fix(pythonic-resources): ResourceDependency with default causes RecursionError#33668
gambletan wants to merge 1 commit intodagster-io:masterfrom
gambletan:fix/33650-resource-dependency-default-recursion

Conversation

@gambletan
Copy link
Copy Markdown

Summary

Fixes #33650.

When a ConfigurableResource subclass instance is used as a class-level default for a ResourceDependency field, accessing that field triggered infinite recursion:

class Outer(ConfigurableResource):
    inner: ResourceDependency[Inner] = Inner()  # Inner() is a data descriptor

    def greet(self):
        return self.inner.val  # RecursionError

Root cause: ConfigurableResourceFactory inherits __get__/__set__ from TypecheckAllowPartialResourceInitParams, making every ConfigurableResource instance a Python data descriptor. When Inner() is stored as a class attribute of Outer, accessing outer.inner called TypecheckAllowPartialResourceInitParams.__get__ which called getattr(obj, self._assigned_name), which re-triggered __get__ — infinite recursion.

A secondary effect was that Pydantic could not read the Inner() class-level default (because Inner().__get__(None, Outer) raised AttributeError before the fix), causing _nested_resources to be empty and the nested resource's lifecycle to be skipped.

Fix — two changes:

  1. TypecheckAllowPartialResourceInitParams.__get__/__set__ (typing_utils.py):

    • __get__: return self when obj is None (class-level access) so Pydantic can read the default. When obj is an instance, read from obj.__dict__ directly to bypass the descriptor protocol.
    • __set__: use object.__setattr__ instead of setattr to avoid re-entering the descriptor.
  2. ConfigurableResourceFactory.__init__ (resource.py):
    After super().__init__(), Pydantic has populated self.__dict__ with all values including defaults. Re-run separate_resource_params over self.__dict__ to pick up resource-typed fields populated from defaults, ensuring they are tracked in _nested_resources and resolved at execution time.

Test plan

  • New regression test test_resource_dependency_with_default_no_recursion covers sensor context
  • New test test_resource_dependency_with_default_in_asset covers asset execution
  • All existing resource tests pass (243 passed including 2 new)
  • All pythonic config tests pass (140 passed, 1 skipped)

🤖 Generated with Claude Code

Fixes dagster-io#33650.

When a `ConfigurableResource` subclass instance is used as a default
value for a `ResourceDependency` field:

    class Outer(ConfigurableResource):
        inner: ResourceDependency[Inner] = Inner()

The `Inner()` instance was stored as a class attribute of `Outer`.
Because `ConfigurableResourceFactory` inherits `__get__`/`__set__` from
`TypecheckAllowPartialResourceInitParams`, every `ConfigurableResource`
instance is a Python *data descriptor*.  Accessing `outer.inner` therefore
triggered `TypecheckAllowPartialResourceInitParams.__get__`, which called
`getattr(obj, self._assigned_name)`, which re-triggered `__get__` ad
infinitum.

Two changes fix this:

1. **`typing_utils.py` – `TypecheckAllowPartialResourceInitParams`**
   - `__get__`: when `obj is None` (class-level access) return `self` so
     Pydantic can read the default correctly.  When `obj` is an instance,
     read from `obj.__dict__` directly to bypass the descriptor protocol
     and break the recursion.
   - `__set__`: use `object.__setattr__` instead of `setattr` so that
     writing a value to an instance attribute also bypasses the descriptor
     protocol.

2. **`resource.py` – `ConfigurableResourceFactory.__init__`**
   After `super().__init__()` Pydantic has populated the instance `__dict__`
   with all field values (including defaults it can now read thanks to fix 1).
   Re-running `separate_resource_params` over `self.__dict__` picks up any
   resource-typed fields populated from defaults, ensuring they are tracked
   in `_nested_resources` and correctly resolved at execution time.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR fixes a RecursionError that occurred when a ConfigurableResource subclass instance was used as a class-level default for a ResourceDependency field. The root cause was that every ConfigurableResource instance is a Python data descriptor (via TypecheckAllowPartialResourceInitParams), so storing one as a class attribute caused __get__getattr(obj, name)__get__ → … infinite recursion.

Changes:

  • typing_utils.py__get__ now short-circuits on class-level access (obj is None) by returning self, allowing Pydantic to read the class default. For instance access it reads from obj.__dict__ directly, bypassing the descriptor protocol and eliminating the recursion. __set__ is updated to use object.__setattr__ for the same reason.
  • resource.py — After super().__init__() (when Pydantic has populated all defaults), the __init__ of ConfigurableResourceFactory re-scans self.__dict__ with separate_resource_params to pick up any resource-typed fields that were set from class-level defaults. This ensures they are tracked in _nested_resources and resolved at execution time. The merge order ({**all_resource_pointers, **resource_pointers}) correctly lets explicitly provided values override defaults.
  • test_nesting.py — Two new regression tests: one covering sensor context, one covering asset execution, verifying both the recursion fix and that explicit overrides continue to work.

Confidence Score: 5/5

  • Safe to merge — the fix is targeted, correct, and well-tested with no behaviour changes outside the broken default-descriptor path.
  • The root cause is accurately diagnosed and both parts of the fix (descriptor __get__/__set__ and the post-init re-scan) are correct. The merge order in the re-scan preserves explicit overrides. Existing tests all pass alongside two new regression tests covering the exact failure modes. The only findings are a dead-code else branch in __set__, a stray blank line, and a duplicate test pattern — none of which affect correctness or production reliability.
  • No files require special attention.

Important Files Changed

Filename Overview
python_modules/dagster/dagster/_config/pythonic_config/typing_utils.py Fixed recursive descriptor loop: __get__ now returns self on class-level access and reads from obj.__dict__ to bypass the protocol. __set__ uses object.__setattr__, but contains a dead-code else branch.
python_modules/dagster/dagster/_config/pythonic_config/resource.py Post-super().__init__() re-scan of self.__dict__ correctly picks up resource-typed fields populated from class-level defaults; merge order {**all_resource_pointers, **resource_pointers} properly preserves explicit overrides. Minor extra blank line.
python_modules/dagster/dagster_tests/core_tests/resource_tests/pythonic_resources/test_nesting.py Two new regression tests covering sensor context and asset execution paths for the fix; solid coverage of default, explicit-override, and nested-resource-tracking scenarios. Minor redundancy between Pattern 1 and Pattern 4.

Sequence Diagram

sequenceDiagram
    participant User
    participant OuterResource
    participant Pydantic
    participant TypecheckDescriptor as TypecheckAllowPartialResourceInitParams.__get__
    participant InstanceDict as obj.__dict__

    Note over User,InstanceDict: BEFORE fix — RecursionError
    User->>OuterResource: outer.inner
    OuterResource->>TypecheckDescriptor: __get__(obj=outer, owner=Outer)
    TypecheckDescriptor->>TypecheckDescriptor: getattr(outer, "inner")
    TypecheckDescriptor->>TypecheckDescriptor: __get__(obj=outer, owner=Outer)
    TypecheckDescriptor-->>TypecheckDescriptor: ∞ RecursionError

    Note over User,InstanceDict: AFTER fix — correct resolution
    User->>OuterResource: outer.inner
    OuterResource->>TypecheckDescriptor: __get__(obj=outer, owner=Outer)
    TypecheckDescriptor->>InstanceDict: obj.__dict__["inner"]
    InstanceDict-->>TypecheckDescriptor: InnerResource instance
    TypecheckDescriptor-->>User: InnerResource instance

    Note over User,InstanceDict: Class-level access (Pydantic reading default)
    Pydantic->>TypecheckDescriptor: __get__(obj=None, owner=Outer)
    TypecheckDescriptor-->>Pydantic: self (descriptor/default InnerResource instance)
Loading

Reviews (1): Last reviewed commit: "fix: ResourceDependency with default no ..." | Re-trigger Greptile

Comment on lines +235 to +238
if obj is not None:
object.__setattr__(obj, self._assigned_name, value)
else:
setattr(obj, self._assigned_name, value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Dead-code else branch in __set__

Python's descriptor protocol never calls __set__ with obj=None__set__ is only invoked on instances, not via class-level attribute assignment (which would replace the descriptor in the class __dict__ directly). The else branch therefore can never be reached, and if it somehow were, setattr(None, …) would raise TypeError.

Consider simplifying to just the guard:

Suggested change
if obj is not None:
object.__setattr__(obj, self._assigned_name, value)
else:
setattr(obj, self._assigned_name, value)
if obj is not None:
object.__setattr__(obj, self._assigned_name, value)

Comment on lines 921 to +924
return out



Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Extra blank line between top-level functions

Three blank lines are present between separate_resource_params and _call_resource_fn_with_default; PEP 8 recommends two.

Suggested change
return out
return out
def _call_resource_fn_with_default(

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +1202 to +1208
# Pattern 4: sensor resolves the nested resource correctly end-to-end
# (access via context.resources to verify full resource initialization path)
with dg.build_sensor_context(
resources={"outer": OuterResource()}, sensor_name="test_sensor_33650"
) as ctx:
outer = ctx.resources.outer
assert outer.greet() == "hello"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Duplicate test pattern

"Pattern 4" is identical to "Pattern 1" — both create a build_sensor_context with OuterResource() and assert outer.greet() == "hello". Consider replacing with a distinct scenario (e.g. verifying the sensor body runs via list(my_sensor(ctx))) or removing it to reduce noise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ResourceDependency with default causes RecursionError in sensor context

1 participant