feat: Support pep604 union operator #2298

ggydush · 2024-03-27T02:26:17Z

Tracking issue

Why are the changes needed?

Python 3.10 introduced the ability to write union types using | rather than importing from typing (e.g., int | str instead of typing.Union[int, str]), and I believe this should be supported. See PEP 604.

What changes were proposed in this pull request?

How was this patch tested?

Added unit tests and registered a large code base with the updated Flytekit and tested a workflow on remote. Ran a small test locally on Python 3.8 too to ensure correct error is bubbled up:

│ ❱  5 def process(a: int | None) -> int:                                                                                                                                      │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

codecov · 2024-03-27T02:30:09Z

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.46%. Comparing base (fa2aa0b) to head (fda183f).
Report is 2 commits behind head on master.

Files	Patch %	Lines
flytekit/core/type_engine.py	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2298      +/-   ##
==========================================
- Coverage   83.89%   83.46%   -0.44%     
==========================================
  Files         342      324      -18     
  Lines       25483    24716     -767     
  Branches     3725     3516     -209     
==========================================
- Hits        21380    20630     -750     
+ Misses       3472     3452      -20     
- Partials      631      634       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: ggydush <[email protected]>

pingsutw

Thank you so much! Many people are asking about this feature

flytekit/core/type_engine.py

Co-authored-by: Kevin Su <[email protected]>

Signed-off-by: ggydush <[email protected]>

tests/flytekit/unit/core/test_type_engine.py

Signed-off-by: ggydush <[email protected]>

tests/flytekit/unit/core/test_interface.py

eapolinario

This is awesome!

flytekit/core/type_engine.py

tests/flytekit/unit/core/test_interface.py

Signed-off-by: ggydush <[email protected]>

* feat: Support pep604 union operator Signed-off-by: ggydush <[email protected]> * fix: check union early Signed-off-by: ggydush <[email protected]> * refactor: Change name to 604 Signed-off-by: ggydush <[email protected]> * fix lint Signed-off-by: ggydush <[email protected]> * fix: Fix duplicated code Signed-off-by: ggydush <[email protected]> * fix: Remove code for testing Signed-off-by: ggydush <[email protected]> * test: Add simple tests Signed-off-by: ggydush <[email protected]> * Add more tests Signed-off-by: ggydush <[email protected]> * fix: Fix names Signed-off-by: ggydush <[email protected]> * fix: Lint Signed-off-by: ggydush <[email protected]> * fix: Fix again Signed-off-by: ggydush <[email protected]> * fix: Fix default Signed-off-by: ggydush <[email protected]> * test: Add test for parameter and defaults Signed-off-by: ggydush <[email protected]> * fix: Fix code coverage by ignoring Signed-off-by: ggydush <[email protected]> * refactor: Use is_union_type Signed-off-by: ggydush <[email protected]> * fix import sort Signed-off-by: ggydush <[email protected]> * fix: cleanup Signed-off-by: ggydush <[email protected]> * fix: fix Signed-off-by: ggydush <[email protected]> * refactor: Clean it up Signed-off-by: ggydush <[email protected]> * fix: Fix lint Signed-off-by: ggydush <[email protected]> * fix: Fix pydantic plugin test failure Signed-off-by: ggydush <[email protected]> * Update flytekit/core/type_engine.py Co-authored-by: Kevin Su <[email protected]> * Address comment Signed-off-by: ggydush <[email protected]> * fix: Use UnionTransformer Signed-off-by: ggydush <[email protected]> * fix: Fix lint Signed-off-by: ggydush <[email protected]> * Skip tests with | syntax on < 3.10 Signed-off-by: ggydush <[email protected]> * fix: Fix test Signed-off-by: ggydush <[email protected]> * fix: More review comments Signed-off-by: ggydush <[email protected]> * fix: Fix lint Signed-off-by: ggydush <[email protected]> --------- Signed-off-by: ggydush <[email protected]> Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Jan Fiedler <[email protected]>

ggydush added 12 commits March 27, 2024 08:22

feat: Support pep604 union operator

51aceb4

Signed-off-by: ggydush <[email protected]>

fix: check union early

854696b

Signed-off-by: ggydush <[email protected]>

refactor: Change name to 604

9ba7b7b

Signed-off-by: ggydush <[email protected]>

fix lint

cf36b8b

Signed-off-by: ggydush <[email protected]>

fix: Fix duplicated code

1fb88e8

Signed-off-by: ggydush <[email protected]>

fix: Remove code for testing

c35321d

Signed-off-by: ggydush <[email protected]>

test: Add simple tests

65cf5a7

Signed-off-by: ggydush <[email protected]>

Add more tests

bbc18a1

Signed-off-by: ggydush <[email protected]>

fix: Fix names

42920c9

Signed-off-by: ggydush <[email protected]>

fix: Lint

43ab779

Signed-off-by: ggydush <[email protected]>

fix: Fix again

c722b3a

Signed-off-by: ggydush <[email protected]>

fix: Fix default

dff23cb

Signed-off-by: ggydush <[email protected]>

ggydush force-pushed the ggydush/support-pep604 branch from f5e7ca2 to dff23cb Compare March 27, 2024 15:22

ggydush marked this pull request as ready for review March 27, 2024 15:22

ggydush requested review from wild-endeavor, kumare3, eapolinario, pingsutw and cosmicBboy as code owners March 27, 2024 15:22

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Mar 27, 2024

ggydush added 9 commits March 27, 2024 08:35

test: Add test for parameter and defaults

2dceb31

Signed-off-by: ggydush <[email protected]>

fix: Fix code coverage by ignoring

216881a

Signed-off-by: ggydush <[email protected]>

refactor: Use is_union_type

33591c7

Signed-off-by: ggydush <[email protected]>

fix import sort

fd6f140

Signed-off-by: ggydush <[email protected]>

fix: cleanup

4d60a09

Signed-off-by: ggydush <[email protected]>

fix: fix

207981f

Signed-off-by: ggydush <[email protected]>

refactor: Clean it up

de3f714

Signed-off-by: ggydush <[email protected]>

fix: Fix lint

d6a367d

Signed-off-by: ggydush <[email protected]>

fix: Fix pydantic plugin test failure

2808d62

Signed-off-by: ggydush <[email protected]>

pingsutw reviewed Mar 27, 2024

View reviewed changes

flytekit/core/type_engine.py Outdated Show resolved Hide resolved

flytekit/core/type_engine.py Outdated Show resolved Hide resolved

flytekit/core/type_engine.py Outdated Show resolved Hide resolved

ggydush and others added 4 commits March 27, 2024 13:06

Update flytekit/core/type_engine.py

d49c8fb

Co-authored-by: Kevin Su <[email protected]>

Address comment

2214c6a

Signed-off-by: ggydush <[email protected]>

fix: Use UnionTransformer

64227b7

Signed-off-by: ggydush <[email protected]>

fix: Fix lint

6914aee

Signed-off-by: ggydush <[email protected]>

ggydush requested a review from pingsutw March 27, 2024 20:16

pingsutw reviewed Mar 27, 2024

View reviewed changes

tests/flytekit/unit/core/test_type_engine.py Show resolved Hide resolved

Skip tests with | syntax on < 3.10

4b405f4

Signed-off-by: ggydush <[email protected]>

ggydush requested a review from pingsutw March 27, 2024 20:29

fix: Fix test

ee8b489

Signed-off-by: ggydush <[email protected]>

pingsutw previously approved these changes Mar 27, 2024

View reviewed changes

tests/flytekit/unit/core/test_interface.py Show resolved Hide resolved

dosubot bot added the lgtm This PR has been approved by maintainer label Mar 27, 2024

eapolinario reviewed Mar 27, 2024

View reviewed changes

flytekit/core/type_engine.py Show resolved Hide resolved

tests/flytekit/unit/core/test_interface.py Show resolved Hide resolved

fix: More review comments

a013aee

Signed-off-by: ggydush <[email protected]>

ggydush dismissed pingsutw’s stale review via a013aee March 27, 2024 23:01

ggydush requested review from eapolinario and pingsutw March 27, 2024 23:02

eapolinario previously approved these changes Mar 27, 2024

View reviewed changes

fix: Fix lint

fda183f

Signed-off-by: ggydush <[email protected]>

ggydush dismissed eapolinario’s stale review via fda183f March 27, 2024 23:06

eapolinario approved these changes Mar 27, 2024

View reviewed changes

ggydush merged commit 312c92f into master Mar 27, 2024
47 of 48 checks passed

This was referenced Apr 4, 2024

Remove typing-inspect dependency #2327

Merged

[BUG] Flytekit python unable to parse int | None style type hints on python3.11 flyteorg/flyte#4706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support pep604 union operator #2298

feat: Support pep604 union operator #2298

ggydush commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

pingsutw left a comment

eapolinario left a comment

feat: Support pep604 union operator #2298

feat: Support pep604 union operator #2298

Conversation

ggydush commented Mar 27, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Mar 27, 2024 • edited Loading

Codecov Report

pingsutw left a comment

Choose a reason for hiding this comment

eapolinario left a comment

Choose a reason for hiding this comment

ggydush commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading