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

Add assert_type in dataclass transformer #1149

Merged
merged 9 commits into from
Sep 8, 2022
Merged

Add assert_type in dataclass transformer #1149

merged 9 commits into from
Sep 8, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Sep 6, 2022

Signed-off-by: Kevin Su [email protected]

TL;DR

slack thread

Add assert_type()in dataclass transformer, so we don't need to specify type_hints when using flyte remote.

We got an error when using dataclass as input in the flyte remote without passing type_hints, because guess_python_type is not equal to the user-defined dataclass type

@dataclass_json
@dataclass
class Foo(object):
    a: int = 0

@task
def t1(a: Foo):
    ...

In this example, we use guess_python_type (FooSchema) as our expected_type by default if using flyte remote to execute the above task.
However, FooSchema is created by flytekit and it's not equal to the user-defined dataclass (Foo), so assert_type raises an error.

We can override assert_type in the dataclass transformer, and check that the type of each attribute is the same as the guess_type by iterating all attributes in dataclass.

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

The code to reproduce

from dataclasses import dataclass
from typing import Optional
from dataclasses_json import dataclass_json
from flytekit import task, workflow, dynamic


@dataclass_json
@dataclass
class Args(object):
    max_iter: int = 0
    init_eval: float = 0
    max_eval: str = 0


@dataclass_json
@dataclass
class Schema(object):
    kwargs: Optional[Args] = None


@task
def t11(schema: Schema):
    print(schema)


@workflow
def wf3(schema: Schema = Schema(Args(1, 2.0, "hello"))):
    t11(schema=schema)


if __name__ == "__main__":
    wf3(schema=Schema(Args(1, 2.0, "hello")))
from flytekit.remote import FlyteRemote
from flytekit.configuration import Config
from example_dataclass import Args, Schema

remote = FlyteRemote(
    config=Config.auto("/Users/kevin/.flyte/config-remote.yaml"),
    default_project="flytesnacks",
    default_domain="development",
)

flyte_workflow = remote.fetch_workflow(
    name="example_dataclass.wf3", project="flytesnacks", domain="development"
)

execution = remote.execute(
    flyte_workflow, inputs={"schema": Schema(Args(1, 2, "hello"))}, wait=False,
)

Tracking Issue

flyteorg/flyte#2844

Follow-up issue

NA

@pingsutw pingsutw marked this pull request as draft September 6, 2022 09:58
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1149 (3254a53) into master (22580d8) will increase coverage by 0.04%.
The diff coverage is 90.19%.

@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage   68.38%   68.43%   +0.04%     
==========================================
  Files         288      288              
  Lines       25963    26014      +51     
  Branches     2899     2910      +11     
==========================================
+ Hits        17756    17802      +46     
- Misses       7728     7733       +5     
  Partials      479      479              
Impacted Files Coverage Δ
flytekit/core/type_engine.py 59.24% <79.16%> (+0.60%) ⬆️
tests/flytekit/unit/core/test_type_engine.py 98.39% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw marked this pull request as ready for review September 6, 2022 18:57
wild-endeavor
wild-endeavor previously approved these changes Sep 7, 2022
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

is there really nothing in dataclass json or json schema that will do this? we have to write our own code?

if dataclasses.is_dataclass(val):
self.assert_type(expected_type, val)
elif original_type != expected_type:
raise TypeTransformerFailedError(f"Type of Val '{original_type}' is not an instance of {expected_type}")
Copy link
Contributor

Choose a reason for hiding this comment

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

is it easy to add a test for this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it.
I've tried to find a kind of function that can compare the attribute in dataclass, but I didn't find that.

Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit eced84b into master Sep 8, 2022
eapolinario pushed a commit that referenced this pull request Sep 15, 2022
* Add assert_type in dataclassTransformer

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

* fix lint

Signed-off-by: Kevin Su <[email protected]>

* Add one more test

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>
eapolinario added a commit that referenced this pull request Sep 16, 2022
* Add deck to papermill plugin task (#1111)

Signed-off-by: Calvin Leather <[email protected]>

* Run compilation even in local execution for dynamic tasks to early detect errors (#1121)

Signed-off-by: Yee Hing Tong <[email protected]>

* Set to pyflyte run blob object remote when dealing with remote files (#1128)

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>

* Override voidPromise resource (#1127)

* override void promise resource

Signed-off-by: Kevin Su <[email protected]>

* override void promise resource

Signed-off-by: Kevin Su <[email protected]>

* Fix how ShellTask retrieves the Pod class name (#1132)

* Fix how ShellTask retrieves the Pod class name

Signed-off-by: Matheus Moreno <[email protected]>

* Set Pod class name as a constant

Signed-off-by: Matheus Moreno <[email protected]>

* Revert last commit

Signed-off-by: Matheus Moreno <[email protected]>

* Execute automatic linting

Signed-off-by: Matheus Moreno <[email protected]>

Signed-off-by: Matheus Moreno <[email protected]>

* Add restriction for pandas to be >=1.2 for fsspec plugin (#1136)

Signed-off-by: Yee Hing Tong <[email protected]>

* Use joblib hashing to generate cache key to ensure repeatability (#1126)

* cherry pick 97b454b

Signed-off-by: Yee Hing Tong <[email protected]>

* requirements

Signed-off-by: Yee Hing Tong <[email protected]>

* Fix usage of save in ProtoJoblibHasher

Signed-off-by: Eduardo Apolinario <[email protected]>

* Regenerate requirements using python 3.7

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add test_stable_cache_key

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Allow None protocol to mean all data persistence supported storage options in Structured Dataset (#1134)

Signed-off-by: Yee Hing Tong <[email protected]>

* handle ImportError and OSError in extras.pytorch (#1141)

* handle ImportError and OSError in extras.pytorch

Signed-off-by: Niels Bantilan <[email protected]>

* isolate exception to torch import

Signed-off-by: Niels Bantilan <[email protected]>

Signed-off-by: Niels Bantilan <[email protected]>

* Register dataframe renderers in structured dataset (#1140)

* Register dataframe renderers in structured dataset

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* pyflyte run imperative workflows (#1131)

Signed-off-by: Kevin Su <[email protected]>

* Using sidecar handler to run Papermill task (#1143)

* remove nb prefix

Signed-off-by: Kevin Su <[email protected]>

* add tests

Signed-off-by: Kevin Su <[email protected]>

* Update requirements.in

Signed-off-by: Kevin Su <[email protected]>

* remove _

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Properly raise error in NumpyArrayTransformer (#1146)

Signed-off-by: Rahul Mehta <[email protected]>

Signed-off-by: Rahul Mehta <[email protected]>

* Add assert_type in dataclass transformer (#1149)

* Add assert_type in dataclassTransformer

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

* fix lint

Signed-off-by: Kevin Su <[email protected]>

* Add one more test

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Pickle in Union Type (#1147)

* Pickel in Union type

Signed-off-by: Kevin Su <[email protected]>

* Pickel in Union type

Signed-off-by: Kevin Su <[email protected]>

* wip

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* Address comment

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Bump max docker version to 7.0.0 (#1138)

Signed-off-by: Rahul Mehta <[email protected]>

Signed-off-by: Rahul Mehta <[email protected]>

* Set flytekit<2.0 in plugins (#1152)

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Add literal type to union literal (#1144)

* Add literal type to union literal

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* Add tests

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Fix the type of optional[int] in nested dataclass (#1148)

* Fix the type of optional[int] in nested dataclass

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* update comments

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Added symlink dereferencing in fast packaging and tests (#1151)

* Added symlink dereferencing and tests

Signed-off-by: Vanshika Chowdhary <[email protected]>

* Added flag to register as well

Signed-off-by: Vanshika Chowdhary <[email protected]>

* More flag propagation

Signed-off-by: Vanshika Chowdhary <[email protected]>

Signed-off-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>

* Strip newline from client secret (#1163)

* Strip newline from client secret

* Add logging and rework the secret file comparison to work on windows

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Fix the type of optional[int] in dataclass (#1135)

Signed-off-by: Kevin Su <[email protected]>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1154)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-OAUTHLIB-3021142
- https://snyk.io/vuln/SNYK-PYTHON-PYSPARK-3021131

Signed-off-by: Eduardo Apolinario <[email protected]>

* Using sidecar handler to run Papermill task (#1143)

* remove nb prefix

Signed-off-by: Kevin Su <[email protected]>

* add tests

Signed-off-by: Kevin Su <[email protected]>

* Update requirements.in

Signed-off-by: Kevin Su <[email protected]>

* remove _

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1145)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-COOKIECUTTER-2414281

* Bump pyspark from 3.2.1 to 3.2.2 in /plugins/flytekit-papermill (#1130)

Bumps [pyspark](https://github.com/apache/spark) from 3.2.1 to 3.2.2.
- [Release notes](https://github.com/apache/spark/releases)
- [Commits](apache/spark@v3.2.1...v3.2.2)

---
updated-dependencies:
- dependency-name: pyspark
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1154)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-OAUTHLIB-3021142
- https://snyk.io/vuln/SNYK-PYTHON-PYSPARK-3021131

Signed-off-by: Calvin Leather <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Matheus Moreno <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Rahul Mehta <[email protected]>
Signed-off-by: Vanshika Chowdhary <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Calvin Leather <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Matheus Moreno <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Niels Bantilan <[email protected]>
Co-authored-by: Rahul Mehta <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Snyk bot <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.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