Support default values in typing.List[dataclass] and typing.Dict[str, dataclass]

[mao3267]

Tracking issue

flyteorg/flyte#5569

Why are the changes needed?

We want to support typing.List[dataclass] and typing.Dict[dataclass] to fix the bug.

What changes were proposed in this pull request?

1. Do convert recursively in JsonParamType to handle nested dataclasses.
2. Fix errors while converting from literal back to python value with default value = None (type_engine.py)
3. Unit tests for nested dataclasses

How was this patch tested?

import typing
from dataclasses_json import dataclass_json
from flytekit import task, workflow

from dataclasses import dataclass


@dataclass_json
@dataclass
class User:
    name: str = "default"
    github_handle: str = "default"


@task
def multiple_items_task(users: typing.List[User]) -> typing.List[User]:
    return users


@workflow
def multiple_items(users: typing.List[User]) -> typing.List[User]:
    return multiple_items_task(users=users)

@task
def multiple_items_dict_task(users: typing.Dict[str, User]) -> typing.Dict[str, User]:
    return users

@workflow
def multiple_items_dict(users: typing.Dict[str, User]) -> typing.Dict[str, User]:
    return multiple_items_dict_task(users=users)

Simple Tests in CLI:

Dict[str, dataclass]

List[dataclass]

Run on remote

Unit test

A new unit test test_nested_dataclass_type is added to check if it supports nested dataclass types and default values.

Setup process

git clone https://github.com/mao3267/flytekit.git
git checkout handle-dataclass-default-value
make setup && pip install -e .

Check all the applicable boxes

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

Related PRs

None

Docs link

TODO

[codecov]

Codecov Report

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

Project coverage is 71.67%. Comparing base (54f0a46) to head (cd8877f).
Report is 1 commits behind head on master.

Files	Patch %	Lines
flytekit/interaction/click_types.py	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2603       +/-   ##
===========================================
+ Coverage   39.16%   71.67%   +32.51%     
===========================================
  Files         187      187               
  Lines       19236    19221       -15     
  Branches     4019     2785     -1234     
===========================================
+ Hits         7533    13776     +6243     
+ Misses      11470     4770     -6700     
- Partials      233      675      +442     

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

[mao3267]

cc @pingsutw Please help me review. In this PR, we have supported list[dataclass], dict[dataclass], and more nested dataclass types. However, since we haven't supported the conversion of dataclass with Flyte types members, now we also don't support other nested dataclass with Flyte Types.

[pingsutw]

Thank you! could we also add a few more tests?

1. Another dataclass in the Datum
2. List of dataclass in the Datum
3. Flyte type in the dataclass (expect to see an error?)
4. Add a nested dataclass in this test

[pingsutw]

when will python_val be None here? could we add a small test for it?

[mao3267]

When dataclass members of type typing.Dict or typing.List default to None, this can lead to exceptions when iterating the list/dict from L552-588. We do need to add a test here.

[pingsutw]

Add some comment for this function. If I pass a dataclass (not flyte type) to this function, it will return True here as well. That seems like not correct.

[Future-Outlier]

This is amazing, thank you for fixing this.

[Future-Outlier]

what is this for?

[Future-Outlier]

comment added by Vincent.

[mao3267]

This is for native list and dict types, which don't have covariant types. Therefore, we can not index the return value from get_args.

[Future-Outlier]

LGTM, thank you so much