Conversation
…trix Master entries can already select one variant of an override-format recipe (recipe.yaml:zip_override_x[i]), but three consumers only understood flat files: - Launchers read the recipe as text for power detection and name/health-check patches. materialize_srt_configs now writes the selected variant beside its source and points CONFIG_FILE / EVAL_CONFIG_FILE at the flat copy, so every later step sees a standalone recipe. Flat recipes pass through untouched. - recipe_node_count now reads the selected variant instead of falling back to the master-topology estimate. - recipe_fingerprint and curve identity map selector CONFIG_FILEs listed in srt-slurm-recipe-identities.yaml back to the flat recipe they replaced, so consolidating recipes keeps history and Klaud baselines intact. Expansion mirrors srtctl's generate_override_configs and is tested against it.
|
Thanks for the contribution!
中文感谢你的贡献！
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if split_config_file(config_file)[1] is None and "base" in yaml.safe_load( | ||
| recipe_path.read_text() | ||
| ): | ||
| # Without a selector srtctl submits every variant, which has no single | ||
| # node count. The master topology supplies the estimate. | ||
| return None |
There was a problem hiding this comment.
🟡 (optional) For an override-format srt-slurm recipe referenced without a selector (a plain CONFIG_FILE, e.g. recipes/x/disagg.yaml), recipe_node_count now returns None before ever checking recipe.get("schema") != 2, so a recipe missing schema: 2 silently falls back to the master-topology node-count estimate instead of raising. On main, the schema check ran unconditionally before the base/override check, so a malformed override recipe always raised ValueError("srt-slurm recipes must declare schema: 2"). Fix: keep the schema validation on the fast-return base-without-selector path too, e.g. by loading the raw dict once and validating schema before deciding whether to return None.
Why this was flagged
Trigger: a checked-in override-format recipe (contains top-level base:) that is missing schema: 2, referenced by a master CONFIG_FILE with no :selector suffix, reaching infx/matrix/generate.py:recipe_node_count via infx/matrix/plan.py's matrix generation. The new early-return at generate.py:248-253 checks only split_config_file(config_file)[1] is None and "base" in raw and returns None, skipping the recipe.get("schema") != 2 check at line 255 entirely for this path. On main the schema check ran first and unconditionally, so this malformed recipe would raise ValueError and fail matrix generation loudly; now it is silently treated as unknown/estimate-only with no error, hiding an authoring mistake until runtime job submission.
Verification: nit. The ordering change is real and as described. Base ran recipe = yaml.safe_load(...); if recipe.get("schema") != 2: raise ...; if "base" in recipe: return None, so a malformed override recipe (top-level base: but no top-level schema: 2) raised. The new code (infx/matrix/generate.py:248-253) short-circuits first: `if split_config_file(config_file)[1] is None and "base" in… | nit.…
| local name value | ||
| for name in CONFIG_FILE EVAL_CONFIG_FILE; do | ||
| value="${!name}" | ||
| [[ "$value" == *:* ]] || continue |
There was a problem hiding this comment.
🟣 Pre-existing (partial fix): materialize_srt_configs() only rewrites CONFIG_FILE when it contains a colon selector, so an override-format recipe referenced with no selector still reaches launchers as raw text with base:-nested keys, exactly the bug this PR sets out to fix. configs/nvidia-master.yaml:2536 sets CONFIG_FILE=recipes/dsr1/sglang/gb200-fp8/8k1k/disagg-tp8-stp-variants.yaml with no :selector, yet that recipe has base: and a 3-way zip_override_lowlat: (file lines 2-117, 118-132). materialize_srt_configs's [[ "$value" == *:* ]] guard (slurm_utils.sh:19) skips it, so runners/launch_gb200-nv.sh:489 sed -i "s/^name:.*/.../" "$CONFIG_PATH" still can't match the indented name: at line 3 of that recipe, same as on main. …
Why this was flagged
…Fix: also materialize/flag override-format CONFIG_FILE values that lack a selector (or reject them) so the launcher-text-parsing checks this PR fixes for selector paths are not silently skipped for this real production entry.
Trigger: configs/nvidia-master.yaml:2536 CONFIG_FILE has no :selector, dispatched through the gb200-nv launcher. materialize_srt_configs (runners/slurm_utils.sh:15-27) requires a colon in the value (line 19) before calling the selector module, so this value is left untouched. runners/launch_gb200-nv.sh:473-489 then treats the raw multi-variant file as flat: CONFIG_PATH equals the source file, and sed -i "s/^name:.*/.../" "$CONFIG_PATH" cannot match the recipe's name: (nested 2-space under base: at recipe line 3), so the job-name patch silently no-ops just as it did before this PR. The PR's own description names this exact class of launcher bug as motivation; its fix (materialize_srt_configs) does not cover CONFIG_FILE values without a selector, so this production row keeps the original failure mode.
Verification: pre-existing. The mechanism is real and reachable, but the base branch already fails identically by the same route; the PR's new code is a no-op on this path. Chain verified: - runners/slurm_utils.sh:19 [[ "$value" == *:* ]] || continue — materialize_srt_configs only rewrites CONFIG_FILE/EVAL_CONFIG_FILE that contain a colon selector; a bare override-format reference is skipped unchanged. -…
|
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 |
|
Heads-up: #3576 (merged) replaced the bash launchers with a Python launcher, so this PR will conflict when you merge |
This branch has not been deployed
3 participants
Base of a two-PR stack. The recipe consolidation that uses this tooling is stacked on top: #3391.
Why
Master entries can already point at one variant of an override-format srt-slurm recipe (
recipe.yaml:zip_override_x[i]), and srtctl handles that natively. Three InferenceX consumers did not:awkover top-leveltelemetry:.launch_b200-nscale-slurm.sh/launch_h100-dgxc-slurm.shsed-patchname,max_attemptsanddist-timeout. In an override file those keys sit indented underbase:, so these checks silently stop matching.base:and fell back to the master-topology estimate. That breaks the invariant thatnodes:Nis exact.CONFIG_FILEstring. Moving a recipe into a variants file would change every fingerprint and break history and Klaud baseline matching, even when the recipe is byte-identical.What
infx/srt_slurm/recipe_selector.py: resolves exactly one variant (base,override_<name>,zip_override_<group>[i]) using srtctl's rules (deep merge,nulldeletes, zip slicing and broadcast, auto-naming). It rejects group and glob selectors, because one matrix row is one job. It is tested against the pinned srtctlgenerate_override_configs.materialize_srt_configsinrunners/slurm_utils.sh, called right afterslurm_utils.shis sourced in all six srt-slurm launchers. For a selectorCONFIG_FILE/EVAL_CONFIG_FILE, it writes<file>.<selector>.resolved.yamlbeside the source and points the variable at it. Every later step (power detection, patches,apply_srt_recipe) then sees a standalone recipe. Flat recipes pass through untouched with no new dependency. The selector path usesuv run --with pyyamlbecause not every login node's systempython3has PyYAML (h100's does not).recipe_node_countreads the selected variant.recipe_fingerprintand_matrix_curve_keymap anyNAME=<value>setting listed inbenchmarks/multi_node/srt-slurm-recipe-identities.yamlback to the flat recipe it replaced. The map is empty in this PR.docs/configuration-procedures.md(+_zh).Behavior change on
mainI diffed the full multi-node matrix (
full-sweep --multi-node, both default and--all-evals) betweenmainand this branch. 457 of 459 rows are identical. The two differences are eval rows of the existingdsr1/sglang/b200-fp4/8k1k/disagg-stp-mtp-variants.yamlentries. Theirnode-countmoves from the master estimate to the recipe's actual allocation::override_stp_maxtpt_7p2d:zip_override_stp_lowlat[2]Their fingerprints change with the node count. Those same entries also start receiving the b200-nscale
name/max_attempts: 720patches that every flat recipe on that launcher already gets. The GLM-5.2 selector entries have no telemetry block, so the power lane is unaffected.Testing
utils/test_recipe_selector.py:materialize_srt_configsrun through bash with a stubbeduvCONFIG_FILEandEVAL_CONFIG_FILEpytest utils/ runners/: 1711 passed locally. Skipped only suites needing deps not installed locally (codeowners, torch).ruff check/ruff format --check infx: clean.perf-changelog.yamlentry.