rope_theta is ignored on the dense attention path (hardcoded 10000 / 1000000)
- Langage dominant
- C++
- Étoiles
- 7k
- Forks
- 660
- Merge moyen
- 20 h 43 min
- PR mergées (30 j)
- 33
Description
`ModelConfig::rope_theta` ([configs.h:852](https://github.com/google/gemma.cpp/blob/dev/gemma/configs.h#L852)) is read in exactly one place — the `mla_*` timescales in `struct Activations` ([activations.h:512-520](https://github.com/google/gemma.cpp/blob/dev/gemma/activations.h#L512-L520)), i.e. the MLA path. The dense path builds its two tables from constants instead:
```cpp
inv_timescale(
CreateInvTimescale(allocator, layer_config.qkv_dim,
layer_config.post_qk == PostQKType::HalfRope)),
inv_timescale_global(
CreateInvTimescale(allocator, max_qkv_dim,
layer_config.post_qk == PostQKType::HalfRope,
1000000.0, config.partial_rotary_factor)) {
```
([activations.h:126-132](https://github.com/google/gemma.cpp/blob/dev/gemma/activations.h#L126-L132) — the 10000.0 for the local table comes from the default argument at [ops/ops.h:30](https://github.com/google/gemma.cpp/blob/dev/ops/ops.h#L30).)
**Nothing is wrong today.** Gemma 2 / PaliGemma / T5Gemma / Gemma3-270M want 10000 on every layer; Gemma 3 and Gemma 4 want 10000 local + 1e6 global; Qwen3 wants 1e6 and reaches it via `use_global_timescale` plus all-global windows. Every model in the tree matches one of the two constants, so this is latent rather than a live bug.
What concerns me is the failure mode for the *next* model. `rope_theta` looks like the knob for this, silently isn't on the dense path, and a mismatch produces no assert and no warning — just quietly degraded output that doesn't look like a position-encoding problem. A Llama-style 500000, or any future Gemma with different values, would run at 10000/1e6 and merely seem "worse than expected."
### Proposed fix
1. Pass `config.rope_theta` to the local `CreateInvTimescale` call (`config` is already a ctor parameter, so no new plumbing).
2. Add `ModelConfig::global_rope_theta = 1000000.0f`, appended at the end of `VisitFields` for serialization compatibility, plus the matching entry in `python/configs.cc`; pass it to the global call.
3. Optionally drop the `base_frequency` default from `CreateInvTimescale` so every call site has to state its theta.
4. Set both fields explicitly in the Qwen3 configs, so their correctness is stated rather than incidental.
This is behavior-preserving: the only assignment of `rope_theta` anywhere is DeepSeek's `= 10000.0f` ([configs.cc:672](https://github.com/google/gemma.cpp/blob/dev/gemma/configs.cc#L672)), which equals the default, and the new field's default equals the current literal — so every config in the tree produces bit-identical timescales. It should be checkable against existing goldens.
### One design question
After this, `use_global_timescale` is arguably redundant: the selector at [attention.cc:156](https://github.com/google/gemma.cpp/blob/dev/gemma/attention.cc#L156) could use the global table whenever `global_rope_theta != rope_theta`. That removes a flag you can forget to set — setting only `global_rope_theta` today silently does nothing — but it touches the 10 configs that set it. Happy to keep the flag or drop it, whichever you prefer.
Glad to send a PR if this looks right.
(Separately, and I can file it on its own if it's worth a look: `partial_rotary_factor` is passed only to the global table, so on Gemma 4 2B — where 4 of every 5 layers are local — the local layers get full rotary. I couldn't tell from the code whether that asymmetry is intentional.)
Guide de contribution
Ouvrir le guide de contribution
Piste de recherche
Commencez par la construction de l’échelle temporelle dense dans activations.h et CreateInvTimescale dans ops/ops.h, puis examinez la sérialisation de ModelConfig dans configs.h et python/configs.cc. Consultez attention.cc pour comprendre la sélection de la table globale. C’est terminé lorsque les valeurs theta locales et globales configurées sont sérialisées et utilisées sans modifier les sorties existantes ; vérifiez-le avec les goldens existants.
Rédigé par le modèle d'indexation à partir du texte de l'issue.
Évaluation
- Stack technique
- cpp, python
- Domaine
- machine-learning
- Type d'issue
- Bug
- Difficulté
- 4/5
- Temps estimé
- 3-5 jours
- Activité
- Active
- Clarté
- Plutôt claire
- Accessibilité débutants
- 64/100