[generate] Stop setting the static cache as an attribute to save memory - #47731
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
vasqu
left a comment
There was a problem hiding this comment.
One question im a bit hesitant about because I dont follow the implications here 100% but other than that it's a nice optim 馃憖
| Returns the resulting cache object. | ||
| Create a static cache for `generate`. To avoid recompilation, the new cache will use the maximum between the current | ||
| `max_cache_len` and the potential previous value of `max_cache_len`, if there was some previous `generate` calls with | ||
| static cache. |
There was a problem hiding this comment.
Do we have any test that checks recompilations in regards to that?
There was a problem hiding this comment.
We don't, but I confirmed it locally at least - will try to add one, but can be tricky to catch recompiles automatically
| "max_cache_len": effective_length, | ||
| "offloading": offload_cache, | ||
| } | ||
| cache = StaticCache(**self_attention_cache_kwargs) |
There was a problem hiding this comment.
It does mean that we recreate a cache in any case, no? Would there be any issue when we use multiple iterations with a chunked prefill? Just a thought, unsure
There was a problem hiding this comment.
Not sure I understand what you mean? Basically, now we create a new StaticCache every generate call vs keeping one attached to the model. To be honest, I have no idea why it was ever attached to the model... compile caches the graphs based on shapes and ops, not objects addresses, so we can just recreate the object
CI recapDashboard: View test results in Grafana |
vasqu
left a comment
There was a problem hiding this comment.
Thanks, I meant that in some cases before we did not recreate the cache itself (nothing to do with the attached cache) but tbh it's weird to have the attached version so rather get that out
What does this PR do?
As per the title. Keeping the cache as an attribute
self._cacheinsidegenerateis a very bad idea in general, because it will keep a big memory footprint without the user noticing, which can be very surprising and unwanted if we want to make 1 call to generate with static cache, and then use the model for something else.It was previously done to avoid recompilation if we were making several calls to
generatewith several lengths, but simply keeping the oldmax_cache_lenas attribute and re-using that later is enough to avoid recompilation.The following snippet illustrates the issue:
Before this PR, it prints:
After this PR:
which makes a HUGE difference as can be easily seen.
Note that this does not incur any additional recompilation, since we still check the max size of the cache. This can be checked with the following snippet for example: