Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/configs/nvidia-master.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8474,7 +8474,7 @@
dp-attn: false
# Mid curve 1p1d-dep4-dep8. 3 nodes.
- spec-decoding: "mtp"
conc-list: [256]
conc-list: [160]

Check warning on line 8477 in .github/configs/nvidia-master.yaml

View check run for this annotation

Claude / Claude Code Review

Missing perf-changelog entry for concurrency tuning

This PR retunes concurrency for four `dsv4-fp4-gb300-dynamo-sglang-mtp` mid-curve configs (in `nvidia-master.yaml` and the four recipe YAMLs) but does not append a corresponding entry to `perf-changelog.yaml`. Because `.github/workflows/run-sweep.yml` gates both its `push` and `pull_request` triggers on a path filter for `perf-changelog.yaml`, the post-merge sweep won't fire for this change and the TTFT improvement vs. run #25642986639 cited in the description can't be auto-recorded — appending

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 This PR retunes concurrency for four dsv4-fp4-gb300-dynamo-sglang-mtp mid-curve configs (in nvidia-master.yaml and the four recipe YAMLs) but does not append a corresponding entry to perf-changelog.yaml. Because .github/workflows/run-sweep.yml gates both its push and pull_request triggers on a path filter for perf-changelog.yaml, the post-merge sweep won't fire for this change and the TTFT improvement vs. run #25642986639 cited in the description can't be auto-recorded — appending a single entry with config-keys: dsv4-fp4-gb300-dynamo-sglang-mtp (matching the pattern of the analogous PR #1305) before merging will fix it.

Extended reasoning...

What the bug is. The PR changes concurrency points for dsv4-fp4-gb300-dynamo-sglang-mtp in .github/configs/nvidia-master.yaml (4 conc-list entries at lines 8477/8492/8507/8522 — 256→160, 256→160, 512→320, 1024→640) and the matching benchmark stanzas of the four mid-curve recipe YAMLs under benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/8k1k/. The PR description's stated goal is to verify a TTFT/E2E improvement against benchmark run #25642986639. However, perf-changelog.yaml is not touched (the PR file list is 5 files — nvidia-master.yaml plus the four recipes).\n\nWhy this matters — the sweep workflow gating. .github/workflows/run-sweep.yml declares its triggers as:\n\nyaml\non:\n push:\n branches: [main]\n paths:\n - "perf-changelog.yaml"\n pull_request:\n branches: [main]\n types: [ready_for_review, synchronize, labeled, unlabeled]\n paths:\n - "perf-changelog.yaml"\n\n\nBoth the push-to-main path and the pull_request path are gated on a paths filter that only matches perf-changelog.yaml. With no edit to that file, the workflow is not eligible to run at all on this PR or on the merge commit.\n\nProject convention reinforces this. AGENTS.md states explicitly: "Changes to perf-changelog.yaml trigger benchmark runs" (Git section) and "perf-changelog.yaml triggers which configs to benchmark" (YAML section). The "Adding a New Benchmark Configuration" checklist (step 2) directs maintainers to "Add corresponding entry to perf-changelog.yaml to trigger benchmark" — and although that step is written for new-config flows, the same gating applies to recipe retunes, which is why earlier tuning PRs added entries too.\n\nPrior precedent — PR #1305. The directly analogous PR "Fix B200 Dynamo vLLM recipe benchmark concurrencies" performed exactly this kind of operation (concurrency tuning in nvidia-master.yaml + recipe YAMLs) and appended a 7-line entry to perf-changelog.yaml with config-keys: dsv4-fp4-b200-dynamo-vllm to trigger the post-merge sweep. That entry is visible at the tail of the current perf-changelog.yaml. The current PR is the same pattern minus the changelog entry, so it falls outside the established workflow.\n\nStep-by-step proof that the sweep will not fire after merge.\n1. Merge commit lands on main. Modified paths: .github/configs/nvidia-master.yaml and four files under benchmarks/multi_node/srt-slurm-recipes/.../8k1k/.\n2. GitHub evaluates run-sweep.yml's on.push block. The paths filter requires at least one changed path to match perf-changelog.yaml.\n3. None of the five changed paths match. The workflow run is not created (no Run Sweep entry will appear in the Actions tab for this SHA).\n4. As a result, the TTFT/TPOT comparison against run #25642986639 that motivates the PR is never produced by the standard pipeline, and no changelog row links this PR for future bisection.\n\nImpact. This is a process/workflow omission, not a code defect — the YAML edits themselves are internally consistent. The consequence is loss of automated verification and changelog provenance for a perf-targeted change.\n\nHow to fix. Append a new entry to the END of perf-changelog.yaml (per AGENTS.md's chronological-append rule), mirroring PR #1305's shape, e.g.:\n\nyaml\n- config-keys:\n - dsv4-fp4-gb300-dynamo-sglang-mtp\n description:\n - "Tune mid-curve MTP disagg concurrencies (1p1d/2p1d/4p1d) to match measured system capacity"\n - "Reduce conc 256→160 (dep4-dep8, dep4-dep16), 512→320 (2p1d), 1024→640 (4p1d); keep num_prompts_mult=16 to preserve total prompt count"\n pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1316\n\n\nMake sure the file still ends with a trailing newline (the check-newline job in run-sweep.yml enforces this on PRs).

prefill:
num-worker: 1
tp: 4
Expand All @@ -8489,7 +8489,7 @@
dp-attn: true
# Mid curve 1p1d-dep4-dep16. 5 nodes.
- spec-decoding: "mtp"
conc-list: [256]
conc-list: [160]
prefill:
num-worker: 1
tp: 4
Expand All @@ -8504,7 +8504,7 @@
dp-attn: true
# Mid curve 2p1d-dep4-dep8. 4 nodes.
- spec-decoding: "mtp"
conc-list: [512]
conc-list: [320]
prefill:
num-worker: 2
tp: 4
Expand All @@ -8519,7 +8519,7 @@
dp-attn: true
# Mid curve 4p1d-dep4-dep8. 6 nodes.
- spec-decoding: "mtp"
conc-list: [1024]
conc-list: [640]
prefill:
num-worker: 4
tp: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ benchmark:
isl: 8192
osl: 1024
random_range_ratio: 0.8
concurrencies: "256"
concurrencies: "160"
num_prompts_mult: 16
req_rate: "inf"
use_chat_template: true
custom_tokenizer: "sa_bench_tokenizers.sglang_deepseek_v4.SGLangDeepseekV4Tokenizer"
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ benchmark:
isl: 8192
osl: 1024
random_range_ratio: 0.8
concurrencies: "256"
concurrencies: "160"
num_prompts_mult: 16
req_rate: "inf"
use_chat_template: true
custom_tokenizer: "sa_bench_tokenizers.sglang_deepseek_v4.SGLangDeepseekV4Tokenizer"
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ benchmark:
isl: 8192
osl: 1024
random_range_ratio: 0.8
concurrencies: "512"
concurrencies: "320"
num_prompts_mult: 16
req_rate: "inf"
use_chat_template: true
custom_tokenizer: "sa_bench_tokenizers.sglang_deepseek_v4.SGLangDeepseekV4Tokenizer"
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ benchmark:
isl: 8192
osl: 1024
random_range_ratio: 0.8
concurrencies: "1024"
concurrencies: "640"
num_prompts_mult: 16
req_rate: "inf"
use_chat_template: true
custom_tokenizer: "sa_bench_tokenizers.sglang_deepseek_v4.SGLangDeepseekV4Tokenizer"