Conversation
M-RoPE enlarges max_position_embeddings 4x so video timestamps that run past the token sequence still land inside the cos/sin cache. Models whose unpruned M-RoPE positions provably stay within the configured sequence pay for that headroom without using it. Add an opt-in, fail-closed position-capacity contract: - A concrete model class certifies itself by declaring mrope_positions_are_sequence_bounded in its own body. The capability is read off __dict__, so a declaration inherited from a parent does not certify a derived position implementation; Qwen3_5MoeForConditionalGeneration is a subclass of the certified dense entrypoint and stays on the legacy allocation. - The YaRN semantic maximum is unchanged. Its correction range is derived from max_position_embeddings, so the parent still receives the enlarged value; only the physical cache row count is reduced. - Multimodal pruning can push positions past the reduced sequence length, so ModelConfig falls back to the legacy allocation whenever it is enabled. - The resolved capacity is part of the get_rope instance key and of ModelConfig.compute_hash. - Only the dense Qwen3_5ForConditionalGeneration path is certified here. Models without a bound keep delegating to the base and YaRN cache implementations unchanged, so their caches stay bit-for-bit identical. Bounded models build the cache from cache_max_position_num instead: the cache is created on the accelerator, so materializing the semantic-sized cache and slicing it afterwards would not lower the construction peak this bound exists to reduce. Signed-off-by: lesj0610 <lesj0610@godoiksan.org>
b8591e0 to
cd31a80
Compare
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesM-RoPE cache bounds
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change bounds M-RoPE cache allocation only for explicitly certified model paths while preserving legacy behavior elsewhere, with targeted correctness and regression checks reported as passing. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ModelConfig
participant Qwen3NextAttention
participant get_rope
participant MRotaryEmbedding
participant RopeRegistry
ModelConfig->>Qwen3NextAttention: mrope_cache_max_position
Qwen3NextAttention->>get_rope: pass cache bound
get_rope->>RopeRegistry: key lookup including cache bound
get_rope->>MRotaryEmbedding: construct with cache bound
MRotaryEmbedding-->>get_rope: bounded rotary cache
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ition-bounds Signed-off-by: lesj0610 <lesj0610@users.noreply.github.com>
Purpose
M-RoPE currently uses a legacy 4x maximum both for position semantics and for the physical cosine/sine cache. The larger range is required by models whose video timestamps can exceed the token sequence length, but it over-allocates the physical cache for models whose M-RoPE positions are proven to stay within the configured sequence bound.
This change adds an explicit, fail-closed position-capacity contract:
__dict__, so a declaration inherited from a parent does not certify a derived position implementation.Qwen3_5MoeForConditionalGenerationis a subclass of the certified dense entrypoint and stays on the legacy allocation.ModelConfigfalls back to the legacy allocation whenever pruning is enabled.Qwen3_5ForConditionalGenerationpath is certified here. Qwen3.5 MoE, Qwen2/2.5-VL, Qwen3-VL, Omni, Transformers-delegated, and out-of-tree models retain the legacy path.Models without a bound keep delegating to the base and YaRN cache implementations, so their caches stay bit-for-bit identical. Bounded models build the cache from
cache_max_position_numfromtorch.arangeonwards rather than materializing the semantic-sized cache and slicing it: the cache is created on the accelerator inside the model loader'starget_devicecontext, so slicing afterwards would not lower the construction peak this bound exists to reduce, and would add a copy on top of it.There are no model-name, quantization, GPU, FlashInfer, or environment-specific branches.
Correctness coverage
_ROPE_DICTis exercised under an isolated dict: the same parameters with and without a bound must not alias, and the unbounded entry must still be reused.Qwen3_5ForConditionalGeneration/Qwen3_5MoeForConditionalGenerationpair is asserted to be a real subclass relationship with opposite capability results.ModelConfig.mrope_cache_max_positioncovers certified/uncertified crossed with pruning enabled/disabled, and the bound is asserted to changecompute_hash()._ModelInforegistry cache is exercised throughinspect_model_cls(): a cache file written before the field existed is re-inspected and rewritten once, and the following call is a warm hit that does not re-inspect the model class. The field intentionally carries no dataclass default.Validation
Base commit
e25c586b90,torch 2.13.0+cu130/ CUDA 13.0, GPUNVIDIA CMP 170HXcompute capability(8, 0).tests/kernels/core/test_mrope.py: 50 passed.tests/models/test_registry.py: 381 passed, 4 failed. All four (HCXVisionForCausalLM,Dots3NoteForCausalLM,Dots3NoteMTPModel,KananaVForConditionalGeneration) reproduce on unmodifiede25c586b90, so they are pre-existing baseline failures rather than regressions.ruff check,ruff format --check, and the full pre-commit hook set (includingmypyfor Python 3.10, SPDX, and DCO sign-off) passed.Isolated M-RoPE cache-construction measurement, using the served checkpoint's own rope parameters (
head_size=256,partial_rotary_factor=0.25sorotary_dim=64,mrope_interleaved=True,mrope_section=[11, 11, 10],rope_theta=10000000,max_position_embeddings=262144, bound= max_model_len = 262144,bfloat16). Each case clears_ROPE_DICT, callstorch.cuda.empty_cache()andreset_peak_memory_stats(), builds underwith torch.device("cuda:0")to mirror the loader'starget_devicecontext, then synchronizes and takes the delta ofmemory_allocated()andmax_memory_allocated():(1048576, 64)(262144, 64)Both figures match the arithmetic: legacy
4 + 128 + 128 + 128 + 256 = 644 MiBand bounded1 + 32 + 32 + 32 + 64 = 161 MiBoft,freqs,cos,sin, and the concatenated result, with a finalbfloat16buffer of 128 MiB versus 32 MiB.These are allocated-memory figures for the M-RoPE cache construction in isolation, not a whole-model initialization peak. The serving-level KV capacity impact was not measured: it depends on engine configuration, block alignment, and memory state, which would add more noise than signal to this change's evidence.
Essential Elements of an Effective PR Description Checklist
Summary by CodeRabbit
New Features
Bug Fixes