🚨 [cache] Cropping can only be done with negative values by Cyrilvallez · Pull Request #47720 · huggingface/transformers

Cyrilvallez · 2026-08-03T03:13:09Z

What does this PR do?

As per the title.
Until before #47347 and #47447, the cache was basically crop'ed to an absolute value, which represented the final expected size of the cache. However, for a lot of caches such as sliding window or linear attention, we evict tokens very soon and only keep necessary states in the cache. This means that cropping to an absolute value does not make sense, as we don't hold all the states. So this PR officially deprecates the old way for the new way, which is cropping the number of tokens we want to remove from the cache (this was already possible before by using a negative value).
Basically, we move from "the integer represents the total number of tokens I want to keep" to "the integer represents the number of tokens I want to remove". This means that it is slightly breaking for the limit value, which is crop(0) as 0 is neither positive nor negative of course. Technically it was not really making any sense before to crop(0), but it used to mean "I want to delete all my cache", and it now means "I want to do nothing".
I put the alarm 🚨 in the title as it's breaking in this way, but note that it was really a limit value, and that crop(0) was literally never called.
Also, note that since #47447, I already updated all the call sites to use negative values (as it is the only way to roll back correctly for ALL caches), so this simply officializes the change for standard full attention cache, so that we have a unique and unified API for crop

Cyrilvallez · 2026-08-03T03:24:37Z

-            if number_of_tokens_to_crop > 0:
-                outputs.past_key_values.crop(-number_of_tokens_to_crop)
+            outputs.past_key_values.crop(-number_of_tokens_to_crop)


Even if it's 0, it's important to call crop still, as for sliding window and linear attention, this is what will force the cache to shrink back to the sliding_window or conv_kernel size, i.e. drop the states it does not need for next forward (not strictly needed in theory, but will be more efficient and will avoid shapes mix-up in the mask creation API)

Gotcha but it kinda bites with the PR description where 0 was seen as no op no?

HuggingFaceDocBuilderDev · 2026-08-03T03:27:00Z

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.

github-actions · 2026-08-03T04:38:31Z

[For maintainers] Suggested jobs to run (before merge)

run-slow: minimax, minimax_m3_vl

github-actions · 2026-08-03T04:51:04Z

CI recap

Dashboard: View test results in Grafana
Latest run: 30781763672:1
Result: success | Jobs: 16 | Tests: 178,129 | Failures: 3 | Duration: 4h 11m

Madhav-000-s · 2026-08-03T11:09:40Z

+        effective = tokens_to_remove if tokens_to_remove > 0 else self.indexer_keys.shape[1] - abs(tokens_to_remove)
        if self.indexer_keys.shape[1] > effective:
            self.indexer_keys = self.indexer_keys[:, :effective, :]


Three crop overrides still compute the old absolute index, so they don't inherit the clamping that the new -abs(tokens_to_remove) slice gives the parent:

DynamicIndexedLayer.crop in cache_utils.py

MiniMaxM3VLSparseCacheLayer.crop in modular_minimax_m3_vl.py (+ generated modeling_)

effective = tokens_to_remove if tokens_to_remove > 0 else self.indexer_keys.shape[1] - abs(tokens_to_remove)

On a 3-token cache, crop(-5) empties keys/values but leaves the index tensor at length 1. Before this PR both kept 1 — consistently wrong, but consistent. After it they diverge, which is the harder state to debug.
I couldn't find a call site that actually passes |n| > seq_len today, so this is robustness rather than a live bug — but the parent now clamps and the subclasses don't, and mirroring the parent is cheap:

if tokens_to_remove > 0: if tokens_to_remove >= self.indexer_keys.shape[1]: return tokens_to_remove = self.indexer_keys.shape[1] - tokens_to_remove if tokens_to_remove == 0: return self.indexer_keys = self.indexer_keys[:, : -abs(tokens_to_remove), :]

No behaviour change except the overflow case — checked over n ∈ {5, 4, 3, 2, 1, 0, -1, -2, -3, -5}, output is identical to the current code everywhere except n = -5, where the index tensor goes to 0 instead of 1 and so matches get_seq_length().

I feel like we need some cache tests in general so we don't have regressions across the cache classes in regards to this

vasqu

Some quick initial comments

vasqu · 2026-08-03T14:23:33Z

-            if number_of_tokens_to_crop > 0:
-                outputs.past_key_values.crop(-number_of_tokens_to_crop)
+            outputs.past_key_values.crop(-number_of_tokens_to_crop)


Gotcha but it kinda bites with the PR description where 0 was seen as no op no?

vasqu · 2026-08-03T14:23:57Z

                self.layers[layer_idx].batch_select_indices(indices)

-    def crop(self, max_length: int):
+    def crop(self, tokens_to_remove: int) -> None:


deprecate kwarg instead?

vasqu · 2026-08-03T14:24:46Z

            self.idx_keys = self.idx_keys[indices, ...]

-    def crop(self, max_length: int) -> None:
+    def crop(self, tokens_to_remove: int) -> None:


last mention but would definitely use deprecate kwargs

vasqu · 2026-08-03T14:25:55Z

-        if self.idx_keys is not None and self.idx_keys.shape[-2] > max_length:
-            self.idx_keys = self.idx_keys[..., :max_length, :]
-        super().crop(max_length)
+        effective_length = tokens_to_remove if tokens_to_remove > 0 else self.get_seq_length() - abs(tokens_to_remove)


are we not triggering deprecation by default here iff tokens to remove > 0

vasqu · 2026-08-03T14:27:19Z

-            self.keys = self.keys[:, :, -self.sliding_window + 1 - tokens_to_remove : -tokens_to_remove, :]
-            self.values = self.values[:, :, -self.sliding_window + 1 - tokens_to_remove : -tokens_to_remove, :]
-            self.cumulative_length = self.cumulative_length - tokens_to_remove
+            # In this case, simply restrict the size back to sliding window without cropping


yea we should really make sure this is properly documented somewhere in normal docs (not only code) because this behavior seems new no or at least not obv/intuitive)

vasqu · 2026-08-03T14:28:45Z

+        effective = tokens_to_remove if tokens_to_remove > 0 else self.indexer_keys.shape[1] - abs(tokens_to_remove)
        if self.indexer_keys.shape[1] > effective:
            self.indexer_keys = self.indexer_keys[:, :effective, :]


I feel like we need some cache tests in general so we don't have regressions across the cache classes in regards to this

Cyrilvallez added 2 commits August 3, 2026 12:03

deprecate

eadfa98

fix

fa56e01

Cyrilvallez commented Aug 3, 2026

View reviewed changes

improve comment

4b060ad

Cyrilvallez mentioned this pull request Aug 3, 2026

DynamicCache.crop(0) retains the full KV cache and oversized negative crops retain stale tokens #47433

Open

oupsi forgot minimax special case

06d1b90

Madhav-000-s reviewed Aug 3, 2026

View reviewed changes

vasqu reviewed Aug 3, 2026

View reviewed changes

Uh oh!

Conversation

Cyrilvallez commented Aug 3, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Aug 3, 2026

Uh oh!

github-actions Bot commented Aug 3, 2026

Uh oh!

github-actions Bot commented Aug 3, 2026

CI recap

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasqu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Cyrilvallez commented Aug 3, 2026 •

edited by github-actions Bot

Loading