Repository navigation
fix(generation): Enforce the auto-compile cache check for encoder-decoder models - #48364
Conversation
vasqu
left a comment
There was a problem hiding this comment.
Also cc @Cyrilvallez and @zucchini-nlp for generation in general
| # layers are, but we don't want to ALWAYS compile when calling `generate`, so we check the type. Encoder-decoder | ||
| # models hold that cache in a subcache, so we unwrap it to check the cache the decoder actually generates with | ||
| decoder_cache = cache.self_attention_cache if isinstance(cache, EncoderDecoderCache) else cache | ||
| using_compilable_cache = cache is not None and cache.is_compileable and type(decoder_cache) is not DynamicCache |
There was a problem hiding this comment.
| # layers are, but we don't want to ALWAYS compile when calling `generate`, so we check the type. Encoder-decoder | |
| # models hold that cache in a subcache, so we unwrap it to check the cache the decoder actually generates with | |
| decoder_cache = cache.self_attention_cache if isinstance(cache, EncoderDecoderCache) else cache | |
| using_compilable_cache = cache is not None and cache.is_compileable and type(decoder_cache) is not DynamicCache | |
| # Encoder-decoder models hold that cache in a subcache, so we unwrap it to check the cache the decoder actually generates with | |
| decoder_cache = cache.self_attention_cache if isinstance(cache, EncoderDecoderCache) else cache | |
| # Note: for some models that only use linear attention (e.g. Mamba), even a DynamicCache is compilable since all | |
| # layers are, but we don't want to ALWAYS compile when calling `generate`, so we check the type | |
| using_compilable_cache = cache is not None and cache.is_compileable and type(decoder_cache) is not DynamicCache |
let's split the comments per line more readable imo
There was a problem hiding this comment.
isn't checking cache.is_compileable enough in general, i don't think we have compileable Dynamic Cache 🤔
There was a problem hiding this comment.
Check the comment above (lin attn caches)
There was a problem hiding this comment.
Resolved, thank you both :)
Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
vasqu
left a comment
There was a problem hiding this comment.
Small last comments from my side
| def test_compileable_default_cache_doesnt_compile_encoder_decoder(self): | ||
| """Test that a compileable default cache doesn't trigger compilation on encoder-decoder models either""" | ||
| model = AutoModelForSeq2SeqLM.from_pretrained("hf-internal-testing/tiny-random-bart") | ||
| decoder_config = copy.deepcopy(model.config.get_text_config(decoder=True)) |
There was a problem hiding this comment.
Might be overkill since it only lives for this test
There was a problem hiding this comment.
You're right, dropped it :)
| ) | ||
| generation_config = GenerationConfig() | ||
| generation_config.compile_config = CompileConfig() | ||
| generation_config.compile_config._compile_all_devices = True |
There was a problem hiding this comment.
Just wondering, could we set it at construction time?
There was a problem hiding this comment.
Can't unfortunately, it's a normal attr not a dataclass field in CompileConfig. Set it like the other usages.
| generation_config = GenerationConfig() | ||
| generation_config.compile_config = CompileConfig() | ||
| generation_config.compile_config._compile_all_devices = True | ||
| self.assertFalse(model._valid_auto_compile_criteria({"past_key_values": cache}, generation_config)) |
There was a problem hiding this comment.
I guess it needs the static cache to be true?
There was a problem hiding this comment.
Yup because otherwise the test passes on CPU regardless. Static cache alone doesn't repro the issue either (both main and the PR return True). The actual diff in behavior is with a compileable DynamicCache (main returns True but the fix returns False) so that's the behavior we're trying to catch.
vasqu
left a comment
There was a problem hiding this comment.
Nice thanks for the explanations, let's merge
|
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. |
|
Hmm the failing test seems unrelated to me lemme try to rerun CI later today again |
CI recapDashboard: View test results in Grafana |
|
Syncing with main worked 🤷 should merge soon |
…oder models (huggingface#48364) * fix: Enforce the auto-compile cache check for encoder-decoder models * change: Update generation/utils.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * fix: Changes --------- Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
What does this PR do?
→ Follow-up to this comment on the Canary PR; the encoder-decoder cache holds two subcaches and the auto-compile check isn't propagated through them.
→ No behaviour change on
maincc: @ebezzam @vasqu
Code Agent Policy
Before submitting