test(surgery): stop the empty-pipeline comparison at the first EOS#760
Merged
Merged
Conversation
empty_surgery_pipeline_matches_baseline_output ran the CLI twice over a fixed -n 20 token budget and compared the two stdouts byte-for-byte. mlxcel generate keeps sampling for the full -n count instead of stopping at EOS, so on qwen2.5-0.5b-4bit both runs already emit <|im_end|> and <|endoftext|> partway through, then continue into a free-running tail with no signal for the no-op property under test. On the CUDA/GB10 backend, near-tied logits in that tail let GPU reduction-order variance flip the argmax and make the full-window comparison flaky (issue #728); the same tail is reproducible locally on the Metal backend, just without observed nondeterminism there. Added EOS_MARKERS (<|im_end|>, <|endoftext|>, matching decode_generated_text's skip_special_tokens = false rendering) and truncate_at_first_eos, applied independently to the baseline and surgery outputs before the equality assertion. Truncating each run's own text at its own first marker bounds the comparison to the deterministic pre-EOS region while still failing loudly on any genuine pre-EOS divergence, and falls back to comparing the full window when no marker appears within the token budget. Validated on this machine's Metal backend: cargo test --release --test surgery_cli empty_surgery_pipeline_matches_baseline_output passed 5 consecutive times, and the full surgery_cli suite (3 tests) passed together. The GB10/CUDA 5-consecutive-run acceptance criterion from the issue still needs confirmation on CUDA hardware, which was not reachable from this session; the fix removes the flake mechanism (post-EOS near-tie argmax) platform-independently by construction, since it no longer depends on GPU reduction order at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
empty_surgery_pipeline_matches_baseline_outputcompared the baseline and empty-surgery-pipeline CLI outputs byte-for-byte over the full fixed-n 20token budget.mlxcel generatekeeps sampling for the whole-ncount instead of stopping at EOS, so onqwen2.5-0.5b-4bitboth runs already emit<|im_end|>and<|endoftext|>partway through and then continue into a free-running tail that carries no signal for the no-op property under test. On the CUDA/GB10 backend, near-tied logits in that tail let GPU reduction-order variance flip the argmax, making the full-window comparison flaky. This PR truncates the comparison at the first EOS marker instead.Root cause
Confirmed locally on this machine's Metal backend that the same post-EOS tail exists outside CUDA (
mlxcel generate -m models/qwen2.5-0.5b-4bit -p "Hello" -n 20 --temp 0.0prints...<|im_end|>\n<|endoftext|>Human: How can I improve my publicfor the last few tokens), matching the left/right sample in the issue. The property under test (an empty surgery pipeline must not alter output) is sound; only the comparison window was fragile, exactly as the issue diagnosed. Changing the CLI's own EOS-stop behavior is out of scope for this test-only fix.What changed
tests/surgery_cli.rs: addedEOS_MARKERS(<|im_end|>,<|endoftext|>— the literal stringsdecode_generated_textrenders since it decodes withskip_special_tokens = false) andtruncate_at_first_eos, which truncates a decoded run's text at the end of whichever marker starts earliest, or returns the full text unchanged when no marker is present.truncate_at_first_eosbefore theassert_eq!, so the assertion is bounded to the deterministic pre-EOS region.Test plan
cargo check --tests(metal, accelerate features)cargo build --release --features metal,acceleratecargo test --release --test surgery_cli empty_surgery_pipeline_matches_baseline_output -- --nocapture— 5 consecutive runs, all passed (test result: ok. 1 passed)cargo test --release --test surgery_cli -- --nocapture— full 3-test suite (includingmalformed_surgery_yaml_fails_fastandmissing_surgery_yaml_fails_fast) passed togethercargo fmt --all -- --checkpython3 scripts/ci/check_cross_repo_refs.pyAll validation above ran on this machine's Metal backend against the real
models/qwen2.5-0.5b-4bitcheckpoint. The issue's GB10/CUDA 5-consecutive-run acceptance criterion still needs confirmation on CUDA hardware, which was not reachable from this session. That said, the fix removes the flake mechanism (post-EOS near-tie argmax under GPU reduction-order variance) platform-independently by construction: the comparison no longer inspects the post-EOS region on any backend, so there is nothing left for reduction-order nondeterminism to disturb.Closes #728