Skip to content

tests, tokenizer and probe: close ten open review follow-ups - #790

Merged
juliankang4 merged 10 commits into
midagedev:mainfrom
juliankang4:dgx-spark-close-the-11-non
Oct 24, 2026
Merged

juliankang4 merged 10 commits into
midagedev:mainfrom
juliankang4:dgx-spark-close-the-11-non

Conversation

@juliankang4

Copy link
Copy Markdown
Collaborator

Supersedes #788. 이 PR은 리뷰에서 남은 비차단 지적 열 건을 닫습니다. 테스트가 약한 곳을 보강하고, gemma4 토크나이저가 너무 긴 공백을 잘못 처리하던 순서를 고치고, 카드에서 호스트로 읽는 속도를 재는 probe 모드를 하나 더합니다.

What and why

Ten non-blocking follow-ups from reviews of #148, #172, #173, #241, #333, #339, #356, #540 and #755 were still open. This PR closes all of them. Seven tighten or add a test that catches the defect the review described. One splits a long function into stages, one changes tokenizer behavior, and one adds a timing arm to an existing probe.

  • crates/refset/src/mtpref/tests.rs: the draft-layer test takes a slice of layers and checks that [47, 48], a set naming two draft layers, is refused as malformed.
  • tools/check-arch.sh: the self-test runs rule 2's literal scan on fixed lines. "blk.", "deepseek2x" is clean and "blk.", "deepseek2.expert_count" is a hit. The blk_archscan comment in tools/check-recipes.sh now says so.
  • crates/gpu/src/host/link/pump.rs: the link relay tests fill every word of a 64-byte 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 now respells a fragment's spaces before it decodes the fragment. An overlong space (C0 A0) 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 also plans two DGX Sparks and checks that doubling the NVMe read rate shortens step_ns by exactly 2 * row, with the owned ranges and node windows unchanged.
  • crates/gpu-gates/src/bin/shared/qwen3moe_place.rs: the new test a_dense_header_plans_through_qwen35_on writes a dense qwen35 header with the existing gguf::write writer (four layers, three gated-delta and one gated-attention, 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 answers at the load's context. generate_qwen3moe.rs registers the test.
  • 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 stack whose row eviction keeps one edge page 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 shows NaN at 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, and the host_filled doc points at it. The device buffer is zero-filled and the stream synchronized before the first timed read, 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 starts through record::at_main, so h2d_probe --records-schema works.

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

Code shape

  • No second copy. The dense header test uses the gguf::write writer 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. The dense header test panics by stage name, and a stream.synchronize() failure in the readback arm propagates.
  • Comments state what is true now.
  • No relaxed gate. No band, tolerance or lint moved.
  • cargo fmt is applied to the two gpu-gates files that gained the Record code, and cargo clippy ran on bloomery-hf only. The other changed crates were not linted here.

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

What I ran

GPU evidence for the h2d_probe and record.rs change, on one DGX Spark (GB10):

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

The two record-list tests that cover the new kinds:

cargo test --release -p bloomery-gpu-gates --features hf --lib -- record::tests::kinds_are_well_formed record::tests::reads_every_kind_whole
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 137 filtered out; finished in 0.00s

A copy of h2d_probe without the record::at_main startup builds but refuses --records-schema. With the startup in place it exits 0 and the readback arm prints one line per destination kind:

h2d build mutant rc=0
h2d_probe: unknown argument --records-schema
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

just gate-gpu-p1 and the other just gate-* recipes: not run on this head: the GPU run above calls the probe and the record tests directly, and a maintainer runs the gate recipes.

cargo clippy -p bloomery-hf --all-targets -D warnings passed after special_ids returned a type alias. rustfmt --edition 2024 --check passed on h2d_probe.rs and record.rs.

Not run on this head: the new and changed tests in the tokenizer, placement, refset, link relay, gemma embedding, dense plan and probe_nvread items, and the check-arch self-test. They are in the diff for CI and the maintainers' checks to exercise.

What I ran into

This does not block the PR. The h2d_probe readback numbers come from single runs on one machine. They show the probe tells the two destination kinds apart, and the PR makes no speed claim. A separate run, 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 took it as transient noise and did not repeat it. A CPU test cannot pin the zero-fill synchronization, because it only moves setup out of the timer and changes no device arithmetic.

Hardware (if the change is for a machine)

One DGX Spark (GB10), driver 595.84, CUDA 13.3. It ran only the GPU checks listed above.

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.

Since the last review: there were no open must-fix or non-blocking items. Ten patches are unchanged. The label-summary commit was dropped.

Verdict: CLEAN: Dropping the summary change preserves label updates and removal guards.

Checked

Duplicates: 0 new items in the interdiff. Dryer reported 0 pairs; its excluded gpu-gates files are unchanged since the previous review.
Complexity: vocabulary (47, validation extraction) and h2d_probe::run (14 → 16, another dispatch arm) are unchanged patches. The interdiff adds no complexity.
Silent failure: 0 new default or ignored-error sites. Merged PRs and incomplete file lists still suppress label removal.
Tests: static inspection confirms seven label-state cases retain their label assertions, and the dry-run case permits only GET requests. Removing the merged guard at tools/review/labels.py:242 would fail the merged-label assertion. No surviving tests changed in the interdiff.
Gates: no base pin, tolerance or test case was removed or relaxed. The dropped commit's summary assertions were additions to the base.
Escape catalog: no C1–C4 pattern matched.

Notes

The reported GB10 run lacks both measured commit hashes and just gate-* commands with output, 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.

🔨 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

The review is done and the contribution check passed, so I'm merging this.

@juliankang4
juliankang4 merged commit 27776a2 into midagedev:main Oct 24, 2026
7 checks passed
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