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

Full parametrized decorator #163

Merged
merged 16 commits into from
Aug 15, 2022
Merged

Full parametrized decorator #163

merged 16 commits into from
Aug 15, 2022

Conversation

elijahbenizzy
Copy link
Collaborator

@elijahbenizzy elijahbenizzy commented Jul 30, 2022 •
edited
Loading

Consolidates logic behind all parametrized decorators

Changes

  • adds full_parametrized decorator
  • changes others to use it
  • Adds a smoke_screen_test with a common set of decorators that we use
  • Adds more testing for distributed integrations

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
Adds parametrized_full function modifier
ceda726 
This attempts to unit all the parametrized flavors of
function_modifiers. Specifically, this enables one to specify two source
of inputs:

1. literal
2. upstream

Replacing the function's arguments, as well as direct assignment to a
node name. Note that, currently, docstrngs are provided as part of the
decorator. We will be allowing this to use parametrized docstrings.

This intends to replace all types of parametrized decorators using delegation.
@elijahbenizzy
Adds ability to parametrize docstring to parametrized_full
9fd52dd 
parametrized_full now has two ways of managing documentation:

1. Passing in documentation directly
2. Parametrizing documentation (as we currently do for
   parametrized_inputs)

 (1) is the easiest route and matches @parametrized
(2) allows for us to have parametrizable docstrings.

Note:

1. We use python's str.format(...) function to enable this
2. We automatically replace a docstring parameter with itself if it
   doesn't have a corresponding input parameter. Note that means that
   not providing parametrization for an input is still valid.
@elijahbenizzy
Copy link
Collaborator Author

elijahbenizzy commented Jul 30, 2022 •
edited
Loading

This s a WIP, but its on the right track. Things to do:

  • Simplify the complexity around literal/upstream -- this might not be the best way to do it...
  • Add a @deprecated decorator for decorators
  • Delete the TODO exception
  • Add @parametrized_full to docs
  • Polish code
  • Add decorator tests for the new @parameterized decorator
  • Add tests to ensure upstream and literal both work
  • Test with dask, ray, spark to ensure serialization works
  • Decide on correct spelling for @parametrized (or @parameterize). Since @skrawcz seems to believe there is a right/wrong approach, he will take this task.
    Then in a separate PR, after this is approved
  • Refactor function_modifiers into separate files, and have an __init__.py that joins them
  • Get the @parameterized delegators to work with the upstream and literal functions
  • Determine the right name for upstream and literal
  • Do the same with tests

@elijahbenizzy elijahbenizzy mentioned this pull request Jul 30, 2022
Closed
@elijahbenizzy
Replaces @parameterized variants with delegation to @parametrized_full
eeaa5c6 
We now no longer need bespoke logic. Thus we remove the body of all
variants, and replace with @parametrized_full. We can probably remove
a little validation as well...
@elijahbenizzy elijahbenizzy changed the title Full parametrized Full parametrized decorator Jul 30, 2022
@elijahbenizzy
Changes @parametrized_full to @parameterize
6c46579 
Next we're going to add `@parameterize_inputs` and
`@parameterize_values` to be the new decorators so its:

1. Consistent with the tenses (E.G. extract_columns)
2. Spelled consistently
3. Reduced surface area
@elijahbenizzy
Adds @deprecated decorator
c6d3b52 
Decided to build my own as I wanted to fully manage the lifecycle of
items. Currently meant for dev use but I guess this could also
be used for hamilton functions/nodes? Interesting...

Some things to think about:

1. This allows us to keep something that's deprecated in the future.
   Unlikely to be useful but gives us some freedom.
2. All versions are standard, no funky versions or rc versions (the rc
   gets stripped)
3. This forces us into contracts. Though we can break them, I like the
   idea of making deprecations a planned event.
@elijahbenizzy
Adds deprecation warnings to all parametrization functions except
847d307 
1. @parameterize
2. @parameterize_values
3. @parameterize_inputs

The rest are old and will be phased out eventually.
@elijahbenizzy
Polishing for PR
71d6f4e 
Some small fixes for the PR
@elijahbenizzy
Fixes a bug in which deprecation was called on function definition
3767b23 
It should be called on function call. This, however, adds complexity,
as we need to be able to decorate both classes (that define __call__)
and functions (that happen to have __call__). There is some complexity
that python doesn't handle well -- a third party library like wrapt
might help but I wanted to keep it simple.
@elijahbenizzy elijahbenizzy marked this pull request as ready for review August 5, 2022 22:42
@elijahbenizzy
Copy link
Collaborator Author

Need to run a ray/spark/dask test, but more likely will add some basic integration tests for everything -- E.G> a DAG that uses all our features.

@elijahbenizzy elijahbenizzy requested a review from skrawcz August 5, 2022 22:46
skrawcz
skrawcz reviewed Aug 5, 2022
View reviewed changes
decorators.md Outdated Show resolved Hide resolved
skrawcz
skrawcz reviewed Aug 6, 2022
View reviewed changes
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.

I think we should have a clearer migration section that we can link to so people know what to do and until when.

hamilton/function_modifiers.py
if bad_values:
raise InvalidDecoratorException(f'@parameterize must specify a dependency type -- either upstream() or literal().'
f'The following are not allowed: {bad_values}.')
self.specified_docstrings = {key: value[1] for key, value in parametrization.items() if isinstance(value, tuple)}

def expand_node(self, node_: node.Node, config: Dict[str, Any], fn: Callable) -> Collection[node.Node]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we break this function up at all into smaller testable chunks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't really think its worthwhile? Its about 40 lines, which doesn't seem ridiculous to me, and all kind of does one thing (gathers the nodes). Might be able to do something aroudn the docstirng, but I 've already streamlined that to some extent.

tests/test_dev_utils.py Show resolved Hide resolved
hamilton/function_modifiers.py Outdated Show resolved Hide resolved
decorators.md Outdated Show resolved Hide resolved
decorators.md Show resolved Hide resolved
@skrawcz skrawcz linked an issue Aug 6, 2022 that may be closed by this pull request
Decorator to specify value and dag node inputs #162
Closed
@elijahbenizzy elijahbenizzy force-pushed the full_parametrized branch 21 times, most recently from 5e98ae8 to 68b3165 Compare August 15, 2022 04:34
skrawcz
skrawcz approved these changes Aug 15, 2022
View reviewed changes
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.

Just some documentation nits.

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
@elijahbenizzy
Makes source and value consistent
d658672 
We now have two decorators:

- @parameterize_sources
- @parameterize_values

And two indicator functions for the value-type:

@source
@value

that correspond. See notes in #162
@elijahbenizzy
Adds ray support for functools.partial functions
364833c 
This is a temporary workaround, but its quite stable. In reality, `ray`
should have something like this. Will open up an issue.
@elijahbenizzy
Adds "smoke screen" module + tests to catch more issues with graphada…
fa89410 
…pters

We could probably improve the shared code here, but this is absolutely
worth getting out as is. We test the most common functionalities in all
graphadapters -- they can copy/paste each others tests. See comments in
smoke_screen_module.py for core features.
@elijahbenizzy elijahbenizzy merged commit b06cc2e into main Aug 15, 2022
@elijahbenizzy elijahbenizzy deleted the full_parametrized branch August 15, 2022 14:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skrawcz skrawcz skrawcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decorator to specify value and dag node inputs
2 participants
@elijahbenizzy @skrawcz