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

[BUG] Support loading cached sublists with multiple data types #4572

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Dec 12, 2023

Tracking issue

#3901

Why are the changes needed?

Loading cached entries of nested lists with sublists of different types fails.

What changes were proposed in this pull request?

Update how LiteralTypes are generated for Literals. Do a ~post-order traversal to combine sibling collections under the same union type. Also, only include a NoneType when there aren't other Types under the same parent Type.

NOTE: deeper nesting will still fail ie:

@task(
    cache=True,
    cache_version=cache_version
)
def multi_type_depth_3() -> list[list[list[Union[str, int]]]]:
    nested_list = [
        [
            ["apple", 2, "banana", 4],
            ["orange", 3, "grape", 5]
        ],
        [
            ["str"]
        ],
    ]
    return nested_list

This occurs as the types won't collapse as they don't share the same parent. This could be solved by building types at each level as opposed to in a DFS manner - however I figure we could run into weird edge cases with that.

How was this patch tested?

  • Added Unit Testing
  • Ran workflow

Setup process

cache_version = "0.0.1"


@task(
    cache=True,
    cache_version=cache_version,
)
def single_type_depth_0() -> str:
    return "world"


@task(
    cache=True,
    cache_version=cache_version,
)
def multi_type_depth_0() -> Union[str, int]:
    return "world"


@task(
    cache=True,
    cache_version=cache_version
)
def single_type_depth_1() -> list[int]:
    return [1]


@task(
    cache=True,
    cache_version=cache_version
)
def multi_type_depth_1() -> list[Union[str, int]]:
    return [1, "foo"]


@task(
    cache=True,
    cache_version=cache_version
)
def multi_type_depth_2() -> list[list[Union[str, int]]]:
    return [["foo"], [2]]


@task(
    cache=True,
    cache_version=cache_version
)
def mixed_none_type_depth_2() -> list[list[str]]:
    return [["foo"], []]


@task(
    cache=True,
    cache_version=cache_version
)
def all_none_depth_2() -> list[list[str]]:
    return [[]]


@workflow()
def cache_test():
    single_type_depth_0()
    multi_type_depth_0()
    single_type_depth_1()
    multi_type_depth_1()
    multi_type_depth_2()
    mixed_none_type_depth_2()
    all_none_depth_2()

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

…ublists with multiple types

Signed-off-by: Paul Dittamo <[email protected]>
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 12, 2023
@pvditt pvditt marked this pull request as draft December 15, 2023 00:45
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (80a8765) 58.91% compared to head (2ed1d17) 58.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4572      +/-   ##
==========================================
+ Coverage   58.91%   58.92%   +0.01%     
==========================================
  Files         645      645              
  Lines       55351    55380      +29     
==========================================
+ Hits        32609    32632      +23     
- Misses      20159    20165       +6     
  Partials     2583     2583              
Flag Coverage Δ
unittests 58.92% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@pvditt pvditt marked this pull request as ready for review January 26, 2024 17:34
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 26, 2024
@pvditt pvditt requested a review from hamersaw January 30, 2024 17:33
hamersaw
hamersaw previously approved these changes Feb 13, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 13, 2024
@hamersaw hamersaw merged commit 1cd775a into master Feb 14, 2024
49 checks passed
@hamersaw hamersaw deleted the bug/loading-cached-sublists-with-multiple-types branch February 14, 2024 15:13
katrogan pushed a commit that referenced this pull request Feb 20, 2024
* update catalog artifact data type mapping to support loading cached sublists with multiple types

Signed-off-by: Paul Dittamo <[email protected]>

* add unit tests

Signed-off-by: Paul Dittamo <[email protected]>

* use proto equals for unit tests

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
yubofredwang pushed a commit to yubofredwang/flyte that referenced this pull request Mar 26, 2024
…org#4572)

* update catalog artifact data type mapping to support loading cached sublists with multiple types

Signed-off-by: Paul Dittamo <[email protected]>

* add unit tests

Signed-off-by: Paul Dittamo <[email protected]>

* use proto equals for unit tests

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants