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

Adds union support check when passing in inputs #173

Merged
merged 4 commits into from
Aug 15, 2022

Conversation

skrawcz
Copy link
Collaborator

@skrawcz skrawcz commented Aug 8, 2022

This should fix #170. If an input function was annotated
with a union, we would barf. Now it wont.

This still means downstream nodes need to have the same
type signature -- this doesn't change any behavior there.

Changes

  • adds support to expand union type when checking inputs

Testing

  1. Unit tests work.

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

@skrawcz
Copy link
Collaborator Author

skrawcz commented Aug 8, 2022

This code works

import typing as t
import pandas as pd
def foo_union(x: t.Union[int, pd.Series]) -> t.Union[int, pd.Series]:
    '''foo union'''
    return x + 1
from hamilton.driver import Driver
from hamilton.base import SimplePythonGraphAdapter, DictResult
from hamilton import ad_hoc_utils
lib = ad_hoc_utils.create_temporary_module(foo_union)
initial_columns = {'x': 1}
adapter = SimplePythonGraphAdapter(DictResult)
dr = Driver(initial_columns, lib, adapter=adapter)
print(dr.execute(['foo_union']))

hamilton/base.py Outdated Show resolved Hide resolved
@elijahbenizzy
Copy link
Collaborator

OK, I just realized, this is really not the right place to put this. Instead it should live here:

def custom_subclass_check(requested_type: Type[Type], param_type: Type[Type]):
. It doesn't have anything to do with a graph adapter... instead it has to do with forming the DAG. Let's move it over?

hamilton/base.py Show resolved Hide resolved
hamilton/data_quality/default_validators.py Outdated Show resolved Hide resolved
hamilton/data_quality/default_validators.py Outdated Show resolved Hide resolved
hamilton/data_quality/default_validators.py Outdated Show resolved Hide resolved
We have two places for type stuff -- some lived in graph.py.
Now they're both in type_utils. We also made a corresponding test file
called test_type_utils.py.
Note that this does supports the following:

Declared: Union[foo, bar], received foo or bar

but not:

Declared foo or bar, received Union[foo, bar]
elijahbenizzy and others added 2 commits August 15, 2022 09:11
This should fix #170. If an input function was annotated
with a union, we would barf. Now it wont.

This still means downstream nodes need to have the same
type signature -- this doesn't change any behavior there.
@skrawcz skrawcz requested a review from elijahbenizzy August 15, 2022 16:43
@skrawcz
Copy link
Collaborator Author

skrawcz commented Aug 15, 2022

LGTM - you need to approve @elijahbenizzy

@elijahbenizzy elijahbenizzy merged commit e7be742 into main Aug 15, 2022
@elijahbenizzy elijahbenizzy deleted the add_union_support branch August 15, 2022 16:57
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.

ValueError with typing.Union type signature
2 participants