Add ability to clear cache for specific function arguments by passing args to <cached_func>.clear()

[OscarSaharoy]

Describe your changes

Hi, I really love streamlit and have been having fun developing an LLM app with it. I wanted to cache the LLM responses, unless a user said it was a bad response. So I decided to implement this. Thanks for taking a look!

I know it might not be perfect, if you give me some pointers I am happy to make a lot of changes as needed :) I think the implementation with passing the key though two layers of methods could be better so I'll think how I can improve that.

This keeps the exisiting interface to CachedFunc.clear() where passing no arguments clears all the cache, but also adds the option that if args/kwargs are passed, the value key will be calculated and the cache storage for that key will be invalidated. This only applies to DataCache for now - I know the error raised could be better so please let me know what type of exception you would prefer it to throw.

GitHub Issue Link

8153

Testing Plan

• Explanation of why no additional tests are needed tests are always needed
• Unit Tests (JS and/or Python) python unit test only
• E2E Tests not sure
• Any manual testing needed? since its very explicit I think unit tests are better

---

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

[OscarSaharoy]

Hi @kmcgrady @willhuang1997 how does this look to you guys :D

[vdonato]

Thanks for the contribution @OscarSaharoy, and sorry for the delay in getting around to reviewing this! This looks like a reasonable addition to me, but I'll have to defer to our product team (CC @jrieke, @sfc-gh-jcarroll) on whether we want to add this to our caching API.

Additionally, I think that, if we want to implement this for the @st.cache_data decorator, we'll likely also want to have it working for @st.cache_resource for consistency.

[OscarSaharoy]

Thanks for the contribution @OscarSaharoy, and sorry for the delay in getting around to reviewing this! This looks like a reasonable addition to me, but I'll have to defer to our product team (CC @jrieke, @sfc-gh-jcarroll) on whether we want to add this to our caching API.

Additionally, I think that, if we want to implement this for the @st.cache_data decorator, we'll likely also want to have it working for @st.cache_resource for consistency.

Hi @vdonato thanks for taking a look!! I have added the corresponding behaviour for @st.cache_resource and improved the testing and documentation. Let me know if anything could be improved :)

[LukasMasuch]

Overall LGTM 👍 But I think it needs another iteration on what to do if there isn't a cache entry for a given set of arguments.

[LukasMasuch]

nit: we might want to consider catching the exception if the key cannot be found in the cache, and rethrow it with a StreamlitAPIException that has a better description of what is going wrong. I think the key shown in the other exception might not be super descriptive. And maybe an additional test for this case (there isn't a cached entry for the arguments) might make sense.

[LukasMasuch]

I thought a bit more about this: maybe we shouldn't even throw an exception here if there isn't an item associated. We just except and pass in that case. If there isn't an item in the cache for the arguments, the user shouldn't even care since there is nothing to delete.

Still needs changes

[sfc-gh-jcarroll]

Yep from a product side I think this would be great to support and the API seems good, sounds like we just need to make some updates on the implementation before we can merge. Thanks for the contribution!!

[OscarSaharoy]

Hi @LukasMasuch @sfc-gh-jcarroll thanks so much for taking a look and helping me get this in :) And I may be wrong but I think the behaviour described is already in place - in the tests I have added, the first calls that are like foo.clear(2) just pass silently. I think this is implemented nicely by the streamlit storage classes, let me know if something else is needed though.

[sfc-gh-pkommini]

Hi Team,

Great job arriving at a fix for this problem. Is there an ETA on when this might make it to a release? We are releasing an internal Streamlit app in April and we keep getting intermittent cache access errors because one user's actions clear cache for everyone and we seem to be ending up with KeyError due to race conditions where the cache is accessed between one user's actions clearing the cache and a cache refresh.

[LukasMasuch]

@OscarSaharoy oh, you are right :) The strange part is that it makes sense for cached_data since it comes down to this .pop() call here:

streamlit/lib/streamlit/runtime/caching/storage/in_memory_cache_storage_wrapper.py

Line 145 in 2d910fd

self._mem_cache.pop(key, None)

But just from looking at cache_resource it doesn't seem to be obvious where the key error might be prevented. Are you sure that the cache_resource test is also running fine?

Also, it needs another merge in or rebase to develop, otherwise the python tests will not run.

[LukasMasuch]

@sfc-gh-pkommini

Is there an ETA on when this might make it to a release?

Next release is on the 2nd Mai. It seems likely to have this in for 1.34 if there aren't any big disagreements with this feature.

intermittent cache access errors because one user's actions clear cache for everyone and we seem to be ending up with KeyError due to race conditions where the cache is accessed between one user's actions clearing the cache and a cache refresh.

This sounds like something we should fix, but probably in a dedicated PR. Would be awesome if you could open a new issue for this.

[OscarSaharoy]

@LukasMasuch Ah yes it may actually be this line which handles it for the cache_resource case:

streamlit/lib/streamlit/runtime/caching/cache_resource_api.py

Line 559 in 2aabcf8

elif key in self._mem_cache:

And I have rebased to develop and the tests are definitely running ok for me locally :)

[LukasMasuch]

@OscarSaharoy oh, of course... my bad 😬 Tests also seem to run fine on CI 👍 LGTM!

[OscarSaharoy]

@LukasMasuch thanks so much for your help with this and excited to use the feature!!! I don't have permission to click merge so I will leave it to one of you :)

[jrieke]

@sfc-gh-jcarroll I'll leave this up to you to give an LGTM from product side since it's in your area. I'm fine with the API proposed here.

Just want to note one thing: if we go with this API pattern of allowing args/kwargs, it might be more complicated to add additional parameters in the future to the clear function. Even though I can't really think of any parameters, so maybe that's a non-issue!

[LukasMasuch]

Good news, we will merge this in for the 1.34 release 👍 Just one more comment that needs to be addressed.

[LukasMasuch]

this needs to be changed to:

Suggested change

	>>> fetch_and_clean_data.clear(_sessionmaker, "https://streamlit.io/")
	>>> get_database_session.clear(_sessionmaker, "https://streamlit.io/")

[LukasMasuch]

cc @sfc-gh-dmatthews to double-check the docstring text. But docstring changes will probably go into a separate PR or?

[sfc-gh-dmatthews]

I can go either way. If you want to get this merged right away, feel free to. Otherwise, I can try to take a look at this tomorrow or Monday. (Just going through tasks after coming back from a few days off....)

[LukasMasuch]

I think it is easier to merge this right now since the code is ready (and pushing changes into a forked PR is a bit more cumbersome) and do the docs iteration in a dedicated PR. There are probably anyway a couple of docstring changes needed before the release. Btw. this wrong code usage within the docstring example would also need to be fixed: #8297 (comment)

[LukasMasuch]

@sfc-gh-dmatthews this one as well