[test] Add regression test for validate_deepseek_v4_cp hook ordering (#32553; fix landed via #33532) - #32557
Open
layahaasini wants to merge 1 commit into
Open
[test] Add regression test for validate_deepseek_v4_cp hook ordering (#32553; fix landed via #33532)#32557layahaasini wants to merge 1 commit into
layahaasini wants to merge 1 commit into
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
layahaasini
force-pushed
the
fix/deepseek-v4-prefill-cp
branch
from
July 27, 2026 17:47
f0f8089 to
bc9d79e
Compare
layahaasini
force-pushed
the
fix/deepseek-v4-prefill-cp
branch
from
August 10, 2026 20:24
bc9d79e to
53d9f11
Compare
layahaasini
changed the title
…2553) The early legacy-CP mirror in ServerArgs.__post_init__ runs before the model arch is known, so --enable-prefill-cp --cp-strategy interleave was mirrored to the generic legacy alias (enable_prefill_context_parallel). The DSV4 hook then set enable_dsa_prefill_context_parallel without clearing the generic one, the re-apply mirror bailed on its both-set guard, and _handle_context_parallelism raised its mutual-exclusion ValueError at startup. Clear the generic alias when the hook selects the DSA alias.
layahaasini
force-pushed
the
fix/deepseek-v4-prefill-cp
branch
from
August 12, 2026 01:11
53d9f11 to
70b7d00
Compare
Author
|
The fix landed via #33532, so this is now test-only: the regression test for the Rebased on main, test file unchanged from what was approved. I verified it passes against current main and fails if #33532's hook change is reverted. @mmangkad @Fridge003 could one of you please |
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
3 participants
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.
Motivation
Relevant to #32553 — the crash was fixed on main by #33532; this PR adds the regression coverage that fix landed without.
--enable-prefill-cp --cp-strategy interleavecrashed at startup on DeepSeek-V4-Flash. Root cause was ordering inServerArgs.__post_init__: the first_handle_legacy_cp_arguments()pass runs before model-specific defaults and mirrors the canonical flags to the generic legacy alias,validate_deepseek_v4_cpthen set the DSA alias without clearing the generic one, and_handle_context_parallelism()raised the mutual-exclusionValueError. This PR originally carried the same fix; after #33532 merged I rebased and dropped it, so the diff is now test-only.Modifications
test/registered/unit/server_args/test_server_args.py(+26/-0): addstest_canonical_cp_survives_deepseek_v4_hook_ordering, which replays the__post_init__handler ordering throughvalidate_deepseek_v4_cpand asserts the canonical CP flags survive. The existing CP tests cover the alias mirroring but never exercise the DSV4 hook path.Verified on the rebased head:
pytest test/registered/unit/server_args/test_server_args.py -k canonical_cp→ 1 passed against current main, and fails with the issue'sValueErrorif #33532's hook change is reverted.Checklist
CI States
Latest PR Test (Base): ❌ Run #31552763963
Latest PR Test (Extra): ❌ Run #31552763913