-
Notifications
You must be signed in to change notification settings - Fork 2.1k
FIX: DynamicCache max_cache_len attribute error #2735
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
FIX: DynamicCache max_cache_len attribute error #2735
Conversation
Resolves current CI errors with prefix tuning. Due to some recent changes in transformers (surfaced by huggingface/transformers#39797), checking hasattr(cache, max_cache_len) results in an error: >>> cache = DynamicCache() >>> hasattr(cache, "max_cache_len") Traceback (most recent call last): File "/home/name/work/forks/transformers/foo.py", line 9, in <module> hasattr(cache, "max_cache_len") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/name/work/forks/transformers/src/transformers/cache_utils.py", line 916, in max_cache_len return max(values) ^^^^^^^^^^^ ValueError: max() iterable argument is empty This has been reported and will be fixed in transformers. On the PEFT side, it is safeest check the cache type and avoid accessing this attribute in the first place, which is what this PR does. Morever, that PR also changed the argument order to initialize HybridCache (will probably also be reverted in transformers), which is also taken into account in this PR by only using keyword arguments.
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, should do the charm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, LGTM! Would maybe change from lower_equal_4_55 to lower_4_56 just in case, but either way should be fine, we don't have any other patches planned for 4.55 (at least at this moment)
| # TODO: remove this logic once transformers < 4.56 is dropped | ||
| transformers_le_4_55 = packaging.version.parse(transformers.__version__) <= packaging.version.parse( | ||
| "4.55.2" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would make more sense to make it lt_4_60 except I'm overlooking something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to transformers_lt_4_56.
It will be deprecated and later removed, so move the import inside a version guard.
Resolves current CI errors with prefix tuning.
Due to some recent changes in transformers (surfaced by huggingface/transformers#39797), checking
hasattr(cache, "max_cache_len")results in an error:This has been reported and will be fixed in transformers. On the PEFT side, it is safest check the cache type and avoid accessing this attribute in the first place, which is what this PR does.
Moreover, that PR also changed the argument order to initialize
HybridCache, which is also taken into account in this PR by only using keyword arguments.Update: This PR also takes care of a recent change in transformers that changed the cache type for Gemma to
DynamicCachefromHybridCache.Since the PR CI uses the transformers release, it won't show the error, but I tested locally and the failing tests pass.