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

delayed evaluation of annotations causes type mismatches #91

Closed
gravesee opened this issue Mar 2, 2023 · 8 comments
Closed

delayed evaluation of annotations causes type mismatches #91

gravesee opened this issue Mar 2, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@gravesee
Copy link

gravesee commented Mar 2, 2023

Current behavior

importing annotations from __future__ causes externally provided nodes to fail type validation.

Stack Traces

Traceback (most recent call last):
  File "c:\Users\gravesee\projects\dsbsvc-dsgtools-modgov\scratch.py", line 21, in <module>
    dr.execute(["dep"])
  File "c:\Users\gravesee\projects\dsbsvc-dsgtools-modgov\.venv\Lib\site-packages\hamilton\driver.py", line 261, in execute
    raise e
  File "c:\Users\gravesee\projects\dsbsvc-dsgtools-modgov\.venv\Lib\site-packages\hamilton\driver.py", line 254, in execute
    outputs = self.raw_execute(_final_vars, overrides, display_graph, inputs=inputs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\Users\gravesee\projects\dsbsvc-dsgtools-modgov\.venv\Lib\site-packages\hamilton\driver.py", line 354, in raw_execute
    self.validate_inputs(
  File "c:\Users\gravesee\projects\dsbsvc-dsgtools-modgov\.venv\Lib\site-packages\hamilton\driver.py", line 225, in validate_inputs
    raise ValueError(error_str)
ValueError: 1 errors encountered:
  Error: Type requirement mismatch. Expected dep:str got hello instead.

Screenshots

(If applicable)

Steps to replicate behavior

from __future__ import annotations

def func1(dep: str) -> str:
    return dep

def func2(dep: str) -> str:
    return dep

if __name__ == "__main__":
    import __main__ as main
    
    from hamilton.driver import Driver

    config = {
        "dep": "hello"
    }

    dr = Driver(config, main)
    dr.execute(["dep"])

Library & System Information

python 3.11.1
sf-hamilton==1.18.0
Windows 11

Expected behavior

The above code works without any issues by commenting out the from __future__ import annotations line.

Additional context

I prefer some type annotation patterns that aren't supported in earlier versions of python. Yet my team needs to target python 3.9+. I imported annotations to make use of the nicer type union syntax.

@gravesee gravesee added the triage label for issues that need to be triaged. label Mar 2, 2023
@elijahbenizzy
Copy link
Collaborator

elijahbenizzy commented Mar 2, 2023

Nice find. Also want to point out that I really like the import __main__ as main trick.

Pretty sure the fix is easy but will take a bit of time -- we need to use typing.get_type_hints(fn) instead of inspect.signature(fn) -- i think this should work in all cases, but the devil is in the details.

Happy to take a stab at this.

~~Out of my own curiosity -- are you making use of delayed type annotations much? ~~ Ahh just saw the additional context

Also I think this specific case might be slightly easier to fix -- digging in to see if there's a quick change

@elijahbenizzy
Copy link
Collaborator

elijahbenizzy commented Mar 2, 2023

OK -- have a quick POC that works with your example. Mind giving the branch a spin? #92.

Something like: pip install git+https://github.com/dagworks-inc/hamilton.git@issue_91 will get it installed.

For productionalizing this I'll add testing with the __future__ import as well.

@gravesee
Copy link
Author

gravesee commented Mar 3, 2023

~~Out of my own curiosity -- are you making use of delayed type annotations much? ~~ Ahh just saw the additional context

My team mostly uses them for classmethods or builder-pattern type methods that return self. We use mypy in pre-commit with fairly conservative settings and it complains about methods without return types. We could use a string as the return type, but the delayed evaluation is supposed to land in python eventually -- although it seems like that's not the case any more.

@gravesee
Copy link
Author

gravesee commented Mar 3, 2023

OK -- have a quick POC that works with your example. Mind giving the branch a spin? #92.

Something like: pip install git+https://github.com/dagworks-inc/hamilton.git@issue_91 will get it installed.

For productionalizing this I'll add testing with the __future__ import as well.

Yes! The code that was previously failing is now running without issue. This behavior was a real adventure to nail down.

@elijahbenizzy
Copy link
Collaborator

OK -- have a quick POC that works with your example. Mind giving the branch a spin? #92.
Something like: pip install git+https://github.com/dagworks-inc/hamilton.git@issue_91 will get it installed.
For productionalizing this I'll add testing with the __future__ import as well.

Yes! The code that was previously failing is now running without issue. This behavior was a real adventure to nail down.

Awesome! Will be getting this out into production shortly -- sorry you had to spend time debugging -- going to be adding some tests to make sure our suite works with the new annotations feature (need to double-check a few extensions.

@skrawcz skrawcz added bug Something isn't working and removed triage label for issues that need to be triaged. labels Mar 3, 2023
@elijahbenizzy
Copy link
Collaborator

OK, this has been merged and we'll be planning to release. Thanks for finding the bug! #92

@skrawcz
Copy link
Collaborator

skrawcz commented Mar 5, 2023

@gravesee would you mind validating things work with pip install sf-hamilton==1.19.0rc0 please?

@gravesee
Copy link
Author

gravesee commented Mar 5, 2023

sf-hamilton==1.19.0rc0

Works on 1.19.0rc0 as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants