Conversation
|
Thanks for the contribution!
中文感谢你的贡献!
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a very large, mechanical restructuring (402 files, +16.8k/-54.6k lines) that changes how ~357 recipes are addressed (moving to positional zip_override_variants[i] indices) and is stacked on an unmerged base branch, a human look would still be worthwhile before merge.
What was reviewed: the variants.yaml consolidation pattern (shared base + zip_override_variants, header mapping old files to indices) in several directories, the corresponding configs/nvidia-master.yaml selector rewrites (spot-checked several CONFIG_FILE=...:zip_override_variants[i] mappings against the variant headers), and the three directories intentionally left flat. The perf-changelog omission and positional-index fragility were already flagged and reasoned through in the ruled-out list.
Extended reasoning...
Large mechanical PR (402 files) consolidating srt-slurm benchmark recipe YAMLs into per-directory variants.yaml files with zip_override_variants, plus corresponding configs/nvidia-master.yaml selector updates; no application/security-sensitive code is touched, only benchmark config data. Spot-checked one variants.yaml (dsr1/sglang/b200-fp8/8k1k) and matching nvidia-master.yaml selector edits (dsr1/trtllm/b200-fp4/8k1k) and both look internally consistent with the stated header-to-index mapping. Decided defer over approve because of sheer scale, the new positional-index addressing scheme (a design choice a human should weigh in on), and that the PR is stacked on an unmerged base branch.
…variants Replace 357 flat recipes referenced by the active NVIDIA master config with one variants.yaml per directory (40 directories): a shared base plus one override_<name> block per benchmark configuration, named after the replaced file (disagg-1p1d-dep8-b8-eplb0-mtp3.yaml -> override_disagg_1p1d_dep8_b8_eplb0_mtp3) and keeping its original recipe name. Master entries select variants.yaml:override_<name>; srt-slurm-recipe-identities.yaml maps each selector back to the file it replaced. No resolved recipe changes: - every override expands (srtctl generate_override_configs and infx) to exactly the flat recipe it replaces; - the full multi-node matrix, including node counts and recipe fingerprints, is identical in default and all-evals modes; - launcher text steps (power-lane detection, name/max_attempts/dist-timeout patches) give the same result on the materialized variant, except max_attempts on 15 GB200 recipes whose launcher does not apply that patch. Every original comment is kept beside the same key; comments specific to one configuration sit inside its override. Skipped: dsr1/sglang/b200-fp4/8k1k and dsr1/trtllm/b200-fp8/8k1k (a differing explicit null deletes rather than sets) and glm5.2/sglang/h200-fp8/agentx (a non-schema-2 sibling). Recipes used by deprecated configs are untouched.
aec6c5b to
85c369c
Compare
|
Sorry, over the weekend, there was 2 major refactors to clean up the technical debt accumalated over the past 11 months of moving at the speed of light. We don't see any major refactors in the forthseeable future besides cleaning up AMD multinode AgentX pile of bash. As much, due to the refactors, u would need to ask your agent to rebase from remote main@latest. Thank you in advance for ur understanding |
Stacked on #3390 (base branch
ix/srt-recipe-selector-tooling). Review and merge that first; this PR retargets tomainafterwards.What
Replaces 357 flat srt-slurm recipes referenced by the active
configs/nvidia-master.yamlwith onevariants.yamlper directory (40 directories). Each file has a sharedbaseplus one named override block per benchmark configuration, separated by blank lines:RECIPES.mdconvention (topology plus distinguishing settings), lowercased with underscores. Each block keeps its original recipenameand lists only what differs frombase.CONFIG_FILE=recipes/<dir>/variants.yaml:override_<name>. The 357 old→new pairs are recorded inbenchmarks/multi_node/srt-slurm-recipe-identities.yaml, so fingerprints and curve identity are unchanged.zip_override_*lists: each configuration reads on its own, and selectors are names rather than positions, so adding or removing a configuration never shifts others. srt-slurm: resolve CONFIG_FILE recipe selectors in launchers and the matrix #3390's resolver handles both forms.RECIPES.md(+_zh) documents the layout.Net diff: 401 files, +25.2k / −54.6k lines.
Not changed
glm5.2/sglang/gb200-fp4/agentx/agg.yaml) was also left alone.dsr1/sglang/b200-fp4/8k1kanddsr1/trtllm/b200-fp8/8k1k: siblings differ by an explicitnull, and anullin an override deletes the key instead of setting it.glm5.2/sglang/h200-fp8/agentx: one sibling is notschema: 2.Equivalence evidence
The scripts were run locally; they are one-off and not committed.
yaml.safe_load(old file)equals srtctl'sgenerate_override_configs(new, "override_<name>")(pinned submodule), and also equals the InferenceX resolver's output.full-sweep --multi-nodeagainst srt-slurm: resolve CONFIG_FILE recipe selectors in launchers and the matrix #3390, in default (518 rows) and--all-evals(459 rows) modes, is identical row for row once the new selectors are mapped back to their old paths. That covers every node count and every recipe fingerprint, including the 454 / 411 rows that now use selectors. Eval grouping is unchanged, because old paths map one-to-one onto distinct selectors.awkand GNU-sed-equivalentname/max_attempts/dist-timeoutpatches. The power lane is identical for all 357, including the 51 power recipes. The only difference ismax_attemptson 15 GB200 AgentX recipes: the originals use inlinehealth_check: {…}, which the b200-nscale sed never matched. Those recipes run throughlaunch_gb200-nv.sh, which has no such patch, so there is no runtime change.base; configuration-specific ones sit inside that configuration's override.perf-changelog
This PR has deliberately no
perf-changelog.yamlentry. No resolved recipe, node count or fingerprint changes. An entry covering these config keys would re-run essentially every NVIDIA multi-node benchmark on merge to reproduce identical configurations. Reviewers can overrule this if the repository rule should apply regardless.Testing
pytest utils/ runners/: 1711 passed locally. Skipped only suites needing deps not installed locally.sweep-enabledsmoke per affected launcher (b200-nscale, b300-dsxe, gb200-nv, gb300-nv, h100-dgxc, h200-dgxc), to exercisematerialize_srt_configson the real login nodes, includinguv run --with pyyaml.