Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Fixes for @does #187

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Fixes for @does #187

merged 3 commits into from
Aug 30, 2022

Conversation

elijahbenizzy
Copy link
Collaborator

Need to add a few more pieces to make this work -- this is a WIP.

[Short description explaining the high-level reason for the pull request]

Changes

Testing

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 can be automatically merged (no conflicts)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Passes all existing automated tests
  • 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.
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Python - local testing

  • python 3.6
  • python 3.7

@elijahbenizzy elijahbenizzy force-pushed the fix-does-bug branch 13 times, most recently from 09d52bd to b9d3663 Compare August 26, 2022 04:18
@elijahbenizzy
Copy link
Collaborator Author

Ughh this took longer than I want but I want to add the ability to have it work with *args. That said, i think this is good enough for now and I want some more user feedback before investing further.

@elijahbenizzy elijahbenizzy changed the title Fixes bug for @does working with optional dependencies #185 Fixes for @does Aug 26, 2022
@elijahbenizzy elijahbenizzy marked this pull request as ready for review August 26, 2022 04:26
@skrawcz
Copy link
Collaborator

skrawcz commented Aug 26, 2022

@elijahbenizzy guess we need to fix the ray workflow API... seems like it changed.

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.

haven't finished look at code just yet -- but found doc string issues.

decorators.md Outdated Show resolved Hide resolved
decorators.md Outdated Show resolved Hide resolved
decorators.md Outdated Show resolved Hide resolved
decorators.md Outdated Show resolved Hide resolved
decorators.md Outdated Show resolved Hide resolved
hamilton/dev_utils/deprecation.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
@@ -85,8 +85,8 @@ def source(dependency_on: Any) -> UpstreamDependency:
This means that it comes from a node somewhere else.
E.G. source("foo") means that it should be assigned the value that "foo" outputs.

@param dependency_on: Upstream node to come from
@return:An UpstreamDependency object -- a signifier to the internal framework of the dependency type.
:paramdependency_on: Upstream node to come from
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing whitespace

hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
hamilton/function_modifiers.py Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
…eadable.

Adds the following features for #186:

1. The ability to have more complex arguments
2. The ability to have an argument mapping

All of this is documented.

Note this also fixes it breaking with optional dependencies (#185)
 This enables more complex polymorphically applicable functions without
the pain of implementing sophisticated type-checking.

Furthermore, I think this was overreach by the Hamilton framework.
Hamilton cares about relationships between nodes, and not the
implementation of nodes. As `@does` creates a node, this does not impact
the relationship between nodes, meaning that this type of type-checking
is out of scope.
decorators.md Outdated Show resolved Hide resolved
@elijahbenizzy elijahbenizzy merged commit 289e516 into main Aug 30, 2022
@elijahbenizzy elijahbenizzy deleted the fix-does-bug branch August 30, 2022 04:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants