Repository navigation
fix(internlm2): add family rope_scaling tests and NTK rescale log - #1736
Conversation
PR #1389 already wired internlm2::Attention to the shared DynamicNtkRope schedule (issue #1324), so the rope_scaling and rope_traditional plumbing itself was already correct on main. This closes the remaining gap issue #1320 asked for: family-level tests that prove ModelArgs::rope() reaches the helper (linear scale, rope_traditional, the checkpoint's own dynamic table, the rope_type spelling, and the factor-less load error), a small traditional()/dims() accessor pair on DynamicNtkRope needed by those tests, and a one-time debug-level log of base_eff the first time any dynamic schedule crosses max_position_embeddings, gated by a process-wide AtomicBool so 32 layers times every decode step past the boundary do not spam it. Also corrected the checkpoint path in existing comments from the nonexistent models/internlm2-7b-4bit to the real models/internlm2_5-7b-chat-4bit, and named that checkpoint as InternLM2's own validation checkpoint in docs/supported-models.md. Validated against models/internlm2_5-7b-chat-4bit: the main binary built before this change and the rebuilt binary produce byte-identical output (MD5-matched) on a 73-word prompt with -n 64 -t 0, confirming the schedule is unchanged inside max_position_embeddings. cargo test --profile test-fast --features metal,accelerate --lib models::internlm2 and --lib models::dynamic_ntk_rope both pass (8 and 16 tests respectively), and cargo clippy --lib --tests --features metal,accelerate -- -D warnings and cargo fmt --all -- --check are clean. Refs #1320
9943dc2 to
c26adb2
Compare
The process-wide `AtomicBool` guarding the new `base_eff` debug log contradicts the convention `rope_utils::report_unusable_rope_scaling_once` documents: a process can hold more than one model (`--models-dir` routing, pipeline stage executors, tensor-parallel ranks), and a single flag lets the first checkpoint past `max_position_embeddings` silence every later one. The guard is now a `OnceLock<Mutex<BTreeSet<_>>>` keyed on `(dims, base bits, max_position_embeddings, factor bits)`, matching that precedent, and `log_dynamic_rescale_once` returns `bool` so the dedup is testable rather than only observable through stderr. The lock is taken only past the boundary, where a decode step already costs about a second. Drops `DynamicNtkRope::dims()`, which had no call site in the tree, and corrects `traditional()`'s `// Used by:` line, which named InternLM3 and non-test InternLM2 callers that do not exist. `docs/supported-models.md` credited the short-prompt run with proving the plumbing, but that A/B was against this branch's parent, which already carries #1389's wiring. It now records the long-context result that does prove it, `base_eff=1077737.0` at 34021 tokens; `internlm2.rs`'s module doc likewise now credits #1389 for the wiring. Validation: `--lib models::dynamic_ntk_rope` 17 passed (one new dedup test), `--lib models::internlm2` 8 passed, clippy and fmt clean. Rebuilt and re-ran the real-checkpoint A/B on `models/internlm2_5-7b-chat-4bit` against a binary built from this branch's parent: an 89-token prompt at `-n 64 -t 0` and a 34021-token prompt at `-n 32 -t 0` are both byte-identical, and the long run emits exactly one rescale line. Refs #1320
`apply` called `log_dynamic_rescale_once` unconditionally, so every forward past `max_position_embeddings` took the global dedup set's lock even with no subscriber installed. That is 64 uncontended acquisitions per decode step on a 32-layer model, and the set is shared across every schedule in the process, so tensor-parallel ranks sharing one process serialize on it. Asking `tracing::enabled!` first makes the default run lock-free. The gate sits in `apply` rather than inside the helper because the dedup test calls the helper directly and asserts on its return value. Also records two properties review surfaced. The logged base is a snapshot taken at the first crossing rather than the base in force later in the run, because the dedup key deliberately omits `seq_len`. And the dedup test asserts on process-global state, so the geometries it depends on are now reserved in a comment beside the shared test constants, where a later test author will see them.
The catalog entry credited the InternLM2 checkpoint with "the second half of the evidence" directly beside InternLM3's corrected mlx-lm parity result, which reads as a comparable claim and is not one. The byte-identity runs recorded in #1320 compare this tree against the same tree with only the schedule's debug diagnostic removed, so the two arms differ by a log line and cannot disagree by construction. That is a control showing the diagnostic is inert, not a result about the rotation, and the repo's own rule against settling for byte-identity applies to it. What the 34021-token run does establish is that the dynamic branch is entered and computes the documented base. The f32 evaluation is load-bearing there rather than incidental: f64 gives 1077674.9 at that length against the observed 1077737.0. No corrected long-context reference exists for this family the way one was built for InternLM3, and the entry now says so instead of implying otherwise. Also points the test module header at both issues, since five of its eight tests came from #1320 rather than from #1324's shared helper.
…ents Comment-only, no line of code changes. Security review read the block closely and found each of these stated something the code does not do. The key's doc claimed `from_scaling` had already rejected NaN through `is_usable_scalar`. That screen covers `factor` only; `base` arrives from `rope_theta` by way of `rope_overrides::resolve_base` and reaches `to_bits()` unscreened. The key is an identity on bit patterns, not on values, and the comment now says so along with what that would cost: a `-0.0` or NaN base could split one schedule across two log lines, and nothing worse. The poisoning-recovery comment blamed a subscriber panicking while logging, which cannot happen, because the guard is dropped before the event is emitted. Only a panic inside `insert` could poison this lock, which in practice means nothing since allocation failure aborts rather than unwinds. The recovery stays; only the reason was wrong. The cost argument cited a decode step costing on the order of a second. That is the 34021-token prefill, not a decode step, which runs tens of milliseconds at these lengths. The argument holds on the real numbers and now uses them, including the part the old comment left out: after the line is emitted the lock is still taken per call for a lookup that can only answer "already present". The `tracing::enabled!` guard declares no fields while the event it guards declares six, so a field-selecting `EnvFilter` directive could enable the event and not the guard and silently drop the diagnostic. Every documented way in is a level directive, `-v` and `--verbosity 4` both expand to one in `server::logging` and the issue's recipe is `RUST_LOG=debug`, so the gate stays and the limit is now written down beside it. Refs #1320
PR Finalization CheckReviewed test coverage, documentation, and lint/format on Tests
Documentation
Lint / format
All PR CI checks ( |
The InternLM2 entry cited 1077674.9 as the f64 value of the dynamic-NTK closed form at 34021 tokens and concluded from the gap to the logged 1077737.0 that the f32 evaluation was load-bearing. It is not: 1077674.9 is the same closed form one position short, at 34020. At 34021 f64 gives 1077736.9875 and the f32 chain the code runs gives 1077737.0, so the two agree to every digit the log prints, and what the number establishes is only that the dynamic branch was entered at the length the line reports. Re-measured on `models/internlm2_5-7b-chat-4bit` with the branch binary rather than recomputed on paper. A 34021-token prompt under `RUST_LOG=debug` emits exactly one rescale line, `seq_len=34021 base_eff=1077737.0`, and answers the question the prompt ends with in fluent on-topic text. A 34022-token prompt emits `base_eff=1077799.125`, which an f32 evaluation of the same form reproduces bit for bit; the 62-per-token gap between the two is why the length has to be quoted with the value, and it is the gap the old parenthetical mistook for a precision effect. Refs #1320
Bilingual report dated today rather than by mergedAt, because `TECHNICAL_REPORTS/.keep-reports` opts this repository into committing the report into the PR branch before the merge. It records the finding that shaped the branch: #1320's whole implementation plan had already merged under #1389, so what was left was the issue's validation half. It then covers what the five family-level tests pin that a helper test cannot see, the three decisions behind the rescale diagnostic (per-schedule dedup instead of a process-wide flag, the `tracing::enabled!` gate in `apply`, and the field-filter blind spot that gate leaves open), the three documentation corrections, and the limits of the validation: the byte-identity pair is a control rather than a parity result, and no corrected long-context reference exists for this family. Refs #1320
Summary
PR #1389 already wired
internlm2::Attentionto the sharedDynamicNtkRopeschedule (issue #1324), so therope_scalingandrope_traditionalplumbing itself was already correct on main. This PR closes the rest of #1320: family-level tests proving that plumbing reaches the helper, a debug log of the rescaled base, and real-checkpoint validation.What changed
src/models/internlm2_tests.rs: tests thatModelArgs::rope(), not just the helper, resolves alinearblock to an inverse-factor scale, carriesrope_traditionalthrough in both states, reproduces the issue's dynamic table (base_for(40000),base_for(65536)), accepts therope_typespelling, and rejects a factor-lessdynamicblock with an error naming the family.src/models/dynamic_ntk_rope.rs: atraditional()accessor, and a debug-level log ofbase_effthe first time a dynamic schedule crossesmax_position_embeddings. The dedup is keyed on the schedule's own parameters rather than a process-wide flag, followingrope_utils::report_unusable_rope_scaling_once, so a second checkpoint in the same process (--models-dirrouting, pipeline stages, TP ranks) still gets its line.applyaskstracing::enabled!before it takes the dedup lock, so a run with no debug subscriber never touches that shared lock; the rotary arithmetic is unchanged either way.docs/supported-models.mdnamed a checkpoint directory that does not exist (models/internlm2-7b-4bit); corrected tomodels/internlm2_5-7b-chat-4bit, and the entry now records the long-context evidence rather than crediting the short run with it, at the figures the closed form actually gives.Validation
--lib models::internlm2(8 passed) and--lib models::dynamic_ntk_rope(17 passed) under--profile test-fast --features metal,accelerate;cargo clippy --lib --testsandcargo fmt --all --checkclean, which is the scopeci.ymlruns. The--workspacegate in issue #1320's acceptance criteria runs innightly-verify.yml, not on this PR.Real checkpoint
models/internlm2_5-7b-chat-4bit, greedy. Against a binary built from this branch's parent commit, an 89-token prompt at-n 64and a 34021-token prompt at-n 32both produce byte-identical generated text (MD55a6d6133and69a12bff). That pair is a control rather than a parity result: the parent already carries #1389's wiring, so the two arms differ only by the debug diagnostic this PR adds, and byte-identity is what it should show.The long-context run was repeated against the final binary, after the
tracing::enabled!gate was added, so the diagnostic is confirmed to still reach aRUST_LOG=debugrun rather than only having done so before that commit. A 34021-token prompt emits exactly one line,seq_len=34021 base_eff=1077737.0, and the generation answers the question the prompt ends with in fluent on-topic text (Rayleigh scattering).1077737.0is the issue's closed form at that length: f64 gives 1077736.99 there, so the f32 the code computes agrees to every digit the line prints, and a 34022-token prompt givesbase_eff=1077799.125, which the same f32 evaluation reproduces bit for bit. The base moves about 62 per token at that length, which is why the entry indocs/supported-models.mdnow quotes the value with its length; its previous parenthetical read the closed form at 34020 as an f64-versus-f32 gap at 34021. The same form gives the issue's table at 40000 (1449795.74) and 65536 (3052773.67).Closes #1320