Repository navigation
fix(runner): reconcile KV block counts across TP and PCP-group workers - #3171
Conversation
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
valar-cop
left a comment
There was a problem hiding this comment.
a few small things..
tp>1 or sp/pcp>1 run to show the startup collective completes and all ranks agree?
ci green.
|
ci triage for 6d7d49a:
|
e8f12e5 to
443b280
Compare
|
Reviewed Not repeating @valar-cop's two inline points — both are fixed in VerdictThe runner fix should go in. The CI half should be a separate PR. The bug is real and universal, not a corner case. I ran the new checker against 25 real, unmodified, pre-fix ATOM TP8 server logs I had locally: 25/25 flagged, every one with all 8 ranks disagreeing, spread 0.57%–1.51%. The CI half (+130 lines, 3 new What I verified by running it, not by reading
🔴 1 — the new collective turns one rank's clear error into a hang on all the othersIn the PR's Both depend on this rank's own Concrete trigger: TP8 where any rank's This convention is already written down twice in this same function: Note main does not have this hazard: with PP=1 there is no collective in Ask (either, first preferred):
|
|
@yitingw1 both my inline points are resolved: still no run in the test plan for tp>1 / sp>1. please paste the and the raise at |
|
@valar-cop on your review above, the remaining ask was a multi-rank startup run. ATOM Test 42943244497 on 443b280 started two of the three new entries, and every rank logged the same count. TP4 + PCP2, DeepSeek-V4-Pro (job): all 8 ranks built 86310 blocks. Their TP2, Meta-Llama-3-8B-Instruct (job): both ranks built 107703 blocks. gsm8k 0.7377 (bar 0.73). For comparison, a TP8 Qwen3-Next run on main before this change logged eight different counts, 2315143 to 2328796. SP4 did not start (job). The CI AITER wheel lacks ROCm/aiter#7346, which CI on 443b280: Pre Checkin, Actionlint, ATOM vLLM Test and ATOM SGLang Test passed; 16 of 17 accuracy jobs passed, and the failure is SP4 above. |
|
@MengqingCao already looked at 443b280, nothing new since my last review. say |
|
@yitingw1 on the CI half ( |
valar-cop
left a comment
There was a problem hiding this comment.
LGTM
ci Atomesh Accuracy Validation failed — not sure yet (low): No job log was provided under .valar/ci, so the cause cannot be read. The job only ran because this PR edits .github/benchmark/models_accuracy.json; its PR-level model whitelist (DeepSeek-R1-0528 main, gpt-oss-120b nightly) selects none of this PR's entries, so the change cannot affect what it validates.
next: Fetch the failed step's log before attributing it; dropping the CI commit (as the author plans) removes this trigger. https://github.com/ROCm/ATOM/actions/runs/42986166499
|
CI on 932aefb, which agrees on TP4 + PCP2, DeepSeek-V4-Pro (job): all 8 ranks built 86310 blocks from one agreed TP2, Meta-Llama-3-8B-Instruct (job): both ranks built 107631 blocks. gsm8k 0.7309 (bar 0.73). SP4 (job) was refused at startup again: the CI AITER wheel still lacks ROCm/aiter#7346. Other accuracy jobs: 13 of the other 14 passed.
A rerun would settle whether it is flaky. Other workflows: Pre Checkin, Actionlint, ATOM vLLM Test and ATOM SGLang Test passed. Atomesh Accuracy Validation failed, as it does on every recent PR. |
…count" This reverts commit 443b280. The entries existed only to publish a multi-rank run of this change; the results are in #3171 (comment). Co-authored-by: Cursor <cursoragent@cursor.com>
Every TP worker of an engine, and every rank of its PCP group, builds its KV pool at the block count rank 0 reports (`call_func` surfaces rank 0's answer). A worker whose own estimate is smaller can therefore be asked for more blocks than it budgeted. Reduce the count to the minimum across the TP group and, when it spans more than one rank, the PCP group, next to the existing pipeline-parallel reduction. The PCP group is gated on `pcp_group_size`, not `prefill_context_parallel_size`: Ulysses SP runs over the same group with PCP=1.
Use the `group_min` helper the runner already imports for the memory guard instead of a second hand-written MIN all-reduce. The source-level test now also checks that the count is a MIN over each TP and PCP group's CPU group and that it is folded back into the plan, so MAX, the device group or a dropped `with_paged_entries` would fail it. Co-authored-by: Cursor <cursoragent@cursor.com>
Add three pr-level accuracy entries that start an engine across more than one worker, each with the pass bar of its existing entry: - Meta-Llama-3-8B-Instruct TP2. - MiniMax-M3-MXFP4 SP4, the validated Ulysses SP setup. - DeepSeek-V4-Pro TP4 PCP2, the PCP guide's launch. A new optional catalog field, `kv_block_ranks`, makes the native ATOM job require that many `Memory budget` lines in the server log, all reporting one `num_kvcache_blocks`. Before the reconcile, a TP8 Qwen3-Next run on main logged eight different counts, from 2315143 to 2328796. Co-authored-by: Cursor <cursoragent@cursor.com>
Reducing `num_kvcache_blocks` after `plan_pools` put a collective behind two checks that read only this rank's memory: the connector reserve against `free`, and `InsufficientPoolBudget`. A rank short of memory raised there, and its peers waited in the reduction until the gloo timeout. Take the minimum of `free` and the KV budget over the TP and PCP groups before either check instead. The ranks then raise together or not at all, and `plan_pools`, a pure function of those inputs, builds the same plan on each, so the `with_paged_entries` refold is gone. The source-level test now also requires the agreement to precede `free_for_kv`, which both raises read, with nothing in between overwriting the agreed values. Co-authored-by: Cursor <cursoragent@cursor.com>
…count" This reverts commit 443b280. The entries existed only to publish a multi-rank run of this change; the results are in #3171 (comment). Co-authored-by: Cursor <cursoragent@cursor.com>
valarLip
left a comment
There was a problem hiding this comment.
- tests/test_model_runner_staging_fence.py:182-229 still checks source text (
ast.unparse(...) == "groups = [get_tp_group()]", line-number ordering), and it is now longer than before. Any rename or reformat breaks it, and it catches no real behavior change. Please replace it with a behavioral test: monkeypatchgroup_min/mem_get_infoso one rank reports less free memory, then assert thatget_num_blocksreturns the reduced block count, and that the connector-reserveRuntimeErroris raised on every rank, not only on the low one. - model_runner.py:2077-2087:
config.pcp_group_size > 1duplicates thegroup.world_size > 1check. Mirror the existing loop at model_runner.py:4320-4322:for group in (get_tp_group(), get_pcp_group()): if group.world_size > 1: .... - Trim the 6-line comment to one sentence on why: every worker builds the pool at rank 0's count, so agree before any rank-local raise.
- The
if reduced != ...guard from the previous round is gone, thanks. Keep the PP reduction on the result as it is (stages differ).
(Reviewed at 6d7d49a.)
6d7d49a to
3d6053b
Compare
valar-cop
left a comment
There was a problem hiding this comment.
LGTM
ci ATOM Test / Accuracy (Kimi-K3 DSpark, moonshotai/Kimi-K3, --kv_cache_dtype fp8 -tp 8 ...) failed — not sure yet (low): The captured log is empty (0 bytes) so the failure cannot be read. It is a plain TP8 entry that does take the new collective, but the same code path (collective present, counts reduced to the group min) passed on the previous head 932aefb, where only GLM-5.3-Flash-kpool failed, so there is no evidence tying this to the PR.
next: Fetch the failed step's log from the run, or re-run the job, and attribute only from the log. https://github.com/ROCm/ATOM/actions/runs/43133797573/job/130908180673
ci ATOM Test / Accuracy (DeepSeek-V4-Pro DSpark, deepseek-ai/DeepSeek-V4-Pro-DSpark, --kv_cache_dtype fp8 --inde...) failed — main is already broken the same way (medium): The same job fails on main in this window (main runs at 17:21, 16:09 and 15:21 red, 14:44 and earlier green), so the failure predates and is not attributable to this PR; its log here is also empty.
next: Treat as baseline; wait for main to go green before reading this job as a signal. https://github.com/ROCm/ATOM/actions/runs/43133797573/job/130908181071
Move the TP/PCP minimum of free memory and KV budget, and the connector reserve check that reads the agreed free memory, out of get_num_blocks into agree_kv_budget in atom/distributed/group_agreement.py. The group gate is now world_size per group, so SP (which runs over the PCP group with PCP=1) needs no separate pcp_group_size check. The source-level AST assertions on get_num_blocks are replaced by a behavioural test that runs the agreement with a stand-in group_min, so it runs in the non-GPU pre-checkin suite. Co-authored-by: Cursor <cursoragent@cursor.com>
Thanks for the review. Addressed in 869fe7a:
Gate is now
Comment trimmed to one sentence: "Every worker builds its pool at rank 0's count, so take the group minimum before any rank raises alone and leaves the others waiting."
PP reduction unchanged, as you suggested. |
valarLip
left a comment
There was a problem hiding this comment.
⚠️ Action required before merge (3)
- Keep the KV-connector check out of
group_agreement.py.atom/distributed/group_agreement.py:60-70is the generic "verdicts every rank takes alike" module, but it now holds theATOM_PD_MLA_STAGING/ATOM_PD_MLA_LANDINGerror. Its caller also takesconnector_bytesoff the budget (model_runner.py:2065-2070) while the helper takes it offfree. Have the helper return only the agreedfreeand budget, then dofree_for_kv, the raise and themininget_num_blocksright after the call. Every rank still raises together, sincefreeis already agreed. That also removes the need for the 4-fieldAgreedKvBudget. - Bring the PR body up to date with 869fe7a. The body still says the agreement is gated on
pcp_group_size(the code now checksworld_sizeper group,group_agreement.py:55). It also liststest_model_runner_staging_fence.py"7 passed" as the test, but that test was removed in favour oftests/test_kv_budget_agreement.py, and it says 443b280 "will be dropped" though it is already reverted (3d6053b). Update the test plan, and the unchecked CI box, to match the current head. - Make the fake
group_minindependent of the values passed.tests/test_kv_budget_agreement.py:30looks up the peer by value (peers[value]). If anyfreehappens to equal any budget, the fake returns the wrong peer without any error. Key the peer on the call (e.g. pop from a list per call, infree, budget order) instead.
Not blocking: heavy ATOM Test jobs at this head are still pending. On earlier heads, DeepSeek-V4-Pro DSpark also failed on main.
Re-reviewed at 869fe7a. Already fixed: AST source test replaced by a behavioural test; pcp_group_size gate replaced by the world_size > 1 loop (group_agreement.py:54-55); comment trimmed to one reason (model_runner.py:2071-2072); PP reduction kept as is (model_runner.py:2143-2156); agreement moved before both rank-local raises (@yitingw1 / valar-cop).
|
The DeepSeek-V4-Pro DSpark accuracy failure on Accuracy (DeepSeek-V4-Pro DSpark) dies during The same job fails the same way on main. The latest completed ATOM Test, 43143449277 ( |
|
@zhuyuhua-v pls help merge the pr |
no need fix in this pr |
Summary
Every TP worker of an engine, and every rank of its PCP group, builds its KV pool at the block count rank 0 reports (
call_funcsurfaces rank 0's answer). A worker whose own estimate is smaller can therefore be asked for more blocks than it budgeted.Before sizing the pools, this takes the minimum of
freeand the KV budget over the TP group and, when it spans more than one rank, the PCP group. Every rank then sizes from the same inputs:freeandInsufficientPoolBudgetfromplan_pools, raise on all ranks together or on none. A rank short of memory can no longer raise alone while its peers wait in a reduction.plan_pools, a pure function of those inputs, builds the same plan on each rank, so no block count has to be folded back into it afterwards.The PCP group is gated on
pcp_group_size, notprefill_context_parallel_size, because Ulysses SP runs over the same group with PCP=1. TheMemory budgetlog line now prints the agreedfreeandavailable_for_kv.It changes behaviour only with PP=1, where
get_num_blockshad no collective at all. With PP>1 the existing reduction afterplan_poolsalready takes the minimum block count over the default (WORLD) group, TP and PCP ranks included, so the counts already agree; there the new agreement leaves them unchanged and costs two single-int64 gloo all-reduces per group at startup, kept so there is one code path. PP stages can still raise apart, as onmain.Split out of #3147 (decode P/D admission), where it was found. It is the bottom of the stack #3171 ← #3147 ← #3173 ← #3165 and does not depend on the PRs above it.
Commit 443b280 (three CI entries,
kv_block_ranks,check_kv_block_agreement.pyand its workflow step) is only there to publish a multi-rank run. It will be dropped before this PR lands.Test plan
tests/test_model_runner_staging_fence.py: a source-level check thatfreeand the KV budget are reduced withgroup_minover each TP and PCP group's CPU group, the PCP group gated onpcp_group_size, beforefree_for_kv, with nothing in between overwriting them. It fails on MAX, the device group, a missing budget reduction, the earlier post-plan_poolsplacement, the agreement moved afterfree_for_kvor after the plan is published, and a re-read offreein between. 7 passed.plan_pools: TP4 + PCP2 (DeepSeek-V4-Pro) and TP2 (Llama-3-8B) report one count on every rank. Log lines and gsm8k in fix(runner): reconcile KV block counts across TP and PCP-group workers #3171 (comment).plan_pools: ATOM Test in progress.