Repository navigation
tests, tokenizer and probe: close eleven open review follow-ups - #788
juliankang4 wants to merge 11 commits into
Conversation
…fragment, so an overlong space stays a space
… missed copy fails
There was a problem hiding this comment.
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)
|
Superseded by #790. |
이 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 anddeepseek2.expert_countis. Theblk_archscancomment intools/check-recipes.shnow 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▁btoken.gemma4_encodes_as_the_referencegets thea\xC0\xA0bcase.crates/placement/src/placement/nodes_scale.rs:n1_is_todaychecks that doubling the NVMe read rate on two DGX Sparks shortensstep_nsby exactly2*row, withownedand 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 existinggguf::write::{Layout, Writer}(four layers, three GDN and one GQA, 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 comes out of the load context.generate_qwen3moe.rsregisters it.crates/gpu-gates/src/bin/probe_nvread.rs: the eviction step sits behind a smallEvicttrait thatSplitTableimplements, anddrop_partsis a free function. A mock with a kept edge folio 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 is caught on 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. Thehost_filleddoc 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 areRecords of a newh2d_readbackkind, andh2d_probenow starts throughrecord::at_main, soh2d_probe --records-schemaworks. The arm refuses--repsbelow 5 by name.Code shape
gguf::writewriter that the neighboring tests already use. The readback timing is one arm ofh2d_probe, and the respelling lives in the encoder only.h2d_probe --arm readbackrefuses a rep count below 5. The dense header test panics by stage name.cargo fmtis applied to the Rust files I changed.cargo clippyran only onbloomery-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,
headis the test on the branch head andmutantis the same test with that defect applied. Thehfitem 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 theh2d_probefollow-up described below. I did not rerun them on this head.Checks that run on the CPU (macOS):
check-archandprobe_nvreadalso ran on a second CPU machine: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:A later fix round changed
h2d_probeandrecord.rsonly: the zero-fill finishes before timing, and the result line is aRecord. On the head before the final rebase, on the same DGX Spark, a mutant ofh2d_probewithout itsrecord::at_mainstartup builds but exits 1 on--records-schema. The head with the startup restored exits 0, and its readback arm prints:The two record-list tests (
kinds_are_well_formed,reads_every_kind_whole) also ran on that head:The refusal of
--repsbelow 5 was checked on an earlier head:just gate-gpu-p1and the otherjust 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 warningspassed afterspecial_idsreturned a type alias. Not run: the full workspace check and clippy on the other crates.rustfmt --edition 2024 --checkonh2d_probe.rsandrecord.rspassed.What I ran into
This does not block the PR. The
h2d_probereadback 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-smilines below.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 초안 리뷰는 제가 직접 했습니다.