Skip to content

Fix @resolve decorator not calling validate() on returned decorators#1524

Open
jmarshrossney wants to merge 3 commits intoapache:mainfrom
jmarshrossney:fix-resolve-extract
Open

Fix @resolve decorator not calling validate() on returned decorators#1524
jmarshrossney wants to merge 3 commits intoapache:mainfrom
jmarshrossney:fix-resolve-extract

Conversation

@jmarshrossney
Copy link
Copy Markdown

Hello!

This PR makes a trivial change to the delayed evaluation decorator (@resolve) so that it works with decorators such as @extract_fields which require their validate() method to have been called. Concretely, the "fix" is to call validate() (if it is defined) in resolve.resolve, before returning it.

Prior to this change, using @resolve with @extract_fields results in an AttributeError: 'extract_fields' object has no attribute 'resolved_fields'.

Changes

Mainly..

  • Call decorator.validate() before returning the decorator from resolve.resolve.
  • Add a unit test to test the combination of resolve and extract_fields specifically

Also..

  • Tweaked other unit tests in the same module which were failing after this change; performing the validation step checks for proper function annotations, so added dummy annodated functions inside these tests.
  • Removed an unreachable assert from test_dynamic_fails_without_power_mode_fails, and renamed it test_dynamic_resolve_...

How I tested this

pytest tests/functions_modifieds/test_delayed.py

Notes

Intended to fix #1409 - I have the same goals/issues.

This is my first contribution to this project. As a novice, the CONTRIBUTING.md guidelines could do with more detail! Note that the link to the developer setup guide appears broken.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • (N/A) New functions are documented (with a description, list of inputs, and expected output)
  • (N/A) Placeholder code is flagged / future TODOs are captured in comments
  • (N/A) Project documentation has been updated if adding/changing functionality.

Copy link
Copy Markdown
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Nice, clean bugfix! The root cause is well-identified: dynamically resolved decorators (via @resolve) skip the normal NodeTransformLifecycle.__call__() path where validate() is invoked, so decorators like @extract_fields that rely on validate() to set up internal state (e.g., resolved_fields) were broken.

The fix is minimal and correct — calling validate(fn) in resolve.resolve() before returning the decorator fills the lifecycle gap. Tests are well-structured and cover the specific failure case.

One minor suggestion below, otherwise LGTM. I'll approve and merge in a couple of days in case you want to address any of the comments first.

return self.decorate_with(**kwargs)
decorator = self.decorate_with(**kwargs)
if hasattr(decorator, "validate"):
decorator.validate(fn)
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.

Minor: Since decorate_with is typed as Callable[..., NodeTransformLifecycle] and NodeTransformLifecycle always defines validate(), the hasattr check will always be True. You could simplify to just decorator.validate(fn). That said, the defensive check is reasonable as a guard against non-conforming lambdas, so this is just a nit — either way is fine.

fn=fn,
)
assert hasattr(decorator_resolved, "resolved_fields")
assert decorator_resolved.resolved_fields == {"a": int, "b": int}
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.

Good test! One small thought: it might be worth adding a negative test case that verifies resolve() properly propagates a validate() failure (e.g., passing a function with the wrong return type to extract_fields). This would confirm the error path works correctly through the dynamic resolution. Not blocking — the happy path coverage is sufficient for this fix.

@jmarshrossney
Copy link
Copy Markdown
Author

Hi @skrawcz . Thanks for the quick and helpful feedback!

I don't have a strong opinion on the "defensive check". If left up to me I'll leave it in with a note that makes exactly the point you just made. Tbh I'm still getting used to the idea that type annotations are in any way enforceable in a python project!

More than happy to spend a little more time on tests. Might take a few days to get round to it though.

Cheers!

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.

Dynamic DAG build fails

2 participants