Skip to content

tests, tokenizer and probe: close eleven open review follow-ups - #788

Closed
juliankang4 wants to merge 11 commits into
midagedev:mainfrom
juliankang4:dgx-spark-close-the-11-non
Closed

juliankang4 wants to merge 11 commits into
midagedev:mainfrom
juliankang4:dgx-spark-close-the-11-non

Conversation

@juliankang4

Copy link
Copy Markdown
Collaborator

이 PR은 리뷰에서 남은 비차단 지적 열한 건을 닫습니다. 테스트가 빠진 곳에 테스트를 더하고, gemma4 토크나이저가 너무 긴 공백을 잘못 처리하던 순서를 고치고, 읽기 속도를 재는 probe를 하나 더합니다.

What and why

Eleven follow-ups from earlier reviews of #106, #148, #172, #173, #241, #333, #339, #356, #540 and #755 were left open because none of them blocked a merge. This PR closes all of them. Eight add or tighten a test so that it catches the defect the review pointed at. One splits a function into stages, one changes tokenizer behavior, and one adds a timing arm to an existing probe.

  • tools/review/labels.py: a merged run prints | merged: nothing removed, no card label decided, as a short file list already did. The self-test checks that line.
  • crates/refset/src/mtpref/tests.rs: the draft-layer test takes a layer slice and refuses [47, 48], a set that names two draft layers, as malformed.
  • tools/check-arch.sh: the self-test pins rule 2's literal scan on fixed lines. "blk.", "deepseek2x" is not a hit and deepseek2.expert_count is. The blk_archscan comment in tools/check-recipes.sh now matches.
  • crates/gpu/src/host/link/pump.rs: the link relay tests fill every word of a 64-word span and check every word, so a short or shifted copy is caught. Before, only the first word was checked.
  • crates/tokenizer: for gemma4 (Metaspace spelling), the encoder respells a fragment's spaces before it decodes the fragment. An overlong space (C0 A0) now stays a space and falls back to the <0x20> byte token. Before, it was decoded first and came out as a ▁b token. gemma4_encodes_as_the_reference gets the a\xC0\xA0b case.
  • crates/placement/src/placement/nodes_scale.rs: n1_is_today checks that doubling the NVMe read rate on two DGX Sparks shortens step_ns by exactly 2*row, with owned and the window unchanged.
  • crates/gpu-gates/src/bin/shared/qwen3moe_place.rs: a new test, a_dense_header_plans_through_qwen35_on, writes a dense qwen35 header through the existing gguf::write::{Layout, Writer} (four layers, three GDN and one GQA, each with a SwiGLU FFN of 256 and no expert key). It checks that qwen35_on counts the whole load's ubatch arena and that the plan comes out of the load context. generate_qwen3moe.rs registers it.
  • crates/gpu-gates/src/bin/probe_nvread.rs: the eviction step sits behind a small Evict trait that SplitTable implements, and drop_parts is a free function. A mock with a kept edge folio backs the new test a_retained_edge_folio_leaves_with_its_shard.
  • crates/hf/src/vocab.rs: keys reads the vocabulary and the special ids in one stage each (vocabulary, special_ids). Behavior is unchanged, so this item has no failing counterpart.
  • crates/gpu-gemma-embedding2/src/pass.rs: before the second encode, the test fills the reused readback rows with NaN, so a readback that skips its copy is caught on the first output value.
  • crates/gpu/src/tensor.rs and crates/gpu-gates/src/bin/h2d_probe.rs: h2d_probe --arm readback times to_host_vec into a fresh destination against bloomery_gpu::to_host into a host-written one. The host_filled doc points at it. Each reading is timed after the device buffer is zero-filled and the stream has synchronized, so the fill does not land in the first fresh read. The result lines are Records of a new h2d_readback kind, and h2d_probe now starts through record::at_main, so h2d_probe --records-schema works. The arm refuses --reps below 5 by name.

Code shape

  • No second copy. The dense header test uses the gguf::write writer that the neighboring tests already use. The readback timing is one arm of h2d_probe, and the respelling lives in the encoder only.
  • Model code holds only architecture facts. The one model-code change moves an existing step earlier in the gemma4 path. No common owner gained a per-model branch.
  • Dead code is gone. The per-word respelling inside the encoder loop is removed with the move.
  • No silent failure. h2d_probe --arm readback refuses a rep count below 5. The dense header test panics by stage name.
  • Comments state what is true now.
  • No relaxed gate. No band, tolerance or lint moved.
  • cargo fmt is applied to the Rust files I changed. cargo clippy ran only on bloomery-hf; the other crates were not linted.

Duplication found or removed (path:line), or "none": none.

What I ran

Each new test runs clean on its head and trips when the defect it targets is put back. In the lines below, head is the test on the branch head and mutant is the same test with that defect applied. The hf item is a behavior-preserving refactor, so it has only the head run. The runs in the first four blocks were taken on an earlier head of this branch, before it was rebased onto current main and before the h2d_probe follow-up described below. I did not rerun them on this head.

Checks that run on the CPU (macOS):

refset head rc=0
refset mutant rc=101
tokenizer head rc=0
tokenizer mutant rc=101
placement head rc=0
placement mutant rc=101
check-arch head rc=0
check-arch mutant rc=1
labels head rc=0
labels mutant rc=1
hf head rc=0

check-arch and probe_nvread also ran on a second CPU machine:

check-arch head rc=0
check-arch mutant rc=1
probe_nvread head rc=0
probe_nvread mutant rc=101

Checks that need the GPU, on a DGX Spark. For the three tests below, the old version of the test still runs clean with the defect applied (old-test+mutant rc=0), which is the gap the new tests close:

pump head rc=0
pump mutant rc=101
pump old-test+mutant rc=0
pass head rc=0
pass mutant rc=101
pass old-test+mutant rc=0
qwen35 head rc=0
qwen35 mutant rc=101
qwen35 old-test+mutant rc=0
qwen3-plan all rc=0

A later fix round changed h2d_probe and record.rs only: the zero-fill finishes before timing, and the result line is a Record. On the head before the final rebase, on the same DGX Spark, a mutant of h2d_probe without its record::at_main startup builds but exits 1 on --records-schema. The head with the startup restored exits 0, and its readback arm prints:

h2d build mutant rc=0
h2d schema mutant rc=1
h2d build head rc=0
h2d schema head rc=0
h2d arm=readback card="NVIDIA GB10" bdf=000f:01:00.0 dest=fresh bytes=33554432 calls=8 first_ms=15.4662 median_ms=14.2391
h2d arm=readback card="NVIDIA GB10" bdf=000f:01:00.0 dest=filled bytes=33554432 calls=8 first_ms=2.4207 median_ms=2.5374
h2d readback head rc=0

The two record-list tests (kinds_are_well_formed, reads_every_kind_whole) also ran on that head:

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 137 filtered out; finished in 0.00s

The refusal of --reps below 5 was checked on an earlier head:

h2d_probe: arm readback: --reps 2, at least 5
h2d bad reps rc=1

just gate-gpu-p1 and the other just gate-* recipes: not run on this head: the GPU checks above call the changed crates' tests and the probe directly.

cargo clippy -p bloomery-hf --all-targets -D warnings passed after special_ids returned a type alias. Not run: the full workspace check and clippy on the other crates. rustfmt --edition 2024 --check on h2d_probe.rs and record.rs passed.

What I ran into

This does not block the PR. The h2d_probe readback numbers come from single runs on one machine. They show that the probe separates the two destination kinds, and this PR makes no speed claim. One earlier run on this head, taken right after a test build finished in the same container, printed a fresh median of 275.5622 ms against the 14.2391 ms above. I read it as transient noise and did not repeat it. The zero-fill synchronization cannot be pinned by a CPU test, because it moves setup out of the timer and changes no device arithmetic.

Hardware (if the change is for a machine)

One DGX Spark (GB10). It ran the GPU checks above and nothing else. The readback lines above come from the nvidia-smi lines below.

NVIDIA GB10, 595.84
| NVIDIA-SMI 595.84                 Driver Version: 595.84         CUDA Version: 13.3     |

Release notes

Release notes: Fixed: gemma4 text with an overlong space (C0 A0) now keeps the space; new tests close eleven open review follow-ups.

AI help

AI 코딩 에이전트가 코드를 작성한 후 설명을 썼고, 테스트를 마쳤습니다. diff 리뷰와 PR 초안 리뷰는 제가 직접 했습니다.

@github-actions github-actions Bot added gpu The card engine: crates/gpu*, crates/app host Host crates: qdot, threads, engram, engram-lab, runtime, sampler, levers, decision, vision, refset model The CPU engine and model files: crates/model, models, hf, gguf, tokenizer, jinja needs-gpu Changes code a card must check, and carries no gate run on a card placement Placement planning: crates/placement tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo labels Oct 24, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CLEAN: No must-fix behavior or quality defect survived the checks.

Checked

Duplicates: 22 new items and 12 trait method declarations/implementations checked; no duplicated owner found. Dryer reported zero pairs; its excluded gpu-gates files were searched manually.
Complexity: vocabulary (47) extracts existing validation; its stages are vocabulary ids, added tokens and merges. run (14 → 16) adds readback dispatch, with no new orchestration concern.
Silent failure: five fallback sites followed. Omitted arguments and absent optional JSON files use defined defaults. Invalid added-token decoder ids return Error::Vocab.
Tests executed in memory: seven label cases passed; restoring the old summary at tools/review/labels.py:262 fails. Two awk fixtures passed; making the key's dot a wildcard at tools/check-arch.sh:104 fails.
Rust mutation traces, checked statically: accepting multiple draft layers at crates/refset/src/mtpref.rs:323 fails the [47, 48] assertion; shortening the relay span at crates/gpu/src/host/link/pump.rs:365 fails the word checks; restoring late space respelling at crates/tokenizer/src/encoder.rs:95 fails the overlong-space case; omitting NVMe fill at crates/placement/src/placement/nodes.rs:1532 fails the two-node delta; using the placed arena at crates/gpu-gates/src/bin/shared/qwen3moe_place.rs:368 makes the dense-header test fail; dropping shard eviction at crates/gpu-gates/src/bin/probe_nvread.rs:467 leaves one resident page; skipping readback at crates/gpu-gemma-embedding2/src/pass.rs:616 leaves NaN and fails the test. Record-list tests cover the new kind's validation and readback.
Gates: no pins, tolerances or existing cases removed or relaxed.
Escape catalog: no C1–C4 match. New shell captures preserve failure status.

Notes

Reported Rust runs predate the final rebase. No just gate-* output or measured commit pair is attached, so GPU evidence is incomplete. Rust and GPU tests were not run during this review.

Card check: needs-gpu. This pull request changes crates/gpu-gates/src/bin/generate_qwen3moe.rs, crates/gpu-gates/src/bin/h2d_probe.rs, crates/gpu-gates/src/bin/probe_nvread.rs … and carries no gate run on a card (CONTRIBUTING's evidence list); a maintainer adds gpu-verified after one.

Not approved: this pull request changes tools/review/labels.py, which changes the bot, CI or the toolchain and needs a person's approval.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

@juliankang4

Copy link
Copy Markdown
Collaborator Author

Superseded by #790.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gpu The card engine: crates/gpu*, crates/app host Host crates: qdot, threads, engram, engram-lab, runtime, sampler, levers, decision, vision, refset model The CPU engine and model files: crates/model, models, hf, gguf, tokenizer, jinja needs-gpu Changes code a card must check, and carries no gate run on a card placement Placement planning: crates/placement tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant