vllm_dissag: remove tests/ scaffolding + follow-ups on #171#175
Open
raviguptaamd wants to merge 5 commits into
Open
vllm_dissag: remove tests/ scaffolding + follow-ups on #171#175raviguptaamd wants to merge 5 commits into
raviguptaamd wants to merge 5 commits into
Conversation
The tests/ folder (offline gates, interactive drivers, TEST_PLAN) existed to validate the 3->1 launcher consolidation during PR ROCm#171, now merged. Nothing in the model-run path (run_xPyD_models.slurm -> vllm_disagg.sh -> connectors) depends on it. Drop it; README keeps a DRY_RUN dry-run snippet for offline argv inspection.
README: - correct rixl proxy port (ROUTER_PORT default 18001, not 30000); split vllm_router vs toy-proxy ports per connector - fix stale "fullsource Dockerfile" refs -> vllm_disagg_inference Dockerfile - document benchmark_long_context.sh (was undocumented) incl. 28K/1K example - clarify MODEL_DIR/MODEL_PATH weight resolution - tighten the image-capability section (drop point-in-time status text) ARCHITECTURE: - fix mermaid rendering: \n -> <br/> in all labels (state/sequence diagrams rendered \n literally); quote edge labels + decision nodes containing parens/pipes so GitHub mermaid parses them - correct hook table: rixl ROUTER_PORT 18001, barrier 5000(TP)/15000(deepep); moriio_toy (not moriio_toy_proxy)
connector_setup_env force-exported VLLM_ALL2ALL_BACKEND=mori unconditionally,
clobbering any .env / models.yaml / submit-time value and leaving the bare
"mori" alias (rejected by newer images) in the process env. Make it
${VLLM_ALL2ALL_BACKEND:-mori} so it still defaults to mori but yields to an
override, matching the file's documented precedence. No change to the assembled
--all2all-backend flag (that resolves from PREFILL/DECODE_MORI_BACKEND).
- Remove point-in-time / private references from comments and docs:
"mori121 image", "v1.2.0 image", "MAD-private #324", "DeepSeekV3_recipe.env"
-> generic wording (ROCm 7.2.3, "newer MoRI-EP images", "validated recipe").
Touches README, connectors/*.env, connectors/{moriio,rixl}.sh, models.yaml.
- Document (not change) the wideEP Gloo default_pg_timeout patch: it is REQUIRED
because the cold AITER JIT compile (~15-20 min) exceeds Gloo's 30-min PG timeout
and would kill the launch; note the --rm / no-host-mount safety invariant.
The default_pg_timeout patch (required so the ~15-20m cold JIT compile doesn't trip Gloo's 30m PG timeout) previously hardcoded the python3.12 dist-packages path and silently no-op'd on failure (2>/dev/null || true) — a torch upgrade or layout change would reintroduce the timeout crash with no signal. Now: - resolve constants.py from the live interpreter (no hardcoded py-version dir) - skip if already patched (idempotent) - verify the substitution actually landed and WARN loudly if not Mechanism unchanged (still an in-source rewrite before torch import — torch exposes no env var and the value is frozen as a default-arg at import, so a post-import monkeypatch is too late). Verified against torch 2.9.
raviguptaamd
requested review from
Rohan138,
amathews-amd,
coketaste,
gargrahul and
ppalaniappan-amd
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up work on top of the merged #171 (unified two-axis vllm_disagg launcher).
scripts/vllm_dissag/tests/— the offline gates (gate_check.sh,argv_assert.sh,run_all.sh), interactive drivers (drive_cell.sh,run_interactive.sh),harvest.sh, andTEST_PLAN.mdexisted to validate the 3→1 launcher consolidation during vllm_dissag: unify 3 launchers into one two-axis driver + models.yaml #171, which is now merged. Nothing in the model-run path (run_xPyD_models.slurm→vllm_disagg.sh→connectors/*.sh) depends on this folder.DRY_RUNsnippet for offline argv inspection; README/ARCHITECTURE references cleaned up.Draft — more follow-up commits to come on this branch.
Test plan
DRY_RUN=1dry-run still assembles correct argv after removal (verified:--enable-expert-parallelpresent for moriio wideEP cell)tests/in launcher, connectors, README, or ARCHITECTURE🤖 Generated with Claude Code