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

Clarify behavior of decorator ordering #249

Closed
skrawcz opened this issue Dec 18, 2022 · 1 comment
Closed

Clarify behavior of decorator ordering #249

skrawcz opened this issue Dec 18, 2022 · 1 comment
Labels
triage label for issues that need to be triaged.

Comments

@skrawcz
Copy link
Collaborator

skrawcz commented Dec 18, 2022

We need to make clear our philosophy and resolution method for functions such as:

@extract_fields({'out_value1': int, 'out_value2': str})
@tag(test_key="test-value")
@check_output(data_type=dict, importance="fail")
@does(_dummy)
def uber_decorated_function(in_value1: int, in_value2: str) -> dict:
    pass

Right now it is not clear, nor obvious.

Current behavior

This is what the graph looks like:

Screen Shot 2022-12-17 at 5 24 42 PM

So it would be unexpected to see check_output over the output of extract_fields.

Steps to replicate behavior

Function code:

def _dummy(**values) -> dict:
    return {f"out_{k.split('_')[1]}": v for k, v in values.items()}


@extract_fields({'out_value1': int, 'out_value2': str})
@tag(test_key="test-value")
@check_output(data_type=dict, importance="fail")
@does(_dummy)
def uber_decorated_function(in_value1: int, in_value2: str) -> dict:
    pass

Expected behavior

check_output should probably operate over what's directly underneath that.
tag similarly should apply to all? or just what's underneath?
does should apply to uber_decorated_function
extract_fields is the last thing that's applied?

Additional context

Thoughts: can we create a linter that reorders decorators?

@skrawcz skrawcz added the triage label for issues that need to be triaged. label Dec 18, 2022
skrawcz added a commit that referenced this issue Dec 18, 2022
Adds a test to cover Elijah's changes.

In the process I created #249.
Since I think we need to add more unit tests around decorator interactions.
skrawcz added a commit that referenced this issue Dec 18, 2022
Cleaner fix for `@does`. The function is only useful for the node generator
 -- not for the rest of DAG creation. This ensures that we don't use it
to handle signatures, rather, we just use the node's return types.

This also adds unit tests, and in the process opened #249 since we need
better clarity and test coverage on decorator interactions.

---- Squashed commits ----
* Fixes does function wrapping #244

In issue #244 types were not being propagated of
the function being wrapped.

This changes that by adding the functools.wraps
decorator.

Adds a unit test to test for two decorators playing nice
together and verified that it's broken before and fixed
afterwards.

* Removes print statement

* Cleaner fix for `@does`. The function is only useful for the

node generator -- not for the rest. This ensures that we don't use it
to handle signatures, rather, we just use the node's return types.

* Adds unit test to cover extract_fields with does

Adds a test to cover Elijah's changes.

In the process I created #249.
Since I think we need to add more unit tests around decorator interactions.

Co-authored-by: Stefan Krawczyk <stefank@cs.stanford.edu>
@elijahbenizzy
Copy link
Collaborator

We are moving repositories! Please see the new version of this issue at DAGWorks-Inc/hamilton#57. Also, please give us a star/update any of your internal links.

Note that everything else (slack community, pypi packages, etc...) will not change at all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
triage label for issues that need to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants