Add H100 MiniMax-M3 NVMe and tiered AgentX sweep - #2775
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
cquil11
changed the title
|
Claude finished @cquil11's task in 4m 35s —— View job Review of PR #2775
SummaryFound 2 blocking issues and 1 warning (see inline comments):
The rest checks out: Note: the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| cleanup_allocation() { | ||
| local rc=$? | ||
| trap - EXIT INT TERM | ||
| if [[ -n "$NVME_HOST_DIR" ]]; then | ||
| srun --jobid="$JOB_ID" bash -c "rm -rf -- '$NVME_HOST_DIR'" 2>/dev/null || true | ||
| fi | ||
| scancel "$JOB_ID" 2>/dev/null || true | ||
| exit "$rc" | ||
| } | ||
| trap cleanup_allocation EXIT INT TERM |
There was a problem hiding this comment.
🔴 BLOCKING: The NVMe KV directory leaks on every successful run.
Why it matters: On the success path, the pre-existing scancel $JOB_ID at the end of the else branch (line 345) cancels the allocation before the script exits and the EXIT trap fires. cleanup_allocation then runs srun --jobid="$JOB_ID" rm -rf … against an already-cancelled job, which fails and is swallowed by 2>/dev/null || true. So the up-to-8 TB inferencex-kv-<jobid> directory is only removed on failure paths (where set -e exits while the allocation is still alive). With 16 NVMe-backed points per sweep, successful runs will steadily fill the node-local /mnt/numa0 array.
Fix: Delete the trailing scancel $JOB_ID on line 345 and let the trap do rm→scancel in order, e.g.:
# cleanup_allocation (EXIT trap) removes the NVMe dir, then scancels.i.e. remove line 345 entirely — the EXIT trap already performs scancel after the rm -rf while the allocation is still alive.
| NVME_OFFLOAD_TOTAL_BYTES=8000000000000 | ||
| NVME_OFFLOAD_PER_RANK_BYTES=$((NVME_OFFLOAD_TOTAL_BYTES / TP)) | ||
| OFFLOAD_ARGS=( | ||
| --kv-transfer-config | ||
| "{\"kv_connector\":\"SimpleCPUOffloadConnector\",\"kv_role\":\"kv_both\",\"kv_connector_extra_config\":{\"kv_offload_backend\":\"disk\",\"disk_path\":\"$NVME_OFFLOAD_DIR/cache.bin\",\"disk_capacity_bytes\":$NVME_OFFLOAD_PER_RANK_BYTES,\"disk_buffer_slots\":4,\"lazy_offload\":false}}" | ||
| ) |
There was a problem hiding this comment.
🟡 WARNING: All TP ranks are pointed at the same disk_path ($NVME_OFFLOAD_DIR/cache.bin) while disk_capacity_bytes is computed per rank (8 TB / TP).
Why it matters: The per-rank capacity math implies the connector is instantiated once per TP worker (consistent with the existing vllm-simple usage in dsv4_fp4_b200_vllm_mtp.sh, which passes cpu_bytes_to_use_per_rank). If the fork's disk backend does not internally shard/suffix the path per rank, 8 workers will open and write the same 1 TB-capacity file and silently corrupt each other's cached blocks — producing wrong "cache hit" data rather than a crash. I couldn't verify the fork's SimpleCPUOffloadConnector disk backend (source unavailable in this review), and the PR's one-off validation covered the tiered OffloadingConnector path, not this NVMe-only path.
Fix: Confirm the disk backend appends a rank identifier to disk_path (and note it in a comment), or make it explicit in the config, e.g. a per-rank template/directory instead of a single cache.bin. If it does shard internally, feel free to resolve this.
There was a problem hiding this comment.
Confirmed in the patched vLLM backend: SimpleCPUOffloadConnector rewrites the configured path as f"{disk_path}.rank_{device.index or 0}" (vllm/v1/simple_kv_offload/worker.py), so TP8 creates cache.bin.rank_0 through cache.bin.rank_7, each with the configured per-rank capacity. Added an inline comment beside the config to make this non-obvious behavior reviewable.
| """ | ||
| kv_offloading = benchmark.get(Fields.KV_OFFLOADING.value, "none") | ||
| if kv_offloading != "dram": | ||
| if kv_offloading != "dram" and kv_offloading != ["dram", "nvme"]: |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1650
- Base Lines: 1642
- Change: 📈 +8 lines
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new dram+nvme branch's DRAM budget arithmetic in minimaxm3_fp8_h100_mtp.sh: it computes PER_RANK_GIB with the same formula as the working dram branch, then multiplies back by TP into CPU_OFFLOAD_TOTAL_BYTES — consistent with cpu_bytes_to_use being an aggregate (not per-rank) budget for TieringOffloadingSpec, so that's not a bug.
Extended reasoning...
This run's inline comments already carry three confirmed findings (a fragile personal ttl.sh image tag with no durable fallback, an NVMe scratch-dir cleanup that races with the pre-existing scancel on the success path so it silently leaks per-job directories, and a schema gap where MultiNodeAgenticMatrixEntry.kv_offloading wasn't widened alongside the single-node/shared entry, letting multinode configs pass validation but fail later). Those are substantive enough that a human should look regardless of anything else. Since findings are already present, per the review protocol the only additional value I can add is naming something concrete that was investigated and ruled out beyond them. I verified the dram+nvme branch's PER_RANK_GIB-then-re-multiply-by-TP pattern in minimaxm3_fp8_h100_mtp.sh against the working dram branch and confirmed it is intentional (aggregate vs. per-rank byte budget for the different connector), not a duplicate concern.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
utils/matrix_logic/validation.py— AgenticCodingSearchSpaceEntry.kv_offloading (used for both single- and multi-node search-space entries) was widened to KVOffloadingConfig, accepting "nvme" and ["dram","nvme"] regardless of topology, but MultiNodeAgenticMatrixEntry.kv_offloading (line 373) was left as the oldLiteral["none","dram"].Extended reasoning...
A user adds
kv-offloading: nvmeorkv-offloading: [dram, nvme]to a multinode agentic search-space entry (prefill/decode or worker topology); it passes config-load validation (AgenticCodingSearchSpaceEntry/_validate_kv_offload_fields places no topology restriction), but generate_full_sweep later builds the multinode matrix entry and validate_agentic_matrix_entry rejects it against MultiNodeAgenticMatrixEntry's still-Literal["none","dram"] kv_offloading, raising a confusing ValueError deep in matrix generation instead of a clear error at the point of misconfiguration. Fix: extend MultiNodeAgenticMatrixEntry.kv_offloading to the same accepted runtime set as SingleNodeAgenticMatrixEntry (Literal["none","dram","nvme","dram+nvme"]), or explicitly reject nvme/dram+nvme for multinode topologies at the AgenticCodingSearchSpaceEntry validation stage if it is intentionally single-node-only.Verification: nit. The inconsistency is real and reachable. validation.py line 326 widens SingleNodeAgenticMatrixEntry.kv_offloading to Literal["none","dram","nvme","dram+nvme"], and line 658 widens the shared AgenticCodingSearchSpaceEntry.kv_offloading to KVOffloadingConfig (none/dram/nvme/["dram","nvme"]) for ALL topologies. But MultiNodeAgenticMatrixEntry.kv_offloading at line 373 is still…
| NVME_HOST_DIR="" | ||
| cleanup_allocation() { | ||
| local rc=$? | ||
| trap - EXIT INT TERM | ||
| if [[ -n "$NVME_HOST_DIR" ]]; then | ||
| srun --jobid="$JOB_ID" bash -c "rm -rf -- '$NVME_HOST_DIR'" 2>/dev/null || true | ||
| fi | ||
| scancel "$JOB_ID" 2>/dev/null || true | ||
| exit "$rc" | ||
| } | ||
| trap cleanup_allocation EXIT INT TERM | ||
|
|
||
| NVME_CONTAINER_MOUNT="" | ||
| if [[ "${KV_OFFLOADING:-none}" == "nvme" || "${KV_OFFLOADING:-none}" == "dram+nvme" ]]; then | ||
| NVME_HOST_ROOT="/mnt/numa0/enroot/cache/group-$(id -g)" | ||
| NVME_HOST_DIR="$NVME_HOST_ROOT/inferencex-kv-$JOB_ID" | ||
| srun --jobid="$JOB_ID" bash -c "set -e; test -w '$NVME_HOST_ROOT'; mkdir -m 700 '$NVME_HOST_DIR'; findmnt -T '$NVME_HOST_DIR'" | ||
| NVME_CONTAINER_MOUNT=",$NVME_HOST_DIR:/kv-offload" | ||
| export NVME_OFFLOAD_DIR=/kv-offload | ||
| fi | ||
|
|
||
| # flock-serialize the enroot import so concurrent sweep jobs on the same | ||
| # shared NFS path don't race each other into 'File already exists' (race |
There was a problem hiding this comment.
🔴 On the normal success path, the explicit scancel $JOB_ID (line 345) runs before the script exits, and the subsequent EXIT trap fires cleanup_allocation (line 299-306), which tries srun --jobid="$JOB_ID" ... rm -rf on the NVMe scratch dir (line 303) against an allocation that was just cancelled. srun into an already-cancelled/completing allocation fails, and the error is swallowed by 2>/dev/null || true, so cleanup silently no-ops.
Extended reasoning...
Any nvme or dram+nvme sweep job that completes normally hits scancel $JOB_ID at line 345 first, then the EXIT trap's srun-based rm -rf against the now-cancelled job silently fails, leaking the per-job directory under /mnt/numa0/enroot/cache/group-$(id -g)/inferencex-kv-$JOB_ID on every successful run (only the error path, which skips the explicit scancel, cleans up correctly). Over a 30-job sweep with several nvme/dram+nvme rows, this accumulates orphaned directories on the shared NVMe array with no other reaper. Fix: remove the NVMe dir before calling scancel (e.g. do NVMe cleanup then scancel in one place, or drop the separate explicit scancel and rely solely on cleanup_allocation).
Verification: normal. Deterministic ordering defect newly introduced by this diff. With set -e (line 2), on a normally-completing run control reaches line 345 scancel $JOB_ID, cancelling the allocation, before the EXIT trap fires cleanup_allocation. That function's line 303 srun --jobid="$JOB_ID" bash -c "rm -rf -- '$NVME_HOST_DIR'" then launches a new step into the just-cancelled job; Slurm…
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33145888176 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33161613244 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33161907619 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33162065629 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33162065629 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33169766010 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33194649881 |
|
|
||
| scancel $JOB_ID | ||
|
|
||
| fi |
There was a problem hiding this comment.
Offload files persist after allocation ends
High Severity
Allocation teardown now only runs scancel, so host NVMe directories under /mnt/numa0 and vLLM DRAM-tier vllm_offload_*.mmap files in /dev/shm are never removed. Cancelled or timed-out jobs also skip the in-container delete, and leftover ~1 TB mmap files can hang the next exclusive job on that node under memory pressure.
Reviewed by Cursor Bugbot for commit e631356. Configure here.
There was a problem hiding this comment.
The launcher-side cleanup sruns and /dev/shm sweep were removed. On graceful shutdown vLLM unlinks its own mmap, and the benchmark clears only its bind-mounted job-scoped NVMe contents with the bounded in-container command from 1a0793b. The allocation trap itself now does only scancel, so cancellation cannot deadlock behind a second Slurm step.
| stop_background_process_tree "$MOONCAKE_MASTER_PID" "Mooncake master" 30 | ||
| if [[ -n "${NVME_OFFLOAD_DIR:-}" ]]; then | ||
| find "$NVME_OFFLOAD_DIR" -mindepth 1 -delete | ||
| fi |
There was a problem hiding this comment.
NVMe teardown delete can stall jobs
High Severity
The benchmark EXIT trap now deletes NVMe offload contents with an unbounded find -delete. After a successful NVMe or tiered run, that teardown can keep the GitHub job stuck in Launch job script even though result files are already written, delaying or blocking artifact upload.
Reviewed by Cursor Bugbot for commit e631356. Configure here.
There was a problem hiding this comment.
Addressed in 1a0793b: the in-container find -delete now has a 120-second timeout plus a 5-second forced-termination bound. It runs only after the existing shutdown trap has stopped vLLM, inside the original Slurm/container step, so it cannot wait on a competing cleanup srun.
…nvme-agentx # Conflicts: # perf-changelog.yaml
Signed-off-by: Cam Quilici <cjquilici@gmail.com>
Signed-off-by: Cam Quilici <cjquilici@gmail.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33214063365 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33221188566 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33221188566 |
…nvme-agentx # Conflicts: # perf-changelog.yaml
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33431683755 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b52c639. Configure here.
|
|
||
| minimaxm3-fp8-h100-vllm-agentic-mtp: | ||
| image: vllm/vllm-openai:v0.27.1 | ||
| image: ttl.sh/cquil11-vllm-tier-3565dfefe1-pr53087-20260828:24h |
There was a problem hiding this comment.
Official image expires after 24 hours
High Severity
The
Reviewed by Cursor Bugbot for commit b52c639. Configure here.
| " | ||
| NVME_CONTAINER_MOUNT=",$NVME_HOST_DIR:/kv-offload" | ||
| export NVME_OFFLOAD_DIR=/kv-offload | ||
| fi |
There was a problem hiding this comment.
Cancelled jobs leak NVMe cache
Medium Severity
Each NVMe job creates a host directory inferencex-kv-$JOB_ID and never removes old ones. Contents are cleared only inside the container after a graceful vLLM stop, so a cancel or timeout leaves up to 8 TB behind. The next allocation uses a new job id and can fail with no space on /mnt/numa0.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b52c639. Configure here.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33221188566 |
|
Superseded by #2796 because this PR remained attached to a stale branch object after the reduced three-point validation commit was pushed. |
1 participant
Summary
kv-offloading: [dram, nvme]; the vLLM recipe maps it toOffloadingConnectorwithTieringOffloadingSpec, an LRU DRAM primary tier, and an FS secondary tier./mnt/numa0XFS/NVMe array, bind-mount it into the Pyxis container, and clear it after vLLM stops in the benchmark's existing shutdown trap.vllm:prompt_tokens_cached_by_sourceand native tiering counters.Validation
source="cpu"and 480 fromsource="fs"in the new Prometheus counter./dev/shm, sufficient for the recipe's approximately 952 GiB DRAM KV pool.ttl.sh/cquil11-vllm-tier-3565dfefe1-pr53087-20260828:24his published at digestsha256:3a9565253862f2741f417440d21e17681f90c76defcfb271dabb1b2868ce61b6. It contains [Metrics] Expose cached prompt tokens by cache tier cquil11/vllm#2 plus upstream vLLM PR #53087's bounded fallback for indefinitely pending primary-tier writes.source="device"(5,341,056 tokens) for no-offload,source="cpu"(9,245,440) plus device (6,672,384) for Mooncake DRAM, andsource="disk"(12,627,456) plus device (4,278,144) for Simple NVMe. All unused source series remained zero.server_metrics_export.json, proving the new Prometheus series survives AIPerf export.HIT_PENDINGfailure fixed by vLLM PR #53087: 0 running requests, 8 deferred, 4 capacity-waiting, idle GPUs/NVMe, and no progress for over 30 minutes.vLLM metrics PR: cquil11/vllm#2
Note
Medium Risk
Changes H100 Slurm launcher mounts, container lifecycle, and agentic KV offload configuration (including custom vLLM images), which can affect job scheduling and benchmark reproducibility but does not touch auth or production serving paths.
Overview
Adds NVMe-only and DRAM+NVMe tiered KV offload paths to the H100 MiniMax-M3 AgentX benchmark, plus matrix/launcher support so those modes can be swept and attributed via new vLLM Prometheus metrics.
Benchmark & infra:
benchmark_lib.shnow acceptsKV_OFFLOADINGvaluesnvmeanddram+nvme(DRAM capacity checks skip pure NVMe). The H100 recipe wiresnvmetoSimpleCPUOffloadConnectoron a launcher-providedNVME_OFFLOAD_DIRanddram+nvmetoOffloadingConnector/TieringOffloadingSpecwith LRU DRAM and a local FS tier.launch_h100-dgxc-slurm.shprovisions a per-job directory on node NVMe, bind-mounts it into Pyxis, extends the default allocation time, and improves allocation cleanup traps.Matrix: Declarative
kv-offloading: [dram, nvme]is validated and emitted as runtimedram+nvmewith DRAM budget sizing and distinct experiment names. NVMe and multi-tier offload are restricted to single-node agentic entries. Theminimaxm3-fp8-h100-vllm-agentic-mtpconfig switches to a patched vLLM image and a narrow validation sweep (Mooncake DRAM, NVMe-only, and tiered points) instead of the prior wide no-offload/DRAM cliff sweep.Reviewed by Cursor Bugbot for commit cb0fec2. Bugbot is set up for automated code reviews on this repo. Configure here.