flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

Fix Bug: with_overrides for cache doesn't work

Open hebiao064 opened this issue 1 year ago • 2 comments

Tracking issue

Closes flyteorg/flyte#6079

Why are the changes needed?

I tried the with_overrides to override cache configuration for a local flyte @task, but it didn't work as expected.

Code:

@task(
    cache=False,
    cache_version="v1",
    cache_serialize=False,
)
def to_be_overridden_task(a: str) -> str:
    return f"*~*~*~{a}*~*~*~"

def to_be_overridden_task_wrapper(a: str, cache: bool = False, cache_version: str = "", cache_serialize: bool = False) -> str:
    return to_be_overridden_task(a=a).with_overrides(
        cache=cache,
        cache_version=cache_version,
        cache_serialize=cache_serialize
    )

@workflow
def my_overridden_wf(a: str):
    return to_be_overridden_task_wrapper(a=a, cache=True, cache_version="foo", cache_serialize=True)

After pyflyte run, I found the cache is still False and cache_version still ""

What changes were proposed in this pull request?

  1. When cache overrides specified, we also override the flyteentity's metadata
  2. Reorder the serialization order to serialize workflow first hence with_overrides will be called first, otherwise the task will be always registered as original task configuration without overrides.

How was this patch tested?

make test with a new test case

Also tested in our company's fork E2E with many workflows submission.

Setup process

Screenshots

Before: Screenshot 2024-12-03 at 5 57 29 PM After: Screenshot 2024-12-03 at 5 57 22 PM

Check all the applicable boxes

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

Related PRs

https://github.com/flyteorg/flytekit/pull/2154

Docs link

hebiao064 avatar Dec 04 '24 07:12 hebiao064