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

Option to disable local caching #2106

Merged

Conversation

bstadlbauer
Copy link
Member

@bstadlbauer bstadlbauer commented Jan 14, 2024

Tracking issue

Closes flyteorg/flyte#4395

Why are the changes needed?

While local caching is useful in some cases, it has often led to unexpected errors for users not being aware. Especially in large codebases this can lead to unexpected test failures, etc.

What changes were proposed in this pull request?

This PR introduces a new config section local with one value cachen_enabled (which defaults to true):

[local]
cache_enabled = True

How was this patch tested?

Added a test which confirms that setting FLYTE_LOCAL_CACHE_ENABLED=false prevents the local use of the cache.

Check all the applicable boxes

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

Docs link

Changed in flyteorg/flytesnacks#1425

https://flyte--1425.org.readthedocs.build/projects/cookbook/en/1425/auto_examples/development_lifecycle/task_cache.html#how-does-local-caching-work

Signed-off-by: Bernhard Stadlbauer <b.stadlbauer@gmx.net>
Signed-off-by: Bernhard Stadlbauer <b.stadlbauer@gmx.net>
Signed-off-by: Bernhard Stadlbauer <b.stadlbauer@gmx.net>
Copy link

codecov bot commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (892b474) 85.77% compared to head (836398a) 85.78%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2106   +/-   ##
=======================================
  Coverage   85.77%   85.78%           
=======================================
  Files         313      313           
  Lines       23500    23513   +13     
  Branches     3512     3514    +2     
=======================================
+ Hits        20158    20171   +13     
  Misses       2734     2734           
  Partials      608      608           

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

Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

Ideally we should also make pyflyte run --(option for invalidating cache) work in local mode

@@ -66,6 +66,11 @@ class AZURE(object):
CLIENT_SECRET = ConfigEntry(LegacyConfigEntry(SECTION, "client_secret"))


class LOCAL(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Local

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in 836398a

Signed-off-by: Bernhard Stadlbauer <b.stadlbauer@gmx.net>
@bstadlbauer
Copy link
Member Author

@kumare3

Ideally we should also make pyflyte run --(option for invalidating cache) work in local mode

That sounds reasonable. I've poked around a little and found pyflyte run --overwrite-cache. Given that that's a slightly different usecase (cache overwrite/invalidation vs. cache disabling) would you want me to open a different PR? Also do you think it would ok to do a full LocalTaskCache.clear()? Or should we invalidate per-task (which might need another config or some other plumbing)

@kumare3
Copy link
Contributor

kumare3 commented Jan 14, 2024

Aah it should be invalidate per cache key only

@bstadlbauer
Copy link
Member Author

@kumare3 I've opened flyteorg/flyte#4727 to track the pyflyte run --overwrite-cache behavior (as that's likely more work). Could we still merge this PR before?

@fg91
Copy link
Member

fg91 commented Jan 18, 2024

(Dropping a message to be notified once this is merged <3)

@bstadlbauer bstadlbauer merged commit 475eecd into flyteorg:master Jan 19, 2024
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants