Repository navigation
tests, tokenizer and probe: close ten open review follow-ups - #790
Conversation
…fragment, so an overlong space stays a space
… missed copy fails
There was a problem hiding this comment.
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)
|
The review is done and the contribution check passed, so I'm merging this. |
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. Theblk_archscancomment intools/check-recipes.shnow 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▁btoken.gemma4_encodes_as_the_referencegets thea\xC0\xA0bcase.crates/placement/src/placement/nodes_scale.rs:n1_is_todayalso plans two DGX Sparks and checks that doubling the NVMe read rate shortensstep_nsby exactly2 * row, with the owned ranges and node windows unchanged.crates/gpu-gates/src/bin/shared/qwen3moe_place.rs: the new testa_dense_header_plans_through_qwen35_onwrites a dense qwen35 header with the existinggguf::writewriter (four layers, three gated-delta and one gated-attention, each with a SwiGLU FFN of 256 and no expert key). It checks thatqwen35_oncounts the whole load's ubatch arena and that the plan answers at the load's context.generate_qwen3moe.rsregisters the test.crates/gpu-gates/src/bin/probe_nvread.rs: the eviction step sits behind a smallEvicttrait thatSplitTableimplements, anddrop_partsis a free function. A stack whose row eviction keeps one edge page backs the new testa_retained_edge_folio_leaves_with_its_shard.crates/hf/src/vocab.rs:keysreads 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.rsandcrates/gpu-gates/src/bin/h2d_probe.rs:h2d_probe --arm readbacktimesto_host_vecinto a fresh destination againstbloomery_gpu::to_hostinto a host-written one, and thehost_filleddoc 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 areRecords of a newh2d_readbackkind, andh2d_probestarts throughrecord::at_main, soh2d_probe --records-schemaworks.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
gguf::writewriter the neighboring tests already use. The readback timing is one arm ofh2d_probe, and the respelling lives in the encoder only.stream.synchronize()failure in the readback arm propagates.cargo fmtis applied to the twogpu-gatesfiles that gained theRecordcode, andcargo clippyran onbloomery-hfonly. 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_probeandrecord.rschange, on one DGX Spark (GB10):The two record-list tests that cover the new kinds:
A copy of
h2d_probewithout therecord::at_mainstartup builds but refuses--records-schema. With the startup in place it exits 0 and the readback arm prints one line per destination kind:just gate-gpu-p1and the otherjust 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 warningspassed afterspecial_idsreturned a type alias.rustfmt --edition 2024 --checkpassed onh2d_probe.rsandrecord.rs.Not run on this head: the new and changed tests in the tokenizer, placement, refset, link relay, gemma embedding, dense plan and
probe_nvreaditems, and thecheck-archself-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_probereadback 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 초안 리뷰는 제가 직접 했습니다.