Cache: slight change in naming by zucchini-nlp · Pull Request #32421 · huggingface/transformers

zucchini-nlp · 2024-08-05T05:40:26Z

What does this PR do?

Following #32315 (comment), this PR adds two attributes on all cache classes, is_sliding and is_static. We can now rely on these attr to crop/prepare attention mask.

HuggingFaceDocBuilderDev · 2024-08-05T05:58:23Z

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.

gante

LGTM, thank you for working on this 💛

One design question about sliding caches

src/transformers/models/mistral/modeling_mistral.py

src/transformers/cache_utils.py

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

gante

LGTM, other than the deprecation cycle and the need to double-check attributes 🤗

gante · 2024-09-19T16:38:57Z

src/transformers/cache_utils.py

        # TODO: deprecate this function in favor of `cache_position`
        raise NotImplementedError("Make sure to implement `get_seq_length` in a subclass.")

-    def get_max_length(self) -> Optional[int]:


We will need a deprecation cycle for get_max_length before we can delete it :P

(in the raised warning, make sure you mention that users should use get_max_cache_shape instead)

gante · 2024-09-19T16:40:34Z

src/transformers/cache_utils.py

    """

+    is_static = False
+    is_sliding = False


(double-check whether this new attribute is correct, after rebasing. I think we've added new cache classes since your original commit. In the encoder-decoder cache, these attributes should be loaded from the decoder cache)

yes, I ran tests for generation in llama and all compile-static tests in whisper. Rebased main and fix-copies propagated changes to some new models

ArthurZucker

Thanks for the in depth changes! Not convinced we have to change the name of the function, and also convinced we should have abstraction on what we call. Checking for is static IMO is not a good way forward. It might be a good internal helper, but in the modeling we would prefer not having to check that wdyt?

Also fine for this specific case TBH! Just not sure it's gonna scale

ArthurZucker · 2024-09-20T14:37:54Z

src/transformers/models/bloom/modeling_bloom.py

+        if (
+            self.config._attn_implementation == "sdpa"
+            and past_key_values is not None
+            and not past_key_values.is_static


I am not entirely sure we want to add these kind of checks directly in the modeling code:
past_key_values.is_static does not really look specific, but if you allow checking is_sliding then we are gonna have is_encoder_decoder and etc.

Aligned with my previous comment where we need a general past_key_value.skip_sdpa_correction() for example

Hmm, maybe we can make a better check to not use isinstance() on cache objects. We are having more varieties of cache now and it is mostly either dynamic or static type of some caching method, so it would be nice to not rely on isinstance() checks in general

We can remove modeling changes in this PR and use the new attributes in specific cases (linked comment from PR description). What I need from this PR currently is the get_max_length that doesn;t return None anymore

Yeah Also want to avoid .is_static and isinstance etc. Not sure how to abstract best tho!

Okay, let's remove then for now

okey, done and rebased main

ArthurZucker · 2024-10-03T07:33:23Z

src/transformers/cache_utils.py


-    def get_max_length(self) -> Optional[int]:
-        """Returns the maximum sequence length of the cached states."""
+    def get_max_cache_shape(self) -> Optional[int]:


is there a specific reason for the name change ?

The only reason is to be more specific what we mean by max_length since I wanted to add max_length in sliding cache, which currently doesn't have any (returns None). And as comment in code explains it was done with idea that the cache object technically handles infinite amount of tokens, so no max length

But we never want to know how many tokens can cache technically handle, our checks are all about max capacity of particular cache instance

Let's add this in the comment for example! I was not sure 🤗

ArthurZucker · 2024-10-03T07:34:50Z

src/transformers/models/bloom/modeling_bloom.py

        sequence_length = input_tensor.shape[1]
-        if using_static_cache:
-            target_length = past_key_values.get_max_length()
+        if past_key_values is not None and past_key_values.is_static:


ArthurZucker

Thanks!

* squash * codestyle * Update src/transformers/cache_utils.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * propagate changes to all cache classes * + whisper * fix tests * more fixes * add deprecation warning * fix copies * address comments * fix mistral also * these didn't have "copied from" --------- Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

zucchini-nlp requested a review from gante August 5, 2024 05:40

gante reviewed Sep 5, 2024

View reviewed changes

src/transformers/models/mistral/modeling_mistral.py Outdated Show resolved Hide resolved

squash

d4c573f

zucchini-nlp force-pushed the kv-cache branch from d59a60d to d4c573f Compare September 6, 2024 13:51

codestyle

28d9e2f

gante reviewed Sep 6, 2024

View reviewed changes

src/transformers/cache_utils.py Outdated Show resolved Hide resolved

zucchini-nlp and others added 6 commits September 6, 2024 19:24

Update src/transformers/cache_utils.py

aee2bd4

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

propagate changes to all cache classes

5cc6f33

+ whisper

3b0eada

Merge branch 'main' into kv-cache

b1d6740

fix tests

ac12c89

more fixes

f07479c

zucchini-nlp requested a review from gante September 9, 2024 09:56

zucchini-nlp mentioned this pull request Sep 19, 2024

Phi3: fix attn for sliding window #33586

Merged

gante approved these changes Sep 19, 2024

View reviewed changes

zucchini-nlp added 3 commits September 26, 2024 17:44

add deprecation warning

c649d95

merge main

c9de458

fix copies

a66806a

zucchini-nlp requested a review from LysandreJik September 26, 2024 16:05

ArthurZucker requested review from ArthurZucker and removed request for LysandreJik October 1, 2024 07:36

ArthurZucker reviewed Oct 3, 2024

View reviewed changes

zucchini-nlp added 2 commits October 7, 2024 09:56

Merge remote-tracking branch 'upstream/main' into kv-cache

fae7e4f

address comments

54247bb

zucchini-nlp changed the title ~~Cache: add class attributes~~ Cache: slight change in naming Oct 7, 2024

zucchini-nlp added 2 commits October 7, 2024 10:19

fix mistral also

10633c1

these didn't have "copied from"

02dcde3

zucchini-nlp requested a review from ArthurZucker October 7, 2024 08:41

ArthurZucker approved these changes Oct 7, 2024

View reviewed changes

zucchini-nlp merged commit bead0fa into huggingface:main Oct 8, 2024

Conversation

zucchini-nlp commented Aug 5, 2024

What does this PR do?

Uh oh!

HuggingFaceDocBuilderDev commented Aug 5, 2024

Uh oh!

gante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gante 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!

ArthurZucker 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!

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!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants