Repository navigation
Conversation
|
run-slow: t5 |
|
This comment contains models: ["models/t5"] |
|
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. |
… cache `_prepare_static_cache` now calls `early_initialization` on BOTH the self-attention and cross-attention caches for encoder-decoder models. Previously only the self-attention cache was initialized eagerly. For encoder-decoder models (e.g. T5), cross-attention K/V tensors are computed from encoder hidden states on the first decode step, which happens inside the CUDAGraph capture context. When `lazy_initialization` runs there, `is_torchdynamo_compiling()` is True so `mark_static_address` is skipped. CUDAGraphs then pools the tensor memory addresses, causing: RuntimeError: Error: accessing tensor output of CUDAGraphs that has been overwritten by a subsequent run. Regression introduced by #47731 ("Stop setting the static cache as an attribute to save memory"), which changed `_prepare_static_cache` to always create a fresh `StaticCache` instead of reusing `self._cache`. Before that PR, the same cache object was reused across `generate` calls and was already initialized from the first call (outside any compiled context), so `lazy_initialization` never ran inside capture. Also folds the decoder-only early-init (previously only for chunked prefill, #46421) into the same unconditional block, since the same CUDAGraph issue applies to any compiled generate call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6a63cb1 to
7d2a05a
Compare
|
run-slow: auto, bert, gpt2, t5, modernbert, vit, clip, detr, table_transformer, got_ocr2, whisper, wav2vec2, qwen2_audio, speecht5, csm, llama, gemma3, qwen2, mistral3, qwen2_5_vl, llava, smolvlm, internvl, gemma3n, gpt_oss, qwen2_5_omni, pi0 |
|
This comment contains models: ["models/auto", "models/bert", "models/clip", "models/csm", "models/detr", "models/gemma3", "models/gemma3n", "models/got_ocr2", "models/gpt2", "models/gpt_oss", "models/internvl", "models/llama", "models/llava", "models/mistral3", "models/modernbert", "models/pi0", "models/qwen2", "models/qwen2_5_omni", "models/qwen2_5_vl", "models/qwen2_audio", "models/smolvlm", "models/speecht5", "models/t5", "models/table_transformer", "models/vit", "models/wav2vec2", "models/whisper"] |
| dtype=self.dtype, | ||
| device=self.device, | ||
| ) | ||
| if self.config.is_encoder_decoder: |
There was a problem hiding this comment.
Why touch instead of overriding in ?
Overriding _prepare_static_cache in T5 is technically feasible, but the bug is not T5-specific: any encoder-decoder model using static_cache + torch.compile would hit the same issue (cross-attention K/V are allocated inside the graph capture, so mark_static_address is never called). T5 is just the only encoder-decoder with a test_compile_static_cache CI test today.
Putting the fix here means BART, Whisper, and any future encoder-decoder model get the correct behaviour automatically, instead of requiring the same override to be copy-pasted into each. The elif prefill_chunk_size block that was already here is the exact same pattern for decoder-only models (#46421), so this just extends it symmetrically to the encoder-decoder case.
…t case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r decoder-only The previous commit incorrectly removed the `elif prefill_chunk_size` guard for decoder-only models. For decoder-only models without chunked prefill, lazy_initialization runs during the (non-compiled) prefill, so mark_static_address is correctly applied outside graph capture — early init is not needed there. Restore the original elif prefill_chunk_size block for decoder-only models, and scope the new encoder-decoder early init as a separate if branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
run-slow: t5 |
|
This comment contains models: ["models/t5"] |
|
|
||
| if self.config.is_encoder_decoder: | ||
| # Eagerly initialize the K/V buffers so that `mark_static_address` is applied before any | ||
| # `torch.compile` / CUDAGraph capture context. Without this, `lazy_initialization` runs inside | ||
| # graph capture (where `is_torchdynamo_compiling()` is True), causing `mark_static_address` to | ||
| # be skipped. CUDAGraphs then treats those tensors as pooled memory and can overwrite them | ||
| # between runs, raising: | ||
| # "accessing tensor output of CUDAGraphs that has been overwritten by a subsequent run" | ||
| # BOTH self-attention and cross-attention caches need early init: cross-attention K/V are | ||
| # computed from encoder hidden states on the first decode step, which happens inside the | ||
| # compiled graph, so they suffer the same lazy-init-inside-capture problem. | ||
| # Skipped when the cache cannot be initialized on a single device. | ||
| init_shape = self._get_static_cache_init_shape() | ||
| if init_shape is not None: | ||
| num_heads, head_dim = init_shape | ||
| early_init_kwargs = dict( | ||
| batch_size=batch_size, | ||
| num_heads=num_heads, | ||
| head_dim=head_dim, | ||
| dtype=self.dtype, | ||
| device=self.device, | ||
| ) | ||
| cache.self_attention_cache.early_initialization(**early_init_kwargs) | ||
| cache.cross_attention_cache.early_initialization(**early_init_kwargs) |
There was a problem hiding this comment.
@Cyrilvallez Let me know if you still want to have this fix only in T5 modeling code. I think the issue is there for all encoder-decoder models, but among them, only T5 has test_compile_static_cache.
|
run-slow: t5 |
|
This comment contains models: ["models/t5"] |
…oder branch Pyright infers `cache` as `StaticCache | EncoderDecoderCache` after the conditional reassignment, so accessing `.self_attention_cache` / `.cross_attention_cache` inside the guarded `if self.config.is_encoder_decoder` block raises unresolved-attribute errors. Adding an `assert isinstance` narrows the type without changing runtime behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| cache = EncoderDecoderCache(cache, StaticCache(**cross_attention_cache_kwargs)) | ||
|
|
||
| if self.config.is_encoder_decoder: | ||
| assert isinstance(cache, EncoderDecoderCache) # always true: assigned above when is_encoder_decoder |
There was a problem hiding this comment.
this isinstance(cache, EncoderDecoderCache) is to avoid Pyright (check_types.py) complain (fail)
CI recapDashboard: View test results in Grafana |
|
gently ping @Cyrilvallez again to help us to have all important models have ✅ tests 🙏 |
| "device": self.device, | ||
| } | ||
| cache.self_attention_cache.early_initialization(**early_init_kwargs) | ||
| cache.cross_attention_cache.early_initialization(**early_init_kwargs) |
There was a problem hiding this comment.
- I assume enc-dec always have the same shapes for head dim and num heads? Would investigate on real models
- Wouldn't it make much more sense to adjust the encoder-decoder cache's implementation of
early_initialization
For 1. I could also see some adjustments for _get_static_cache_init_shape which might be related to #48619 (meaning we could combine something using the new utils introduced there)
|
cc @CyrilBurner |
CyrilBurner
left a comment
There was a problem hiding this comment.
Are you sure?? Prefill is never compiled so should not be an issue
| # Eagerly initialize the K/V buffers so that `mark_static_address` is applied before any | ||
| # `torch.compile` / CUDAGraph capture context. Without this, `lazy_initialization` runs inside | ||
| # graph capture (where `is_torchdynamo_compiling()` is True), causing `mark_static_address` to | ||
| # be skipped. CUDAGraphs then treats those tensors as pooled memory and can overwrite them | ||
| # between runs, raising: | ||
| # "accessing tensor output of CUDAGraphs that has been overwritten by a subsequent run" | ||
| # BOTH self-attention and cross-attention caches need early init: cross-attention K/V are | ||
| # computed from encoder hidden states on the first decode step, which happens inside the | ||
| # compiled graph, so they suffer the same lazy-init-inside-capture problem. | ||
| # Skipped when the cache cannot be initialized on a single device. | ||
| init_shape = self._get_static_cache_init_shape() | ||
| if init_shape is not None: | ||
| num_heads, head_dim = init_shape | ||
| early_init_kwargs = { | ||
| "batch_size": batch_size, | ||
| "num_heads": num_heads, | ||
| "head_dim": head_dim, | ||
| "dtype": self.dtype, | ||
| "device": self.device, | ||
| } | ||
| cache.self_attention_cache.early_initialization(**early_init_kwargs) | ||
| cache.cross_attention_cache.early_initialization(**early_init_kwargs) |
There was a problem hiding this comment.
I don't get it, I believe all this comment is wrong since prefill is never compiled - since it's an AI comment, I believe the AI probably hallucinated it
There was a problem hiding this comment.
Thanks for the note! To clarify: "the compiled graph" in the comment refers to the compiled decoder step, not the encoder. The encoder runs eagerly, but the cross-attention K/V computation (and lazy_initialization) happens on the first decoder step, which is inside the compiled graph — that's where mark_static_address gets skipped.
|
cc @Cyrilvallez for the redirect :D but yes this is a valid issue because the decoder part of the enc-dec models are not properly accounted for (even if the AI comment is heavy lol) |
|
Superseded by #48923 |
What
Fixes
T5ModelIntegrationTests::test_compile_static_cachewhich fails with:Root cause
_prepare_static_cachemust callearly_initializationon cache buffersbefore any CUDAGraph capture so that
mark_static_addressis appliedwhile
is_torchdynamo_compiling()is False. Without it,lazy_initializationruns inside the capture context,
mark_static_addressis skipped, andCUDAGraphs pools the tensor memory — overwriting it on the next replay.
For encoder-decoder models both the self-attention and cross-attention
caches need early init. The self-attention cache is straightforward, but the
cross-attention cache is the key issue: its K/V tensors are computed from
encoder hidden states on the first decode step, which happens inside the
compiled graph. So
lazy_initializationruns inside graph capture, andmark_static_addressis never called.Regression introduced by #47731 ("Stop setting the static cache as an
attribute to save memory"): before that PR,
_prepare_static_cachereusedself._cacheacrossgeneratecalls, solazy_initializationhad alreadyrun outside any compiled context on the first call. After #47731, a fresh
StaticCacheis always created, making eager initialization necessary.Decoder-only models (e.g. afmoe) — also affected, not yet fixed here
The same regression from #47731 also affects decoder-only models with
test_compile_static_cache. Before #47731,self._cachewas reused acrossgeneratecalls, socumulative_length(and other buffers) always had thesame tensor address. After #47731, a fresh
StaticCacheis created on everycall, so
cumulative_lengthis at a different memory address each time —CUDAGraph replay references the old address and triggers the overwrite error.
Confirmed:
afmoe::test_compile_static_cachefails with the sameRuntimeError: Error: accessing tensor output of CUDAGraphson the Aug 05 CIcommit (
9f66415aec). The existingelif prefill_chunk_sizeblock does notcover this case.
This PR only fixes the encoder-decoder path (T5). The decoder-only fix
needs a separate look — would like @Cyrilvallez to have a first look before
going further.
Changes
_prepare_static_cache: add anif self.config.is_encoder_decoderbranchthat calls
early_initializationon bothself_attention_cacheandcross_attention_cachebefore graph capture.elif prefill_chunk_sizedecoder-only block is unchanged.Test plan
T5ModelIntegrationTests::test_compile_static_cache— verified PASS on A10G runner (torch 2.13, sm_86)T5ModelIntegrationTestspass