[https://nvbugs/6105768][fix] ** Runtime GPU detection inside the test function: when total_memory < 80 GiB - #13471
[https://nvbugs/6105768][fix] ** Runtime GPU detection inside the test function: when total_memory < 80 GiB #13471tensorrt-cicd wants to merge 2 commits into
total_memory < 80 GiB #13471Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughAdds a TinyLlama disaggregated cancellation configuration. The test runner accepts configurable prompt-length ranges and selects test settings from GPU count and memory. The DeepSeek-V3-Lite waiver is removed. ChangesCancellation stress test
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change localizes GPU-dependent test behavior and adds a small-model fallback without any identified merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CancellationTest
participant GPUDetection
participant CancelStressRunner
CancellationTest->>GPUDetection: read GPU count and memory
GPUDetection->>CancellationTest: return available resources
CancellationTest->>CancelStressRunner: run selected model with prompt-length range
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the root cause, fix, affected hardware conditions, fallback configuration, test plan, and bug link. It omits the template's explicit PR checklist, but the core required information is present. Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/defs/disaggregated/test_disaggregated.py (1)
1-1: Consider updating copyright year.The copyright header shows
2022-2024, but since this file is being modified in 2026, it should be updated to2022-2026per coding guidelines. As per coding guidelines: "update year on modified files".📝 Proposed fix
-# SPDX-FileCopyrightText: Copyright (c) 2022-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2022-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/defs/disaggregated/test_disaggregated.py` at line 1, Update the copyright header year range in test_disaggregated.py from "2022-2024" to "2022-2026" so the file reflects the current modification year; locate the SPDX/header comment at the top of the file and change the year substring accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/defs/disaggregated/test_disaggregated.py`:
- Line 1: Update the copyright header year range in test_disaggregated.py from
"2022-2024" to "2022-2026" so the file reflects the current modification year;
locate the SPDX/header comment at the top of the file and change the year
substring accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5baeccdf-3fcb-478d-926d-8942b9c81d28
📒 Files selected for processing (3)
tests/integration/defs/disaggregated/test_configs/disagg_config_cancel_stress_test_small.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
a0728f1 to
24110eb
Compare
24110eb to
d23214d
Compare
c9c14f2 to
92ebad0
Compare
92ebad0 to
4f684d0
Compare
7b9048b to
0adb238
Compare
0adb238 to
d19b782
Compare
d19b782 to
f38489b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/integration/defs/disaggregated/test_configs/disagg_config_cancel_stress_test_small.yaml`:
- Line 1: Add the repository-standard NVIDIA copyright header at the beginning
of the configuration file before the hostname entry, using 2026 as the latest
meaningful modification year.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1430ad7f-6db5-4ef7-9f0d-00106408abbc
📒 Files selected for processing (3)
tests/integration/defs/disaggregated/test_configs/disagg_config_cancel_stress_test_small.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/defs/disaggregated/test_disaggregated.py
| @@ -0,0 +1,39 @@ | |||
| hostname: localhost | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required NVIDIA copyright header.
This new file starts with hostname and has no NVIDIA copyright header. Add the repository-standard header before Line 1 and use 2026 as the latest meaningful modification year.
As per coding guidelines, all new files matching **/* must include the NVIDIA copyright header with the year of the latest meaningful modification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/integration/defs/disaggregated/test_configs/disagg_config_cancel_stress_test_small.yaml`
at line 1, Add the repository-standard NVIDIA copyright header at the beginning
of the configuration file before the hostname entry, using 2026 as the latest
meaningful modification year.
Source: Coding guidelines
f38489b to
489cf2a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
489cf2a to
885397f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…M on L40S The test_disaggregated_cancel_large_context_requests test fails with OOM on L40S (44.4 GiB) because DeepSeek-V3-Lite bf16 requires ~37 GiB per disaggregated worker, and two workers sharing a single GPU need ~74 GiB. Add runtime GPU memory detection to fall back to TinyLlama with a smaller config on single-GPU systems with <80 GiB memory. This preserves the test's cancellation stress-test coverage while fitting within L40S memory constraints. On H100 or multi-GPU systems, the original DeepSeek-V3-Lite bf16 model is still used. Also adds a TinyLlama-compatible disagg config with conservative memory fractions to prevent KV cache allocation races on shared GPUs, and removes the test waiver from waives.txt. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com> Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
885397f to
9111360
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ruodil
left a comment
ruodil
left a comment
There was a problem hiding this comment.
Review summary - CONCERNS
Verdict: Mechanically mergeable (mergeable: true, mergeable_state: blocked - i.e. waiting on required checks/approvals, not on conflicts), but I would not merge as-is: the memory guard mis-classifies at least one common runner shape, and the fix trades a waived test for a silently weaker test that keeps the same node id.
Issues
- [MAJOR]
tests/integration/defs/disaggregated/test_disaggregated.py:4085- 80 GiB threshold mis-fires on single-GPU H100;and num_gpus < 2leaves multi-small-GPU nodes on the OOM path - [MAJOR]
tests/integration/defs/disaggregated/test_disaggregated.py:4090- model swapped silently under the[DeepSeek-V3-Lite-bf16]id while the waive is removed - [MINOR]
tests/integration/defs/disaggregated/test_disaggregated.py:4033-run_cancel_stress_testcalled with aprompt_len_rangekwarg whose signature change is not in this diff - [MINOR]
tests/integration/defs/disaggregated/test_disaggregated.py:4082-get_device_properties(0)without anis_available()guard - [NIT]
tests/integration/defs/disaggregated/test_disaggregated.py:4096- default prompt range duplicated - [NIT]
tests/integration/defs/disaggregated/test_configs/disagg_config_cancel_stress_test_small.yaml:1- missing copyright header (bot comment still open)
QA view
- Test coverage: partial - this is a test-only change. The new TinyLlama branch runs only on small single-GPU nodes and the DeepSeek branch only on >=80 GiB or multi-GPU nodes. Still uncovered: DeepSeek-V3-Lite bf16 cancel-during-prefill with 2000-8000 token prompts on any <80 GiB single-GPU runner, and nothing asserts or logs which branch ran.
- SM coverage: architecture-independent - no
get_sm_version(), arch guard, fp8 or nvfp4 path. The real matrix here is node topology (single small GPU / multi small GPU / 80 GiB+), and that matrix is only partly handled. - Test code: same pytest id for two different models and two different prompt scales; magic
80compared againsttotal_memory(an 80 GB H100 reports ~79.6 GiB); nois_available()guard; no log of the chosen model/config;(2000, 8000)duplicated in signature and caller; new yaml has no copyright header. - Test time: small - one waived integration test is re-enabled (server start with
server_start_timeout=1200, 5 bursts x 32 requests); the fallback path is cheaper than the original. No timeout raised, no new parametrisation. Wall-clock is not measurable from a diff. - Needs
/qa-verify: yes - a waive is removed while nvbugs/6105768 is stillOpen issue, and the replacement test does not reproduce the original OOM configuration. Please run the id on the L40S runner from the bug and on a single-GPU 80 GB H100, and confirm from the log which model each run loaded.
Does this actually fix nvbugs/6105768?
The bug is test_disaggregated_cancel_large_context_requests is OOM on L40S (disposition: Open issue). The diff does not make the DeepSeek-V3-Lite bf16 disagg run fit in 44.4 GiB; it detects total_memory < 80 GiB and device_count < 2 and runs TinyLlama-1.1B with cancel_stress_test_small and 200-800 token prompts instead. Partial. On a genuinely single-GPU L40S runner the OOM can no longer happen. Still uncovered: (a) if the failing runner exposes >=2 GPUs the guard never fires and the ~37 GiB workers start exactly as before - I cannot tell from the diff how the launcher pins the two workers to devices, so I cannot confirm this is safe; (b) even where the guard fires, the DeepSeek large-context cancellation path is now simply not run anywhere on that machine, while the waive that made this visible is deleted.
Possible new issues
- Single-GPU H100/H200:
torchreports ~79.6 GiB for an 80 GB H100, so< 80is true and the test silently downgrades to TinyLlama on hardware that can run the real model. - Nodes with several small GPUs (4x L40S, 2x A10) skip the guard because of the
and num_gpus < 2conjunction and keep the old memory profile. - Re-enabling the test while the bug is open puts a potential live OOM back into the queue on any runner the guard misses.
- Generation server sets
cuda_graph_config.max_batch_size: 64withrequests_per_burst=32andmax_num_tokens: 2048on a shared GPU - a new, untested combination on top of the 0.2/0.3 KV fractions.
What I could not verify
- The signature of
run_cancel_stress_test- it is not in the diff, so I cannot confirm it acceptsprompt_len_range. - Whether
<llm_models_root>/llama-models-v2/TinyLlama-1.1B-Chat-v1.0exists on the runners, and whethersetup_model_symlinkhandles the nested link nameTinyLlama/TinyLlama-1.1B-Chat-v1.0. - How disagg workers are assigned to devices, i.e. whether the multi-GPU branch really gives each worker its own GPU.
- The GPU count of the L40S runner that produced nvbugs/6105768, and the CI list membership claimed in the description (
l0_dgx_h100.yml,qa/llm_function_core.txt) - neither file is in the diff. - Any actual run result: I reasoned from the diff only and ran no tests.
Automated review by NVCortex Lite, run by @ruodil.
| gpu_mem_gib = torch.cuda.get_device_properties(0).total_memory / (1024**3) | ||
| num_gpus = torch.cuda.device_count() | ||
|
|
||
| if gpu_mem_gib < 80 and num_gpus < 2: |
There was a problem hiding this comment.
[MAJOR] 80 GiB threshold mis-fires on single-GPU H100; and num_gpus < 2 leaves multi-small-GPU nodes on the OOM path
gpu_mem_gib is derived from torch.cuda.get_device_properties(0).total_memory, which is the usable framebuffer, not the marketing capacity: an 80 GB H100 reports ~79.6 GiB (85520809984 / 1024**3) and an H200 141 GB reports ~139.7 GiB. So on a single-GPU H100 node — the exact platform the PR description says is fine — 79.6 < 80 and num_gpus < 2 are both true and the test silently degrades to TinyLlama even though memory is ample. Conversely the and makes the guard weaker than the diagnosis: a node with 2-4 small GPUs (4x L40S 44.4 GiB, 2x A10 24 GiB) skips the guard entirely and still launches the ~37 GiB DeepSeek workers, so if the L40S runner that filed nvbugs/6105768 has more than one GPU the OOM is unchanged. Express the guard in terms of what the model actually needs per GPU instead of a round number, and use or semantics for the small-GPU case:
| if gpu_mem_gib < 80 and num_gpus < 2: | |
| # DeepSeek-V3-Lite bf16 needs ~37 GiB of weights per disagg worker. | |
| DEEPSEEK_WEIGHTS_GIB = 37 | |
| workers_per_gpu = 1 if num_gpus >= 2 else 2 | |
| if gpu_mem_gib < DEEPSEEK_WEIGHTS_GIB * workers_per_gpu * 1.15: |
Also note total_memory of device 0 is taken as representative of the whole node; on a heterogeneous host the smallest device is the one that matters.
| "TinyLlama-1.1B-Chat-v1.0") | ||
| setup_model_symlink(llm_venv, model_path, | ||
| "TinyLlama/TinyLlama-1.1B-Chat-v1.0") | ||
| test_desc = "cancel_stress_test_small" |
There was a problem hiding this comment.
[MAJOR] Model is swapped silently while the test id stays [DeepSeek-V3-Lite-bf16], and the waive is removed — green CI no longer means the DeepSeek path passed
On the fallback branch the test runs TinyLlama-1.1B with cancel_stress_test_small and 200-800 token prompts, but the pytest node id is still test_disaggregated_cancel_large_context_requests[DeepSeek-V3-Lite-bf16] (the param comes from the fixture) and the waive for that exact id is deleted in tests/integration/test_lists/waives.txt:79. Concrete consequence: on any node the guard fires, CI reports that id as PASSED while neither the DeepSeek-V3-Lite bf16 model nor a 'large context' (the whole point of the test - 800 tokens with max_seq_len: 2048 is not a large context) was ever exercised; a future regression in cancel-during-prefill under real large contexts will be reported green on that machine. Nothing in the diff records which branch was taken, so a CI log reader cannot tell the two runs apart. Two things would make this honest: emit the choice, and make the reduced run distinguishable, e.g. pytest.skip(...) on undersized nodes and a separate parametrised TinyLlama case, or at minimum
print(f"[cancel_stress] {gpu_mem_gib:.1f} GiB x {num_gpus} GPU(s) -> "
f"model={model_path} desc={test_desc} prompts={prompt_len_range}")so the substitution shows up in the test output.
| num_bursts=num_bursts, | ||
| requests_per_burst=requests_per_burst) | ||
| requests_per_burst=requests_per_burst, | ||
| prompt_len_range=prompt_len_range) |
There was a problem hiding this comment.
[MINOR] run_cancel_stress_test is called with a new prompt_len_range kwarg but its signature is not shown in this diff
The diff only widens the wrapper run_disaggregated_cancel_test (line 4006); it never touches the definition of run_cancel_stress_test, yet that call now passes prompt_len_range=prompt_len_range. If that helper does not already accept the keyword, every invocation - including the unchanged cancel_stress_test_large caller that relies on the default - dies with TypeError: run_cancel_stress_test() got an unexpected keyword argument 'prompt_len_range' before a single request is sent, i.e. the re-enabled test fails everywhere rather than only on small GPUs. The PR summary says the range was 'added to the cancellation test helper', which is ambiguous about which of the two functions changed. Please confirm the helper already takes prompt_len_range (and with the same default), otherwise add it in this PR.
| setup_model_symlink(llm_venv, deepseek_v3_model_root, | ||
| "DeepSeek-V3-Lite/bf16") | ||
| import torch | ||
| gpu_mem_gib = torch.cuda.get_device_properties(0).total_memory / (1024**3) |
There was a problem hiding this comment.
[MINOR] torch.cuda.get_device_properties(0) raises before any assertion when CUDA is unavailable
If the test is collected and started on a host where CUDA init fails or no device is visible (driver hiccup, CUDA_VISIBLE_DEVICES= leaked from a previous case), this line raises RuntimeError: No CUDA GPUs are available / AssertionError from the middle of the test body, which surfaces as a hard test error with no hint that it is an environment problem rather than a disagg failure. Guard it and skip explicitly:
if not torch.cuda.is_available():
pytest.skip("CUDA device required for disaggregated cancel stress test")| model_path = deepseek_v3_model_root | ||
| setup_model_symlink(llm_venv, model_path, "DeepSeek-V3-Lite/bf16") | ||
| test_desc = "cancel_stress_test" | ||
| prompt_len_range = (2000, 8000) |
There was a problem hiding this comment.
[NIT] Default prompt range duplicated between the helper signature and the caller
prompt_len_range=(2000, 8000) is now written twice: as the default on line 4006 and literally here on the DeepSeek branch. If the default is ever tuned, the DeepSeek path silently keeps the old numbers and the two callers of the helper diverge. Drop the literal here and let the default apply (or pass the same named constant in both places).
| @@ -0,0 +1,39 @@ | |||
| hostname: localhost | |||
There was a problem hiding this comment.
[NIT] New file has no NVIDIA copyright header
CodeRabbit raised this on 2026-08-05 and it is still unaddressed on head 9111360. If the repo's header policy covers test config files, prepend the standard SPDX header with 2026 as the modification year; if sibling disagg_config_*.yaml files carry no header either, reply to close the bot comment so it stops re-appearing on every rescan.
ruodil
left a comment
ruodil
left a comment
There was a problem hiding this comment.
Review summary - Approve (non-blocking)
Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.
Worth doing before this is relied on: A waive is removed for a bug still marked 'Open issue', the 'fix' substitutes a different model instead of reproducing the original OOM configuration, and the guard's classification of real runners (single-GPU H100 reporting ~79.6 GiB; multi-GPU L40S nodes) cannot be settled from the diff. QA should run the test id on the L40S runner from nvbugs/6105768 and on a single-GPU 80 GB H100, and confirm from the logs which branch and which model each run actually used, plus that run_cancel_stress_test accepts prompt_len_range.
Automated review by NVCortex Lite, run by @ruodil.
7 participants
Summary
model._apply(init_meta_tensor). The test was designed for H100 (80 GiB) with workers on separate GPUs but had no memory guard.total_memory < 80 GiBanddevice_count < 2, fall back to TinyLlama-1.1B-Chat-v1.0 (~2 GiB) with a dedicated small-model config (disagg_config_cancel_stress_test_small.yaml) that uses conservativefree_gpu_memory_fractionvalues (0.2/0.3 vs 0.3/0.85) to prevent KV cache allocation races on shared GPUs. The parametrize ID[DeepSeek-V3-Lite-bf16]is preserved since it comes from the fixture parameter, not the actual model loaded. On H100 or multi-GPU systems, the original DeepSeek-V3-Lite bf16 path is unchanged.Test plan
Links
Summary by CodeRabbit
Dev Engineer Review
nvbugs/6105768reference.QA Engineer Review
test_disaggregated_cancel_large_context_requests.run_disaggregated_cancel_testto accept configurable prompt-length ranges.cancel_stress_test_smallconfiguration.disaggregated/test_disaggregated.py::test_disaggregated_cancel_large_context_requests[DeepSeek-V3-Lite-bf16].tests/integration/test_lists/test-db/l0_dgx_h100.ymltests/integration/test_lists/qa/llm_function_core.txt