[CI] Migrate tone_tests to CUDA 13: pull cu130 wheel, adapt sglang/vllm paths, add NCCL env & offline model cache#2811
[CI] Migrate tone_tests to CUDA 13: pull cu130 wheel, adapt sglang/vllm paths, add NCCL env & offline model cache#2811luketong777 wants to merge 10 commits into
Conversation
…talling wrong CUDA wheel
…oncake proxy path
…lear residual memory
…/ERDMA state (keeps deps)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces GPU memory draining mechanisms and offline HuggingFace cache checks to prevent out-of-memory errors and rate-limiting issues during test execution. It also corrects several test directory paths and updates configurations for vLLM and SGLang servers. Feedback suggests using awk instead of tr when parsing nvidia-smi output to avoid accidentally killing unrelated processes, and making the hardcoded CUDA_VISIBLE_DEVICES configurable to improve test suite portability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| force_kill_gpu_procs() { | ||
| command -v nvidia-smi >/dev/null 2>&1 || return 0 | ||
| local pids | ||
| pids=$(nvidia-smi --query-compute-apps=pid --format=csv,noheader 2>/dev/null | tr -cd '0-9\n' | grep -E '^[0-9]+$' | sort -u) |
There was a problem hiding this comment.
Using tr -cd '0-9\n' to strip all non-digit characters from the entire output of nvidia-smi is highly risky. If nvidia-smi outputs any warning or informational messages containing numbers (such as driver versions, GPU temperatures, or bus IDs), those numbers will be mangled into digits, matched as valid PIDs, and subsequently force-killed. This can lead to the accidental termination of critical system or unrelated user processes on a shared host.
Using awk to extract only fields that are strictly numeric is a much safer and more robust approach.
| pids=$(nvidia-smi --query-compute-apps=pid --format=csv,noheader 2>/dev/null | tr -cd '0-9\n' | grep -E '^[0-9]+$' | sort -u) | |
| pids=$(nvidia-smi --query-compute-apps=pid --format=csv,noheader 2>/dev/null | awk '$1 ~ /^[0-9]+$/ {print $1}' | sort -u) |
| local extra_args="--tensor-parallel-size 2 --max-model-len 32768 --gpu-memory-utilization 0.85 --no-enable-prefix-caching --kv-transfer-config '$kv_config_json'" | ||
|
|
||
| local env_vars="CUDA_VISIBLE_DEVICES=0,1" | ||
| local env_vars="CUDA_VISIBLE_DEVICES=6,7" |
There was a problem hiding this comment.
Hardcoding CUDA_VISIBLE_DEVICES=6,7 makes the test suite non-portable and prone to failure on environments with fewer than 8 GPUs, or where those specific GPUs are already occupied.
Consider allowing this to be overridden via an environment variable, defaulting to 6,7 if not specified.
| local env_vars="CUDA_VISIBLE_DEVICES=6,7" | |
| local env_vars="CUDA_VISIBLE_DEVICES=${CUDA_VISIBLE_DEVICES:-6,7}" |
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Review Summary
This PR does a solid job modernizing the tone_tests CI flow for CUDA 13 and the upgraded sglang/vLLM images: the artifact selection fix, NCCL_GIN_TYPE injection, and HF offline helper all move the system toward more reliable, deterministic runs, and the GPU drain/reset helpers aim to keep long run-all sequences stable.
🛡️ Key Risks & Issues
- GPU drain and host-wide kill behavior: The new drain_gpu_local()/drain_gpu_between_tests path in scripts/tone_tests/scripts/common.sh restarts the container and, if memory doesn’t fall below the threshold, calls force_kill_gpu_procs(), which uses nvidia-smi to enumerate all GPU-compute PIDs on the host and sends kill -9 to each. This runs after every run-all case on both local and remote nodes. On truly dedicated CI nodes that might be acceptable, but on shared hosts or multi-tenant environments this will silently terminate unrelated GPU workloads and can look like a denial-of-service. Even on dedicated nodes, the lack of scoping to the Mooncake container/user makes debugging unexpected kills hard.
- GPU drain fallbacks and flakiness risk: drain_gpu_local() treats docker restart and force_kill_gpu_procs as best-effort and proceeds even when GPU memory remains high, only logging a warning. Combined with run-all reuse of the same container, that can leave partial GPU occupancy for the next case and manifest as intermittent OOMs, NCCL hangs, or SIGKILLs rather than a clean early failure when the environment cannot be reset.
- HF offline cache detection brittleness: hf_offline_prefix() in scripts/tone_tests/scripts/common.sh infers “cached” by checking for
/root/.cache/huggingface/hub/models--<model>/snapshots/*/config.jsonand printsHF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1when found, which is then wired into launch_sglang_server(), launch_vllm_server(), test_moe_mooncake.sh, and test_disaggregation_different_tp.sh. This is directionally good (only enable offline when a snapshot exists), but it assumes a specific HF cache layout and equates the presence of config.json with a fully usable snapshot. Partial or stale snapshots, or future layout changes, could flip tests into offline mode and fail in non-obvious ways or, conversely, silently revert tests back to online behavior when we thought they were pinned to cache. - vLLM GPU layout assumptions: test_vllm_1p1d_erdma.sh now hardcodes
CUDA_VISIBLE_DEVICES=6,7and adds--gpu-memory-utilization 0.85. That looks tuned for a particular CI host topology; on different hardware (fewer GPUs, different indexing, or shared use of devices 6/7) this can cause startup failures or contention with other jobs.
🧪 Verification Advice
- Run
run_test.sh run-all(SGLANG and VLLM) on a dedicated multi-GPU CI node and confirm that drain_gpu_between_tests resets both local and remote nodes cleanly, with wait_gpu_idle() completing without hitting force_kill_gpu_procs in normal operation. - Exercise the GPU drain helpers on hosts both with and without nvidia-smi (or where nvidia-smi fails) to validate that the early-return paths behave as expected and do not attempt host-wide kills in unsupported environments.
- On a staging node, intentionally leave an unrelated GPU workload running while executing tone_tests run-all to observe whether force_kill_gpu_procs terminates it; use this to decide whether the operational assumption “dedicated CI nodes only” is acceptable or whether the kill logic needs to be scoped or downgraded to a hard failure.
- Verify HF offline behavior in both cache-present and cache-absent scenarios: pre-populate the HF cache for the relevant models and confirm that sglang/vLLM servers and pytest runs stay fully offline; then clear/alter the cache and check that tests either fail clearly when offline mode is misapplied or explicitly fall back to online behavior in a controlled way.
- Validate the vLLM proxy path change and version gating by running test_vllm_1p1d_erdma.sh on images with vLLM >= 0.16.0 (new examples/disaggregated/mooncake_connector proxy) and < 0.16.0 (toy_proxy_server), confirming that both paths start, report readiness, and complete the chat request successfully.
- Confirm that the CI GPU hosts actually provide devices 6 and 7 for Mooncake tone_tests and that running the ERDMA test with
CUDA_VISIBLE_DEVICES=6,7and--gpu-memory-utilization 0.85is stable (no OOMs or contention with other jobs). - In GitHub Actions, validate that integration-test.yml now reliably picks the cu130 py312 wheel when both cu12 and cu13 artifacts exist and that the failure mode when no cu130 artifact is present is clear and actionable.
💡 Thoughts & Suggestions
- For the GPU drain path, consider either scoping force_kill_gpu_procs to processes launched by this harness (e.g., cgroup/container tagging, user-based filters), or treating “GPU did not drain within timeout” as a hard failure for the test suite rather than a reason to kill arbitrary GPU workloads on the host.
- It might be worth adding slightly richer logging around hf_offline_prefix (e.g., logging which models are considered cached and when offline mode is enabled) to make cache-related CI behavior easier to debug, and possibly tightening the detection to check for a known weights file in addition to config.json.
- The vLLM ERDMA test’s CUDA_VISIBLE_DEVICES=6,7 and gpu-memory-utilization 0.85 settings could be parameterized or derived from CI configuration so they don’t quietly assume a particular topology; even a short comment or config variable referencing the CI layout would help future maintainers understand the constraint.
- Overall, the direction of the PR is good: it’s tackling real CI stability issues for CUDA 13 and HF rate limiting. With some tightening around the GPU kill behavior and cache detection, it should give you the robustness you’re aiming for without surprising other workloads or future operators.
🤖 Generated by Qoder • View workflow run
| fi | ||
| } | ||
|
|
||
| # Echo an offline env prefix when the given model already exists in the |
There was a problem hiding this comment.
The HuggingFace offline helper and its call sites look generally safe, but there are a couple of behavioral edge cases worth tightening or at least documenting.
hf_offline_prefix() in scripts/tone_tests/scripts/common.sh builds a cache_dir name of models--<model> and then checks for /root/.cache/huggingface/hub/${cache_dir}/snapshots/*/config.json via docker_exec; if that file exists, it prints HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1 . The prefix is then injected into:
- launch_sglang_server() (as
${offline_prefix}python -m sglang.launch_server ...), - launch_vllm_server() (appended to env_prefix), and
- pytest calls in test_moe_mooncake.sh and test_disaggregation_different_tp.sh.
This is good in that offline mode is only enabled when a cache snapshot is detected, but it assumes that:
- The HF cache layout always uses
models--<name>/snapshots/*/config.json, and - The presence of config.json implies a fully usable snapshot (weights, tokenizer, etc.).
If the cache structure changes, or if a partial snapshot exists (config.json present but other files missing), we might flip into offline mode and end up with non-obvious startup failures that are hard to debug from CI logs. On the flip side, if the snapshot is valid but located differently (or config.json is absent for some reason), the tests will unexpectedly go online again and hit the hub/mirror, reintroducing the 429 risk this PR is trying to avoid.
Would it make sense to either (a) harden the detection (e.g., also checking for weights or a known file) and log when offline mode is enabled, or (b) treat missing cache as a more explicit failure for these tone tests so we don’t silently flip back to online behavior?
🤖 Generated by Qoder • Fix in Qoder
| return 1 | ||
| } | ||
|
|
||
| # Force-kill every host process still holding GPU memory. This catches leftovers |
There was a problem hiding this comment.
The new GPU drain helpers added around wait_gpu_idle(), force_kill_gpu_procs(), and drain_gpu_local()/drain_gpu_between_tests in scripts/tone_tests/scripts/common.sh look like they assume the CI node is fully dedicated to Mooncake tests, but the host-wide kill behavior is pretty aggressive.
In drain_gpu_local(), if GPU memory has not dropped below the threshold after a container restart, the code calls force_kill_gpu_procs(), which uses nvidia-smi --query-compute-apps=pid to enumerate all GPU-holding PIDs on the host and then sends kill -9 to each of them. drain_gpu_between_tests() calls drain_gpu_local for every case in run_all_tests(), and also invokes it on the remote node via SSH.
On a shared CI host or any environment where other GPU workloads run alongside tone_tests, this will repeatedly and silently terminate unrelated jobs (training runs, other services, etc.), which is a pretty big safety/availability risk. There’s no scoping to the Mooncake container, user, or any job-specific tag; the only filter is "uses GPU compute".
If the operational assumption really is "these nodes are single-tenant and reserved for Mooncake CI", it would be helpful to either codify that constraint or tighten the kill logic so it only targets processes launched by this test harness (e.g., via container PID namespace, cgroups, or a known user). Otherwise, it might be safer to treat "GPU did not drain within timeout" as a hard failure for the suite rather than attempting to clear arbitrary GPU workloads on the host.
🤖 Generated by Qoder • Fix in Qoder
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Description
Summary
Migrate the scripts/tone_tests integration test suite to the CUDA 13 runtime and fix a series of issues surfaced by the sglang/vllm image upgrade, so run-all runs reliably on CUDA 13. All changes are confined to scripts/tone_tests/ and .github/workflows/integration-test.yml.
What & Why
1. Inject NCCL_GIN_TYPE=0 into the test container
The test container requires NCCL_GIN_TYPE=0. Injected it via -e NCCL_GIN_TYPE=0 in setup_node_env when assembling the container run args (applies to both local and remote nodes).
2. CI must pull the CUDA 13 (cu130) wheel after the sglang/vllm upgrade
The runtime image was upgraded to CUDA 13, but the integration test picked the CUDA 12 wheel, causing ImportError: libcudart.so.12 at runtime.
integration-test.yml: changed the artifact filter from contains("cu130") | not to contains("cu130") so it selects the CUDA 13 build.
common.sh (get_whl): fixed a quoted-glob bug (rm -f "$whls_path/.whl" → rm -f "$whls_path"/.whl) that left stale wheels behind. With both cu12 and cu13 wheels present, the head -n 1 selection alphabetically picked the wrong (cu12) wheel; proper cleanup ensures only the intended wheel is installed.
3. Adapt to path changes introduced by the sglang/vllm upgrade
sglang test path: .../test/registered/distributed was renamed to .../test/registered/disaggregation in the new image — updated test_disaggregation_different_tp.sh.
vLLM proxy path: in vLLM 0.24.0 the mooncake proxy moved from examples/online_serving/disaggregated_serving/mooncake_connector/... to examples/disaggregated/mooncake_connector/mooncake_connector_proxy.py — updated test_vllm_1p1d_erdma.sh.
Also fixed run_single_test to create the log directory (previously only run_all_tests did), avoiding tee: No such file or directory.
4. Model cache offline detection (avoid hf-mirror.com 429 startup failures)
Models are already cached on both nodes, but vllm/sglang still queried the hub at startup and got throttled by hf-mirror.com (HTTP 429), causing server launch failures. Added a hf_offline_prefix helper in common.sh that detects whether a model already exists in the local HF cache (/root/.cache/huggingface/hub/models--/snapshots//config.json) and, if so, injects HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1. Wired into launch_sglang_server, launch_vllm_server, and the moe / disaggregation pytest commands.
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
https://tone.openanolis.cn/ws/gclfnh19/test_result/229741
Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure