Conversation
|
Thanks for the contribution!
中文感谢你的贡献！
|
amd-master.yaml moved minimaxm3-fp8-mi300x-vllm-agentic-mtp to vllm/vllm-openai-rocm:v0.30.0 in #3361, but the recipe still named v0.29.0, so the single-node adapter rejected every point.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also looked at the new lmcache-server service binding --host 0.0.0.0 --http-host 0.0.0.0 in the vendored srt-slurm patch (line ~600) — this exposes the RPC/HTTP ports to the whole node network rather than just localhost, but that's consistent with how srt-slurm's existing service kinds bind for multi-worker access within the job's network namespace, so it doesn't look like a new exposure introduced by this PR.
Extended reasoning...
The diff migrates LMCache DRAM cache setup from a per-TP-rank shell-scripted fleet to a vendored srt-slurm lmcache-server service; it touches a shell script, a benchmark YAML config, a large vendored patch, and a changelog, with no direct auth/crypto surface. An inline finding (pre-existing set -euo pipefail nounset violation) is already being posted, so I did not approve; I additionally checked the 0.0.0.0 bind in the new service definition and found it matches the pattern of other srt-slurm service kinds rather than being a novel exposure.
| # Install the ROCm LMCache wheel for the MI300X MiniMax-M3 AgentX LMCache point. | ||
| # It runs twice: as the vLLM worker's setup_script (the connector is imported | ||
| # there) and in the lmcache-server service's preamble (its own container). | ||
| set -euo pipefail |
There was a problem hiding this comment.
🟣 Pre-existing: lmcache-mp-rocm.sh still has set -euo pipefail (line 5), violating AGENTS.md's mandatory "Do not enable nounset" bash rule, which explicitly bans combined flags like set -euo pipefail. The diff rewrites nearly the whole script (strips the shard loop and env-var gating) and now runs this script from two call sites (setup_script and service preamble), but keeps the nounset flag. Fix: drop -u (use set -eo pipefail) per AGENTS.md, consistent with the rest of the sweep since this script is being touched anyway.
Why this was flagged
AGENTS.md:34 forbids set -u/set -euo pipefail in active bash scripts, requiring explicit validation instead. lmcache-mp-rocm.sh:5 keeps set -euo pipefail, unchanged by this diff but the surrounding script is rewritten and now invoked from a new preamble call site as well as setup_script. No unset-variable dereference currently exists in the new script body, so there's no active crash today, but the convention violation persists and is reachable from a second, new invocation path (the service preamble) that did not exist before. This is pre-existing, not newly introduced, but the diff is a natural point to bring it into compliance since the file is being substantially rewritten.
Verification: pre-existing. lmcache-mp-rocm.sh:5 has set -euo pipefail, which AGENTS.md:34 explicitly forbids: "Do not enable nounset. No set -u, set -o nounset, combined flags such as set -euo pipefail... Use explicit validation; preserve other intended shell options, for example set -eo pipefail." So the candidate's claim is factually accurate — the fix is set -eo pipefail. Severity is…
…MCache point The refreshed lmcache-server patch (SemiAnalysisAI/srt-slurm#32 at 181b2e4) keeps a role's connector on a direct vllm serve aggregate worker, so the variant names connector: lmcache-mp instead of hand-writing its kv-transfer-config. The message queue timeout falls back to LMCache's default (300 s) instead of 6000 s.
The role connector is now the lmcache-mp preset written out plus lmcache.mp.mq_timeout 6000, so the point keeps its measured timeout instead of falling back to LMCache's 300 s default.
…slurm v2.30.0 main bumped srt-slurm to v2.30.0 and dropped the 504 patch. The 507 patch is regenerated on v2.30.0 from SemiAnalysisAI/srt-slurm#32 at 181b2e4, and the #3545 perf-changelog entry is rewritten to state the benchmark change: one lmcache-server with 1296 GB L1 instead of 8 per-rank 162 GB servers, image v0.30.0, LMCacheMPConnector at localhost:8750 with mq_timeout 6000, re-measured results.
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=36479049525 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=36479049525 |
This branch has not been deployed
2 participants
What changed
The MI300X MiniMax-M3 AgentX LMCache point (
minimaxm3-fp8-mi300x-vllm-agentic-mtp, variantoverride_tp8_c16_lmcacheofbenchmarks/single_node/srt-slurm-recipes/minimaxm3/vllm/mi300x-fp8-mtp/agentic.yaml) now uses srt-slurm's nativelmcache-serverservice instead of LMCache servers thatlmcache-mp-rocm.shstarted inside the worker container.services:entry oftype: lmcache-server. srtctl runslmcache serveron the worker node on ports 8750 (RPC) and 8751 (HTTP) before vLLM starts, marks it critical, and waits forGET /healthcheck. The LMCache flags are unchanged:--l1-init-size-gb 10 --l1-read-ttl-seconds 7200 --chunk-size 256 --max-workers 2 --eviction-policy LRU --supported-transfer-mode lmcache_driven.connectoris thelmcache-mppreset JSON (LMCacheMPConnectorattcp://localhost:8750) plus the measuredlmcache.mp.mq_timeout: 6000. The preset alone would fall back to LMCache's 300 s timeout.lmcache-mp-rocm.shnow only installs the ROCm LMCache 0.5.3 wheel (plus cupy-rocm and the Prometheus exporter). It runs twice: as the LMCache variant'ssetup_scriptin the worker container, because the connector is imported inside vLLM, and as the service'spreamble, because the service runs in its own step of the job container.setup_scriptmoved from the base recipe to the LMCache variant, so the GPU-resident points no longer run it (it was a no-op for them).http://<node>:8751/healthcheck) with a 900 s timeout instead of 300 s, since the preamble's pip install counts against it. The old script waited up to 600 s after installing.runners/srt-slurm/patches/507-lmcache-server-atom-sglang.patchand its README entry, byte-identical to feat(agentx): port the MI355X DSV4 ATOM disagg LMCache config to srt-slurm #3543 (feat: add LMCache support for ATOM and SGLang srt-slurm#32, which includes feat(services): lmcache-server kind and lmcache-mp connector NVIDIA/srt-slurm#507).The other variants are unchanged.
Why
The DRAM point should use the same LMCache server path srt-slurm now supports natively, rather than a setup script that daemonizes servers inside the vLLM step and polls them itself.
Behavior changes
lmcache.mp.server_urls. It now runs one server per node with the same total L1, 1296 GB, shared by all 8 ranks. The service gives each node one instance on fixed ports; reproducing 8 shards would take 8 service entries, each overriding ports and readiness and each repeating the install, so I kept the recipe simple instead. The single server still uses--max-workers 2, now for 8 ranks instead of 1. Expect the measured numbers to move.frontend.type: vllm, srtctl used to drop a role'sconnectorfor a direct aggregate worker. SemiAnalysisAI/srt-slurm#32 commit 181b2e4 now runs the connector an aggregate role names (the engine-wide default stays prefill/decode only), and the regenerated507-lmcache-server-atom-sglang.patchincludes it.Local verification
origin/agentx/srt-atom-disagg-lmcache.bash -npasses on the script.--kv-transfer-config '{"kv_connector":"LMCacheMPConnector",...,"lmcache.mp.host":"tcp://localhost","lmcache.mp.port":8750,"lmcache.mp.mq_timeout":6000.0}', andsrtctl dry-runlists the service aslmcache server --host 0.0.0.0 --port 8750 --http-host 0.0.0.0 --http-port 8751 --l1-size-gb 1296 ..., placement workers, start before_workers, critical, container = job container, readinesshttp://<node>:8751/healthcheckwith a 900 s timeout, preamblebash /configs/lmcache-mp-rocm.sh. The GPU-resident variants render with no kv-transfer-config and no setup script.launch_mi300x-amd.sh->launch_srt_single_node) goes throughsetup_srt_slurm, which appliesrunners/srt-slurm/patches/*.patchand copiesbenchmarks/multi_node/srt-slurm-recipes/configs/to srt-slurm'sconfigs/(mounted at/configs).validate_perf_changelogpasses against origin/main.Not verified
connector).LMCacheMPConnectorwithmq_timeout 6000, and the server registered 8 GPUs.--max-workers, would separate the two.configs/amd-master.yamlmoved this config tovllm/vllm-openai-rocm:v0.30.0in [Klaud Cold] Update minimaxm3-fp8-mi300x-vllm-agentic-mtp vLLM ROCm image to v0.30.0 / 将 minimaxm3-fp8-mi300x-vllm-agentic-mtp 的 vLLM ROCm 镜像更新至 v0.30.0 #3361, but the recipe still namedv0.29.0, so the single-node adapter rejected every point, on main as well. The recipe now usesv0.30.0, andcheck_sn.pybinds all 12 points (6 points × perf/eval).pytest infx/tests/srt_slurm: the same 15 local-environment failures asmain, none new.--max-workers 2keeps up with 8 TP ranks, and whether the pinned L1 grows to 1296 GB within the job's host memory as the 8 shards did.Dependency
Requires the lmcache-server srt-slurm patch, the same one as #3543 (SemiAnalysisAI/srt-slurm#32 / NVIDIA/srt-slurm#507). If #3543 lands first, this PR's copy of the patch becomes a no-op diff after rebase.