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.

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

add a "resolve from config" wrapper around the resolve decorator #828

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

janhurst
Copy link
Contributor

@janhurst janhurst commented Apr 17, 2024

Add a small wrapper around resolve function modifier to "resolve from config"

Changes

Add a hamilton.function_modifiers.resolve_from_config hook that just passes ResolveAt.CONFIG_AVAILABLE flag to the original resolve class.

How I tested this

Have been using this as a small custom snippet (albeit not as a wrapper around the class) in some internal projects for quite a while. Its really just a convenience wrapper to make my pipeline code a little cleaner.

I've made a very small attempt to duplicate a relevant unit test.

Notes

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
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@elijahbenizzy elijahbenizzy self-requested a review April 17, 2024 16:28
Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Looking good! Minor change for docstring (and maybe name) then let's merge.

@@ -148,3 +148,8 @@ def resolve(self, config: Dict[str, Any], fn: Callable) -> NodeTransformLifecycl
if key in config:
kwargs[key] = config[key]
return self.decorate_with(**kwargs)


class resolve_from_config(resolve):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thought from @zilto -- would @resolve_at_config be cleaner?

Also, let's add a docstring then stick it in the decorators documentation.

See https://github.com/DAGWorks-Inc/hamilton/blob/main/docs/reference/decorators/resolve.rst -- should be easy!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @resolve_at_config better capture that it's a subset of @resolve where we set the value of ResolveAt to ConfigAvailable.

There's no other ResolveAt value, but I think the semantic make sense. I believe the name @resolve_at_config is an improvement over @resolve as it better communicate it's role during the Driver building.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 just to voice my opinion that resolve_from_config doesn't need you to understand what the driver does, but it does help more clearly tie the connection that it comes from configuration passed in. Which I think for a new person to Hamilton that might be more intuitive, than resolve_at_config since now they need to understand what "at" might be referring to.

@janhurst thoughts on the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

realistically im pretty ambivalent...

i used @resolve_from_config for the reason @skrawcz mentioned... it described to me where the resolve was obtaining information

I think @resolve_at_config better capture that it's a subset of @resolve where we set the value of ResolveAt to ConfigAvailable.

the other way to look at this is to just default the when of @resolve to the only value it can use right now :) ... this is practically why i wrote my original wrapper functions anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

the other way to look at this is to just default the when of @resolve to the only value it can use right now :) ... this is practically why i wrote my original wrapper functions anyway

We could do that. But would that be clear where the values come from? That's only reason we did such a verbose API in the first place -- to make it easier to read... 😆

@janhurst
Copy link
Contributor Author

Looking good! Minor change for docstring (and maybe name) then let's merge.

added a little bit of a redirect to the @resolve docs... feedback greatly appreciated

Copy link
Collaborator

@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.

This works for me.

@skrawcz skrawcz merged commit 4a0694e into DAGWorks-Inc:main Apr 22, 2024
23 checks passed
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.

4 participants