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

fix(sdk): Get short name of complex input/output types to ensure we can map to appropriate de|serializer #6504

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

alexlatchford
Copy link
Contributor

Description of your changes:

The problem is that to turn an input value like "[]" into a python List type you need to use something like json.loads. To tell argparse.ArgumentParser to use json.loads kfp must be able to map the input type to this deserializer function, it does this in the get_deserializer_code_for_type_struct method (see code here). Unfortunately the type_name_to_deserializer cannot possibly contain all the List[<type>] combinations so if you have a type hint that uses List[str] instead of just List you need to run the type_annotation_utils.get_short_type_name to strip List[<type>] to just List so we can map to the appropriate serializer method. The same logic applies for Dict[<type>] to Dict too.

This is behavior that changed in Python v3.7+, KFP have been fixing various issues around this since then, see these PRs:

Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

sdk/python/kfp/components/_data_passing.py Outdated Show resolved Hide resolved
sdk/python/kfp/components/_data_passing.py Outdated Show resolved Hide resolved
sdk/python/kfp/components/_data_passing.py Outdated Show resolved Hide resolved
@alexlatchford alexlatchford force-pushed the alex/fix-type-serialization branch 2 times, most recently from 42110d9 to 3a566aa Compare September 8, 2021 15:57
@alexlatchford
Copy link
Contributor Author

Hey @chensun thanks for the review, I think I've answered all feedback. Let me know if you need anything more from my end to get this merged 😄

@alexlatchford
Copy link
Contributor Author

/retest

sdk/RELEASE.md Outdated Show resolved Hide resolved
…appropriate de|serializer

Also:
- Simplify _data_passing methods, add in type hints and docstrings.
- Remove get_deserializer_code_for_type.
@alexlatchford
Copy link
Contributor Author

Just fixed the conflicts on this one @chensun so should be ready once the tests pass!

@chensun
Copy link
Member

chensun commented Sep 10, 2021

/lgtm
/approve

Thanks!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit a0b18eb into kubeflow:master Sep 10, 2021
alexlatchford added a commit to zillow/pipelines that referenced this pull request Sep 10, 2021
…an map to appropriate de|serializer (kubeflow#6504)

Also:
- Simplify _data_passing methods, add in type hints and docstrings.
- Remove get_deserializer_code_for_type.
@alexlatchford alexlatchford deleted the alex/fix-type-serialization branch September 10, 2021 05:44
alexlatchford added a commit to zillow/pipelines that referenced this pull request Sep 16, 2021
…an map to appropriate de|serializer (kubeflow#6504)

Also:
- Simplify _data_passing methods, add in type hints and docstrings.
- Remove get_deserializer_code_for_type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants