Fix kvzip having poor performance#152
Merged
maxjeblick merged 1 commit intomainfrom Nov 10, 2025
Merged
Conversation
Signed-off-by: Max Jeblick <maximilianjeblick@gmail.com>
Collaborator
Author
|
/ok to test 7520bdc |
Collaborator
Author
|
Ruler 4k @ 50% cr, for qwen3-8b |
SimJeg
approved these changes
Nov 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description
Probable fix of #147.
Root cause of the issue:
kwargs.get("past_key_values", None)returnsDynamicCache(layers=[])bool(DynamicCache(layers=[]))equals to false, as bool method falls back to__len__andlen(DynamicCache(layers=[])) == len([]) = 0.self._cache=None, and it won't be used to perform kvzip compression.Cause of this bug:
Transformers library 4.54. changed the naming convention from
past_key_valuetopast_key_values.During some transition period, however, there existed some back on forth on the naming convention ( transformers changed cache implementation a few times, see e.g .#104). I decided to make kvzip compatible with both naming conventions while working on #115 to allow for smoother devleopment. As it turned out, my implementation was faulty.
Comments:
There are other parts in kvzip that can be refactored. For this PR, I'll only address the actual bug fix to have better visibility.
Checklist
Before submitting a PR, please make sure:
make test)make style, on errors try fix withmake format)git commit -s