fix(qwen3): preserve explicit stop-token causes - #978
RicardoMin wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4d324ec8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
FeathBow
left a comment
There was a problem hiding this comment.
Thanks. Please first rebase this PR onto the current main and handle CI error :)
cb10da3 to
3879253
Compare
|
Hi @FeathBow, I have rebased this PR onto the latest |
FeathBow
left a comment
There was a problem hiding this comment.
Thanks for separating the Qwen3 stop contract and for preserving the trigger token, logprob, completion count, EOS priority, and legacy fallback.
First, a successful hedged DFlash verify returns before terminal truncation. It selects and copies a winner, advances DFlash hidden context, updates acceptance accounting, and ticks the hedge controller from the untruncated span. The later executor truncation protects the final returned/KV length, but cannot undo those worker-side decisions. Apply each request's stop policy to every A/B candidate before winner comparison and add a hedge + mid-span terminal gate.
Second, nonzero min_tokens is silently accepted even though the new policy starts EOS/explicit-stop classification at token one. Restore the fail-early rejection until sampler-side masking exists; one assertion in the shared validator test is sufficient because both bridges invoke that validator before submission.
Please also revert the frontend architecture text that describes nonexistent ActiveRequest/StepEmitter code. Finally, normalize and share large stop sets: the current Vec::contains is linear in a per-token path and the full vector is deep-copied three times per speculative step. A shared sorted slice with binary search removes the request-size-linear scan and bulk copies; verify both the common zero/one-ID and full-vocabulary shapes before choosing anything more elaborate.
Keep the patch narrow: remove the unconstructed EosPolicy::Token branch, collapse the two identical resolver wrappers, and replace the one-off fake speculative executor/test with the missing production hedge gate. The existing bridge mapping and prefill/decode logprob tests cover distinct local contracts and should remain.
|
All commit attributions previously flagged on this pull request are resolved. |
1247f39 to
50f0e16
Compare
SummaryThis update narrows the stop-contract change to Qwen3 while retaining the legacy frontend path for models that have not migrated yet. The previous implementation could classify a terminal token after speculative candidate selection and KV-state preparation. When a stop token appeared in the middle of a verify span, the accepted suffix could therefore remain visible in the candidate state or be copied back before truncation. This update applies terminal truncation before hedge winner selection and before speculative KV/state commit. Stop Contract
The contract preserves the triggering token, its logprob, and completion-token accounting. Review Fixes
Only Qwen3 consumes the typed stop policy in this change. The legacy frontend conversion path remains compatible with other models. Verification
The change does not modify model mathematics, attention kernels, sampling kernels, or CUDA Graph shapes. |
|
Hi @FeathBow, your requested changes have been implemented in the latest commits. Please take another look when you get a chance. Thank you for the detailed review! |
c2e801e to
fcc8b9e
Compare
xiaguan
left a comment
There was a problem hiding this comment.
Thank you for the careful revision. The production ordering is much closer to the required contract, and the min_tokens, stop-set lookup, and documentation changes address several parts of the previous review. I still do not think the new hedge test protects the worker-side bug, and there is one invariant leak in the new public type.
-
Please make the hedge regression fail against the previous implementation.
dflash_hedged_midspan_stop_retains_triggeronly asserts the final emitted tokens andTerminal. Before the latest worker-side fix,execute_speculative_verify_implalready calledtruncate_after_terminalafterrun_stepand beforeRequestKv::apply_speculative, so those assertions still pass even if the worker ranks A/B candidates, copies KV/hidden state, and records DFlash context from the untruncated result.The test also derives its stop ID from a baseline run on the same hedged engine. Because the stop policy does not affect sampling, the old worker reproduces the same candidates and winner; the later executor truncation then produces exactly the expected external prefix. The parent gate requires each child to execute a hedge span, but
total_wins > 0is aggregated across all three children. The stop child is not required to have a B win, and it never demonstrates a case where terminal truncation changes the A/B ordering.Please add a real hedge case that observes the request-local raw and retained candidate lengths and the selected winner, with a fixture where truncation changes the winner decision. It should also verify that the retained winner is what feeds the KV/hidden context and committed/controller accounting. As a practical mutation check, this gate must fail when the new pre-selection truncation in
try_execute_hedged_verifyis reverted while the executor-side safety truncation remains. -
Please keep the sorted stop-set invariant inside
StopPolicy.StopPolicy::classifyrelies onbinary_search, while botheosandtoken_idsare public. Any caller can therefore constructStopPolicy { token_ids: Arc::from([7, 3]), ... }and get a silently incorrect classification, bypassing the normalization promised by the type. The production callers only neednew,default, andclassify; please make the fields private and add a narrow read-only accessor only if a real caller needs one. -
Please reduce the low-value tests and refresh the comments to match the final flow.
normalizes_stop_sets_across_common_sizesrepeats the one-ID behavior already covered above it and allocates an entire 151,936-ID set while primarily retestingsort_unstable,dedup, andbinary_search. It does not prove that per-step clones share the allocation or that the production wire conversion preserves the contract. A focused dedup/membership test plus the meaningful wire/hedge gates is enough.Several comments now overstate or contradict the implementation: the hedge test claims it proves pre-winner selection and commit ordering although it only checks the final stream; the parent gate still says it runs “two” losslessness tests and “one child per lossless suite” after adding a third, non-losslessness child; and
execute_speculative_verify_implsays the worker returns the mathematically accepted span even though the worker now applies the stop policy and the executor only rechecks the invariant. Please remove or update these while narrowing the tests.
The main implementation direction looks reasonable, but the current regression can stay green with the original worker-side bug restored, so I cannot approve this head yet. Thank you.
fcc8b9e to
da4efc6
Compare
SummaryThis update addresses the remaining review feedback for the Qwen3 stop-contract migration. Review Fixes
Verification
The strict hedge test now fails if terminal truncation is moved after winner selection, even when the final output is later corrected by the legacy safety truncation. |
|
Hi! @xiaguan ,The PR description has been updated with the full verification details. Please take another look when you have a moment. Thank you! |
FeathBow
left a comment
There was a problem hiding this comment.
Thanks. Two things I would still fix before merge. First, the parent ladder now parses the hedge win count and discards it: total_wins > 0 and total_spans > total_wins are gone, and the stop child's qualifying case is raw_winner=B with selected=A, which is exactly the case where copy-back does not run. So no test proves the copy-back branch executes any more; please restore both assertions for the two losslessness children. Second, the trace that makes the new gate mutation-sensitive is larger than the two facts it needs (raw B win flipped to A with retained < raw, and selected_len == commit_len): retained_winner equals selected by construction and context_len equals selected_len by construction, and the hedged flag on VerifyResult and record_verify_dflash_context exists only to pair log lines. btw please remove outdated docs and descriptions.
| total_spans > total_wins, | ||
| "every hedge span won ({total_wins}/{total_spans}) — the discard path never executed" | ||
| ); | ||
| assert!(total_rounds > 0 && total_spans > 0); |
There was a problem hiding this comment.
The previous gate asserted total_wins > 0 (the page copy-back / hidden compaction branch executed at least once) and total_spans > total_wins (the discard branch executed too). This head parses the win count and drops it, replacing both with total_spans > 0. The stop child cannot supply that coverage: its qualifying trace is raw_winner=B with selected=A, which is precisely the case where copy-back does not run. So after this change no test shows the hedge copy-back branch ever executes. I recommend restoring both assertions over the two losslessness children (per child or aggregated as before) and leaving the stop child to its own worker-trace check.
| results_b.len(), | ||
| hedge_spans.len() | ||
| ); | ||
| let trace = std::env::var_os("PEGAINFER_TEST_LOG").is_some(); |
There was a problem hiding this comment.
I recommend keeping raw A/B lengths (captured before truncation) plus selected_is_b, dropping the retained_* / best recomputation, and dropping the hedged field on VerifyResult and the hedged parameter on record_verify_dflash_context: if the commit log line is emitted for every verify round (not only hedged ones) the parent can pair detail and commit lines per request by order without the flag, and the context line is not needed.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Sorry, I do not have my computer on hand at the moment. I will resolve the CL problem tomorrow. |
FeathBow
left a comment
There was a problem hiding this comment.
Thank you for addressing the earlier worker-ordering, stop-set ownership, and win/discard coverage feedback. The production path now applies terminal truncation before hedge selection and context recording.
I found four remaining issues. The strict hedge gate still requires an appended= field that is no longer emitted, so it cannot pass when executed. The wire policy also treats secondary model EOS IDs as primary EOS, losing the stop reason required by the pinned vLLM contract. In addition, the HTTP probe accepts several invalid outcomes, and the detail/commit pairing can compare different verify rounds.
Please address these issues and refresh the validation evidence for the resulting head. Handle redundant compatibility and test/comment descriptions.
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
0a25c14 to
eafc232
Compare
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
eafc232 to
8def997
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
SummaryThis head resolves all four findings from FeathBow's Sep 8 review and refreshes Review Fixes
VerificationEnvironment: RTX 4080 SUPER 32 GB, driver 580.142 / CUDA 12.8, Rust
This PR remains scoped to Qwen3. Other model lines and their legacy scheduler |
|
Thanks for the updates! There are still a couple of gaps in the validation:
There is also some duplication we can remove: merge the two |
…probe Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
SummaryThis update completes the final review round for the Qwen3 typed stop contract. Review fixes
What the probe verifiesAgainst each OpenAI-compatible server it drives 7
Running itSelf-check (no GPU, stdlib only): python3 scripts/qwen3_stop_contract_probe.py --self-checkStart the adapted Qwen3 server: cargo run --release -p pegainfer-server -- \
--model-path "$QWEN3_MODEL" \
--served-model-name qwen3-adapted \
--port 18081Start the legacy Qwen3.5 server (its Triton AOT kernels need a Python with export PEGAINFER_TRITON_PYTHON=/path/to/python-with-triton
cargo run --release -p pegainfer-server --features qwen35 -- \
--model-path "$QWEN35_MODEL" \
--served-model-name qwen35-legacy \
--port 18082Then run the A/B probe: python3 scripts/qwen3_stop_contract_probe.py \
--qwen3-url http://127.0.0.1:18081 --qwen3-model qwen3-adapted \
--qwen35-url http://127.0.0.1:18082 --qwen35-model qwen35-legacy \
--require-legacy-gap --out stop-contract-ab.jsonValidation
|
|
Thanks for the updates! The hedge test now rejects the injected token after a stop, and the policy-array and collection-loop cleanup looks good. I rechecked
These were deliberately constructed local HTTP responses, not errors observed from the inference server. Could you request the returned token IDs and check the exact trigger, its corresponding logprob, and the complete token sequence? The parser should also account for or reject content in a terminal frame instead of silently skipping it. One other point: removing only the executor-side second truncation loop, with worker truncation intact, still passed the real hedge gate locally. That differs from the reported mutation result. Could you clarify what that second pass needs to protect and update the validation note? Both worker paths already normalize the span before recording context; if a guard is needed at commit, an explicit invariant check would make a broken worker result visible instead of silently truncating it again. |
Signed-off-by: RicardoMin <17879681016@163.com>
Signed-off-by: RicardoMin <17879681016@163.com>
SummaryThis update strengthens the committed HTTP probe's assertions and consolidates the remaining stop-policy validation loops. The production scope remains Qwen3; the legacy compatibility bridge and Qwen3.5 scheduler are unchanged. Review fixes
Worker candidate normalization now uses one traversal with each candidate's own policy. Request-ID and commit-invariant validation also share one traversal, retaining rollback before any KV commit. The existing hedge baseline now checks Correction to the previous mutation resultMy previous statement that removing only the executor-side second truncation must fail the real hedge gate was incorrect. Workers already normalize candidates before winner selection, KV/hidden copying, and DFlash context recording. The executor's invariant check exposes an invalid worker result before KV commit; it does not replace worker normalization. I repeated isolated mutations, preserving the exact diff for each experiment and restoring the original source bytes between runs:
The gate retains its real hedge win/discard assertions and same-round worker/context/commit checks. ValidationValidation used the CUDA 12.8 / SM89 instance with Qwen3-4B,
The single-ID test returned Qwen3.5 passed model identity, baseline generation, and mixed ordinary-generation controls. Under the full-vocabulary stop set it continued to The constructed malformed responses validate the probe's assertions; they are not errors observed from the inference server. Running the probeSelf-check, using only the Python standard library: python3 scripts/qwen3_stop_contract_probe.py --self-checkWith the Qwen3 and Qwen3.5 servers running: python3 scripts/qwen3_stop_contract_probe.py \
--qwen3-url http://127.0.0.1:18081 \
--qwen3-model qwen3-adapted \
--qwen3-eos-token-id 151645 \
--qwen35-url http://127.0.0.1:18082 \
--qwen35-model qwen35-legacy \
--qwen35-eos-token-id 248046 \
--require-legacy-gap \
--out stop-contract-ab.jsonLive validation requires each server's actual primary EOS ID. For these checkpoints, the pinned vLLM backend resolves primary EOS from the tokenizer; Qwen3.5 uses |
|
Hi @xiaguan , |
Related to #865
Qwen3: preserve explicit stop-token causes in the stepped contract
Why this is a separate PR
This is the scoped follow-up requested by the maintainers during review of
#865. They asked
that the broad stop-contract change be split so that the shared boundary,
frontend bridges, and one model can be reviewed and validated independently.
This PR therefore extracts the Qwen3 migration from that work; Qwen3.5 and the
other model schedulers remain on their existing contract for now.
Summary
The previous vLLM dependency update fixed several frontend compatibility issues,
but the stepped Qwen3 path still collapsed two independent controls into the
single legacy
ignore_eosflag. That made an explicitstop_token_idsrequestindistinguishable from a model-EOS request and forced the bridge to guess a
synthetic stop token after the scheduler had already discarded the real one.
This PR gives Qwen3 a typed stop contract. It preserves the sampled trigger
token and its logprob, carries the concrete
StopCausethrough the schedulerand stepped bridge, and keeps EOS handling independent from request-provided
stop IDs.
What was wrong
The old contract exposed only
FinishReason::StoporFinishReason::Length.When a request stopped, the bridge could not tell whether the model emitted EOS
or an explicit request stop token. It therefore reconstructed a sentinel (EOS
first, otherwise the first configured stop ID). That reconstruction can report
the wrong token, loses the token's logprob, and is incorrect for a speculative
span where the first terminal token is followed by additional accepted tokens.
The old boolean also could not express the valid combination "ignore model EOS,
but still stop on these explicit request token IDs".
Contract change
FinishReason::Stop; trigger may be suppressed or reconstructedFinishReason::Stop+StopCause::Eos(id); token is retained internally; wirestop_reasonis absentstop_token_idsmatchFinishReason::Stop+StopCause::Token(id); actual ID is reported as wirestop_reasonignore_eos=trueFinishReason::LengthFinishReason::Length+ no stop cause; final sampled token is retainedEOS has precedence when the same ID is both the active EOS token and an explicit
request stop. Completion-token accounting is incremented once, including the
trigger token.
Scope and compatibility
Following the maintainer's scope request on #865, this PR intentionally
migrates Qwen3 only. The shared request/step types
accept an optional typed cause, while the existing legacy event path remains
available for models that have not been audited. The legacy bridge keeps its
synthetic-sentinel fallback only when an old producer supplies no typed cause.
Therefore Qwen3.5 and other model schedulers are not changed in this PR and do
not need to adopt the new resolver contract yet. If the maintainers agree with
the semantics, the remaining model lines can be migrated one at a time with
their own lifecycle tests.
Implementation
StopPolicy,EosPolicy, andStopCauseat the frontend engineboundary.
boolean for the stepped path.
ordinary decode, and speculative verification.
StopCause::Token(id)to the vLLM-compatible wirestop_reason.shared default field; no legacy model runtime behavior is changed.
Automated verification
cargo test --release -p pegainfer-frontend --libcargo test --release -p pegainfer-qwen3 --libcargo test --release -p pegainfer-sim --tests -- --test-threads=1cargo check --release -p pegainfer-qwen35 --features qwen35cargo build --release -p pegainfer-server --bin pegainfercargo fmt --all -- --checkgit diff --checkHTTP A/B verification
The comparison used two already-running OpenAI-compatible endpoints on the
same validation host. The explicit stop set covered the complete vocabulary,
so the first generated token was guaranteed to exercise the request-stop path.
This is a deterministic contract probe, not a generation-quality benchmark.
Results are shown as
finish_reason / stop_reason / completion_tokens:ignore_eos=true)length / null / 8stop / 12095 / 1stop / 12095 / 1length / null / 8length / null / 8length / null / 8The Qwen3.5 rows are an intentional legacy comparison: ordinary generation
still works, but its un-migrated scheduler does not yet satisfy the new typed
explicit-stop contract. They are not a claim that every legacy model fails in
all workloads.
Reproduction
Build and start each server independently. Qwen3.5 requires its feature-gated
Triton build environment; it does not support or require a
--gpu-memory-utilizationCLI argument.Then run the attached script (Python standard library only):
The script prints a compact comparison table and writes machine-readable JSON.
Use
--stop-token-id IDto replace the full-vocabulary deterministic set witha single known token when reproducing on a different prompt/model pair.
Follow-up
As requested during review of #865, this PR deliberately stops at the Qwen3
migration boundary. After the
maintainers confirm that the independent EOS/request-stop semantics are wanted,
the same policy propagation and resolver audit can be applied to Qwen3.5 and the
other legacy model lines in separate, model-scoped changes.
pr865_qwen3_stop_contract_ab.py