Skip to content

Allow annotated FlyteFile as task input argument#1632

Merged
pingsutw merged 5 commits into
flyteorg:masterfrom
AdrianoKF:3424-annotated-flytefile-2
May 19, 2023
Merged

Allow annotated FlyteFile as task input argument#1632
pingsutw merged 5 commits into
flyteorg:masterfrom
AdrianoKF:3424-annotated-flytefile-2

Conversation

@AdrianoKF

Copy link
Copy Markdown
Contributor

TL;DR

This PR allows annotated FlyteFiles to be used as input arguments of a task, e.g., when calculating a custom hash using Annotated[FlyteFile, HashMethod(...)].

See the discussion on Slack for additional details.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Using an annotated FlyteFile type as an input to a task was previously impossible due to an exception being raised in FlyteFilePathTransformer.to_python_value.

This MR applies the fix previously used in FlyteFilePathTransformer.to_literal to permit using annotated FlyteFiles as either inputs or outputs of a task (see MR #1544), by unwrapping the origin type of the annotated type.

Tracking Issue

flyteorg/flyte#3424

Follow-up issue

NA

Using an annotated FlyteFile type as an input to a task was previously impossible due
to an exception being raised in `FlyteFilePathTransformer.to_python_value`.

This commit applies the fix previously used in `FlyteFilePathTransformer.to_literal`
to permit using annotated FlyteFiles as either inputs and outputs of a task.

Issue: flyteorg#3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM overall, thanks

Comment thread flytekit/types/file/file.py Outdated
Comment on lines +31 to +32
if get_origin(t) is Annotated:
return get_args(t)[0]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we do this all the place in the type_engine.py.

if get_origin(python_type) is Annotated:
python_type = get_args(python_type)[0]
if python_type in cls._REGISTRY:
return cls._REGISTRY[python_type]
# Step 2
if hasattr(python_type, "__origin__"):
# Handling of annotated generics, eg:
# Annotated[typing.List[int], 'foo']
if get_origin(python_type) is Annotated:
return cls.get_transformer(get_args(python_type)[0])

could you move this function to type_engine.py, and import it instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved the function there and refactored type_engine.py quite a bit in 093536f. This should also make the handling of annotated types there more uniform (and added a few tests).

Is this what you had in mind?

@codecov

codecov Bot commented May 11, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1632 (34600b0) into master (ba70f46) will increase coverage by 0.00%.
The diff coverage is 86.84%.

@@           Coverage Diff           @@
##           master    #1632   +/-   ##
=======================================
  Coverage   71.02%   71.02%           
=======================================
  Files         336      336           
  Lines       30724    30736   +12     
  Branches     5567     5566    -1     
=======================================
+ Hits        21821    21831   +10     
- Misses       8360     8361    +1     
- Partials      543      544    +1     
Impacted Files Coverage Δ
flytekit/types/file/file.py 54.54% <66.66%> (+0.32%) ⬆️
flytekit/core/type_engine.py 61.15% <80.95%> (-0.14%) ⬇️
tests/flytekit/unit/core/test_flyte_file.py 88.81% <100.00%> (+0.17%) ⬆️
tests/flytekit/unit/core/test_type_engine.py 98.66% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

Issue: flyteorg#3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, LGTM. would you resolve branch conflict?

…ytefile-2

Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

# Conflicts:
#	flytekit/core/type_engine.py
@AdrianoKF

Copy link
Copy Markdown
Contributor Author

Thanks, LGTM. would you resolve branch conflict?

Sorry, was sick for the past days, just merged upstream master and resolved the conflicts.

pingsutw
pingsutw previously approved these changes May 17, 2023

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AdrianoKF hope you feel better.

@AdrianoKF

Copy link
Copy Markdown
Contributor Author

Merged master again and fixed the test failures on Python 3.8 in 34600b0.

The remaining failing test on 3.11/Windows seems to be unrelated to my changes and I'm not familiar enough with that part of the code base to suggest a fix - anything I should do about that, @pingsutw?

@pingsutw

Copy link
Copy Markdown
Member

@AdrianoKF never mind. That's flaky test.

@pingsutw pingsutw merged commit 06fffc7 into flyteorg:master May 19, 2023
@AdrianoKF AdrianoKF deleted the 3424-annotated-flytefile-2 branch May 23, 2023 06:13
ArthurBook pushed a commit to ArthurBook/flytekit that referenced this pull request May 26, 2023
* fix: Allow annotated FlyteFile as task input argument

Using an annotated FlyteFile type as an input to a task was previously impossible due
to an exception being raised in `FlyteFilePathTransformer.to_python_value`.

This commit applies the fix previously used in `FlyteFilePathTransformer.to_literal`
to permit using annotated FlyteFiles as either inputs and outputs of a task.

Issue: flyteorg#3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* refactor: Unified handling of annotated types in type engine

Issue: flyteorg#3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* fix: Use py3.8-compatible types in type engine tests

Issue: flyteorg#3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

---------

Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>
Signed-off-by: Arthur <atte.book@gmail.com>
ArthurBook added a commit to ArthurBook/flytekit that referenced this pull request Jun 12, 2023
eapolinario pushed a commit that referenced this pull request Jul 10, 2023
* fix: Allow annotated FlyteFile as task input argument

Using an annotated FlyteFile type as an input to a task was previously impossible due
to an exception being raised in `FlyteFilePathTransformer.to_python_value`.

This commit applies the fix previously used in `FlyteFilePathTransformer.to_literal`
to permit using annotated FlyteFiles as either inputs and outputs of a task.

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* refactor: Unified handling of annotated types in type engine

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* fix: Use py3.8-compatible types in type engine tests

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

---------

Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>
eapolinario added a commit that referenced this pull request Jul 12, 2023
* Multi arch imageSpec (#1630)

Multi arch imageSpec (#1630)

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Add executor_path and applications_path to spark config (#1634)

* Add executor_path and applications_path to spark config

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Add support for env vars to pyflyte run (#1617)

* Add support for env vars to pyflyte run

Signed-off-by: Kevin Su <pingsutw@apache.org>

* bump idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update doc

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Fetch task executions in dynamic  (#1636)

* fetch task executions in dynamic

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Added metrics command to pyflyte (#1513)

Signed-off-by: Daniel Rammer <daniel@union.ai>

* Add http_proxy to client & Fix deviceflow (#1611)

* Add http_proxy to client & Fix deviceflow

RB=3890720

Signed-off-by: byhsu <byhsu@linkedin.com>

* nit

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint!

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Improve variable names (#1642)

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Address resolution (#1567)

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* pyflyte run supports pickle (#1646)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Wait for the pod plugin instead of flytekit (#1647)

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Beautify deviceflow prompt (#1625)

* Beautify deviceflow prompt

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint!

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Improve flytekit register (#1643)

* Fix pyflyte register

Signed-off-by: byhsu <byhsu@linkedin.com>

* revert

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Pass verify flag to all authenticators (#1641)

Signed-off-by: byhsu <byhsu@linkedin.com>

* Allow annotated FlyteFile as task input argument (#1632)

* fix: Allow annotated FlyteFile as task input argument

Using an annotated FlyteFile type as an input to a task was previously impossible due
to an exception being raised in `FlyteFilePathTransformer.to_python_value`.

This commit applies the fix previously used in `FlyteFilePathTransformer.to_literal`
to permit using annotated FlyteFiles as either inputs and outputs of a task.

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* refactor: Unified handling of annotated types in type engine

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* fix: Use py3.8-compatible types in type engine tests

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

---------

Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* Use logger instead of print statement in sqlalchemy plugin (#1651)

* use logging info instead of print

Signed-off-by: wirthual <wirthra@gmail.com>

* isorted files

Signed-off-by: wirthual <wirthra@gmail.com>

* import root logger from flytekit

Signed-off-by: wirthual <wirthra@gmail.com>

---------

Signed-off-by: wirthual <wirthra@gmail.com>

* Map over notebook task (#1650)

* map over notebook

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* add a flag

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Support single literals in tiny url (#1654)

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* Add support overriding image (#1652)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix ability to pass None to task with Optional kwarg, add test (#1657)

Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Co-authored-by: Fabio Grätz <fabiogratz@googlemail.com>

* Regenerate plugins requirements

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Regenerate plugins requirements and linting

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Regenerate whylogs requirements

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: byhsu <byhsu@linkedin.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>
Signed-off-by: wirthual <wirthra@gmail.com>
Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Dan Rammer <daniel@union.ai>
Co-authored-by: ByronHsu <byronhsu1230@gmail.com>
Co-authored-by: byhsu <byhsu@linkedin.com>
Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Adrian Rumpold <a.rumpold@gmail.com>
Co-authored-by: wirthual <wirthra@gmail.com>
Co-authored-by: Fabio M. Graetz, Ph.D <fabiograetz@googlemail.com>
Co-authored-by: Fabio Grätz <fabiogratz@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants