Fix Bug: with_overrides for cache doesn't work

[hebiao064]

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:

After:

Check all the applicable boxes

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

Related PRs

#2154

Docs link

[welcome]

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

• Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
• Sign off your commits (Reference: DCO Guide).

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 42.28%. Comparing base (55d4d2d) to head (698d62b).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
flytekit/core/node.py	0.00%	8 Missing ⚠️
flytekit/tools/serialize_helpers.py	0.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (55d4d2d) and HEAD (698d62b). Click for more details.

HEAD has 11 uploads less than BASE

Flag	BASE (55d4d2d)	HEAD (698d62b)
	18	7

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2975       +/-   ##
===========================================
- Coverage   79.62%   42.28%   -37.34%     
===========================================
  Files         200      226       +26     
  Lines       20997    22685     +1688     
  Branches     2705     2710        +5     
===========================================
- Hits        16719     9593     -7126     
- Misses       3511    12876     +9365     
+ Partials      767      216      -551     

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

[pingsutw]

We should not override the run_entity.

[pingsutw]

Suppose you have two tasks in a workflow, like below. The cache version of the second task will be overridden as well.

@workflow
def wf():
  t1().with_override(cache_version="v2")
  t1()

[pingsutw]

You can pass the node info to local_execute, and use the cache version in the node info to retrieve the data from the cache.

flytekit/flytekit/core/base_task.py

Line 306 in 935be5c

if self.metadata.cache and local_config.cache_enabled:

[hebiao064]

@pingsutw Could I know how to do this?

"You can pass the node info to local_execute`

[hebiao064]

I think the problem is:

In node.py, self._metadata._cacheable won't be affecting the actual Task entity.

That's why we want to modify the Task's entity's metadata.

But I acknowledge the issue that the two tasks example woulld be problematic. I am not sure how local_config can help with it.