fix(agentic): propagate speculative decoding to sweep jobs#2183
Conversation
|
Claude finished @cquil11's task in 2m 36s —— View job Review of PR #2183
LGTM - no blocking issues found. Verified the details: Note: I could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pcp_size = bmk.get(Fields.PCP_SIZE.value, 1) | ||
| ep = bmk.get(Fields.EP.value) | ||
| dp_attn = bmk.get(Fields.DP_ATTN.value) | ||
| spec_decoding = bmk.get(Fields.SPEC_DECODING.value, "none") |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1313
- Base Lines: 1307
- Change: 📈 +6 lines
There was a problem hiding this comment.
LGTM — this is a small, well-tested plumbing fix, and the one issue found (missing spec-decoding suffix in multi-node agentic exp-names) is a cosmetic naming gap, not a functional bug.
Extended reasoning...
Overview
This PR threads the existing single-node agentic spec-decoding field through matrix generation (generate_sweep_configs.py), into the two workflow entrypoints (run-sweep.yml, e2e-tests.yml), and into experiment-name suffixes, replacing a hardcoded 'none' that could previously mislabel a config running mtp. It also adds a default-"none" spec_decoding field to SingleNodeAgenticMatrixEntry in validation.py and two new unit tests covering the propagation and exp-name suffix behavior for the single-node agentic path.
Security risks
None. This is CI/config-generation logic only — no auth, secrets, or user-facing surface is touched. The workflow YAML changes are simple substitutions of a hardcoded literal for an already-validated matrix field (matrix.config.spec-decoding), which flows through the same pydantic-validated pipeline as every other matrix field.
Level of scrutiny
Low-to-moderate. This is internal benchmark-orchestration tooling (not a production request path), the change is small and mechanical (add a field, forward it, append a suffix), and it comes with new passing unit tests plus the full existing suite (221 passed). The one finding from the bug hunt (multi-node agentic exp-name not getting the same _spec-{mode} suffix as single-node) is explicitly cosmetic — the correct spec-decoding value is still stored and forwarded to the workflow regardless of exp-name, so no benchmark would run with the wrong launcher/config as a result.
Other factors
The candidate issue about SingleNodeAgenticMatrixEntry.spec_decoding having a default was raised and refuted — a default of "none" here matches the PR's explicit backward-compatibility goal (omitted values stay "none"), and the same pattern is already used elsewhere in this file (e.g. SingleNodeSearchSpaceEntry.spec_decoding). Test coverage for the new behavior is present for the single-node path; the multi-node exp-name gap is a minor documentation-style nit worth a follow-up but not blocking.
| Fields.EXP_NAME.value: ( | ||
| f"{model_code}_tp{tp}_conc{conc}_" | ||
| f"{agentic_kv_offload_suffix(kv_offloading, kv_offload_backend)}" | ||
| + (f"_spec-{spec_decoding}" if spec_decoding != "none" else "") | ||
| ), | ||
| Fields.SCENARIO_TYPE.value: "agentic-coding", | ||
| } |
There was a problem hiding this comment.
🟡 The multi-node agentic exp-name (around lines 700-712 and 1000-1012) wasn't updated to append _spec-{spec_decoding} the way the single-node agentic exp-name in this PR was, even though the multi-node agentic entry does carry spec-decoding. Two multi-node agentic configs differing only in spec-decoding will get identical exp-names — consider adding the same suffix logic there for consistency.
Extended reasoning...
This PR's stated goal is to "include non-default speculative-decoding modes in experiment names," and it does this correctly for the single-node agentic path in both generate_full_sweep and generate_test_config_sweep:
Fields.EXP_NAME.value: (
f"{model_code}_tp{tp}_conc{conc}_"
f"{agentic_kv_offload_suffix(kv_offloading, kv_offload_backend)}"
+ (f"_spec-{spec_decoding}" if spec_decoding != "none" else "")
),However, the analogous multi-node agentic exp-name construction (generate_sweep_configs.py lines ~700-712 and ~1000-1012 in the two functions) was not given the same treatment:
Fields.EXP_NAME.value: (
f"{model_code}_p{prefill[Fields.NUM_WORKER.value]}x{prefill[Fields.TP.value]}"
f"_d{decode[Fields.NUM_WORKER.value]}x{decode[Fields.TP.value]}"
f"_conc{'x'.join(str(c) for c in conc_batch)}"
f"{offload_suffix}"
),This block only ever appends offload_suffix (for KV offloading) — it never references spec_decoding, despite the fact that the multi-node agentic entry a few lines above already resolves spec_decoding = bmk.get(Fields.SPEC_DECODING.value, "none") and stores it via Fields.SPEC_DECODING.value: spec_decoding in the same entry dict.
Concrete proof of the gap: take two multi-node agentic search-space entries with identical prefill/decode topology and the same conc batch, differing only in spec-decoding: mtp vs spec-decoding: none. Both will produce entry["spec-decoding"] values of "mtp" and "none" respectively (correct, and correctly forwarded to run-sweep.yml/e2e-tests.yml via matrix.config.spec-decoding), but both will produce the exact same exp-name string, e.g. kimik2.6_p1x8_d1x8_conc16_kvnone. This is inconsistent with the single-node agentic path, where the same scenario would produce ..._kvnone_spec-mtp vs ..._kvnone.
Why existing tests don't catch it: the new tests added in this PR (test_agentic_spec_decoding_is_propagated, test_single_node_agentic_spec_decoding_is_propagated) only exercise the single-node agentic path's sample_single_node_agentic_config fixture; there is no equivalent multi-node agentic fixture/test asserting exp-name behavior for spec-decoding.
Impact: this is cosmetic, not functional. The correct spec-decoding value is still stored on the entry and propagated through to the workflow inputs/launcher regardless of exp-name — so no benchmark runs with the wrong speculative-decoding setting. It only makes it harder to visually distinguish multi-node agentic result rows that differ solely by spec-decoding mode when reading exp-name in dashboards/logs. It's also worth noting that exp-name is already not a strict uniqueness key elsewhere (e.g. fixed-seq-len paths use the coarser {model_code}_{seq_len_str} across many differing configs), so this doesn't introduce a new class of problem, just extends an existing gap to a scenario this PR specifically claims to have addressed.
Suggested fix: mirror the single-node suffix logic in both multi-node agentic exp-name blocks, e.g. append + (f"_spec-{spec_decoding}" if spec_decoding != "none" else "") after {offload_suffix} in both generate_full_sweep and generate_test_config_sweep.
1 participant
What changed
spec-decodingfrom single-node agentic search-space entries into both generated sweep matrix formatsrun-sweep.ymlande2e-tests.ymlnoneWhy
Single-node agentic master configs already accept
spec-decoding, but sweep generation dropped it and both workflow entrypoints hardcodednone. As a result, a config requestingmtpcould run the STP launcher and be labeled incorrectly.This reusable plumbing is split out from #2158 so the Kimi K2.6 benchmark PR can focus on its model-specific launcher and configuration.
Validation
python -m pytest utils/matrix_logic/ -v(221 passed)