Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
hostname: localhost

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

model: TinyLlama/TinyLlama-1.1B-Chat-v1.0
backend: pytorch
enable_autotuner: false
context_servers:
disable_overlap_scheduler: true
num_instances: 1
tensor_parallel_size: 1
pipeline_parallel_size: 1
max_num_tokens: 2048
max_seq_len: 2048
enable_chunked_prefill: true
kv_cache_config:
enable_block_reuse: true
enable_partial_reuse: true
free_gpu_memory_fraction: 0.2
cache_transceiver_config:
backend: DEFAULT
max_tokens_in_buffer: 2048
cuda_graph_config:
enable_padding: true
max_batch_size: 1
generation_servers:
num_instances: 1
tensor_parallel_size: 1
pipeline_parallel_size: 1
max_num_tokens: 2048
max_seq_len: 2048
enable_chunked_prefill: true
kv_cache_config:
enable_block_reuse: true
enable_partial_reuse: true
free_gpu_memory_fraction: 0.3
cache_transceiver_config:
backend: DEFAULT
max_tokens_in_buffer: 2048
cuda_graph_config:
enable_padding: true
max_batch_size: 64
35 changes: 29 additions & 6 deletions tests/integration/defs/disaggregated/test_disaggregated.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,8 @@ def get_test_config(test_desc, example_dir, test_root):
f"{test_configs_root}/disagg_config_ctxtp2_gentp2_gptoss_tllm.yaml",
"cancel_stress_test":
f"{test_configs_root}/disagg_config_cancel_stress_test.yaml",
"cancel_stress_test_small":
f"{test_configs_root}/disagg_config_cancel_stress_test_small.yaml",
"qwen3_8b":
f"{test_configs_root}/disagg_config_ctxtp2_gentp2_qwen3_8b.yaml",
"mamba_conc_greater_than_mbs":
Expand Down Expand Up @@ -3988,7 +3990,8 @@ def run_disaggregated_cancel_test(example_dir,
requests_per_burst=64,
server_start_timeout=1200,
model_path=None,
cwd=None):
cwd=None,
prompt_len_range=(2000, 8000)):
"""Run disaggregated test with request cancellation stress test."""
cleanup_output_files()
run_env = env.copy()
Expand All @@ -4014,7 +4017,8 @@ def run_disaggregated_cancel_test(example_dir,
# Run the cancel stress test
run_cancel_stress_test(server_url,
num_bursts=num_bursts,
requests_per_burst=requests_per_burst)
requests_per_burst=requests_per_burst,
prompt_len_range=prompt_len_range)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


# Create a temporary client config with the correct dynamic port
client_config = config.copy()
Expand Down Expand Up @@ -4057,16 +4061,35 @@ def test_disaggregated_cancel_large_context_requests(disaggregated_test_root,

This test sends bursts of requests with large contexts and cancels them
during prefill to stress test resource cleanup.

DeepSeek-V3-Lite bf16 (~37 GiB) requires two disagg workers on separate
GPUs. On single-GPU systems with <80 GiB, fall back to TinyLlama to avoid
OOM while still exercising the cancellation code path.
"""
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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")

num_gpus = torch.cuda.device_count()

if gpu_mem_gib < 80 and num_gpus < 2:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
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.

model_path = os.path.join(llm_models_root(), "llama-models-v2",
"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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

prompt_len_range = (200, 800)
else:
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).


run_disaggregated_cancel_test(disaggregated_example_root,
"cancel_stress_test",
test_desc,
env=llm_venv._new_env,
num_bursts=5,
requests_per_burst=32,
model_path=deepseek_v3_model_root,
model_path=model_path,
prompt_len_range=prompt_len_range,
cwd=llm_venv.get_working_directory())


Expand Down
1 change: 0 additions & 1 deletion tests/integration/test_lists/waives.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ accuracy/test_llm_api_pytorch_multimodal.py::TestMistralLarge3_675B::test_nvfp4_
cpp/test_multi_gpu.py::test_cache_transceiver[8proc-mooncake_kvcache-90] SKIP (https://nvbugs/5838199)
cpp/test_multi_gpu.py::test_cache_transceiver[8proc-nixl_kvcache-90] SKIP (https://nvbugs/5838199)
cpp/test_multi_gpu.py::test_cache_transceiver[8proc-ucx_kvcache-90] SKIP (https://nvbugs/5838199)
disaggregated/test_disaggregated.py::test_disaggregated_cancel_large_context_requests[DeepSeek-V3-Lite-bf16] SKIP (https://nvbugs/6105768)
disaggregated/test_disaggregated.py::test_disaggregated_deepseek_v3_lite_bf16_cache_aware_balance[DeepSeek-V3-Lite-bf16] SKIP (https://nvbugs/6162322)
disaggregated/test_disaggregated.py::test_disaggregated_deepseek_v3_lite_bf16_conditional[DeepSeek-V3-Lite-bf16] SKIP (https://nvbugs/6162322)
disaggregated/test_disaggregated.py::test_disaggregated_deepseek_v3_lite_fp8_attention_dp_gen_only[DeepSeek-V3-Lite-fp8] SKIP (https://nvbugs/6162322)
Expand Down
Loading