[None][feat] adding support for disaggregated multi-instance tests #6674
Conversation
📝 WalkthroughWalkthroughRefactors disaggregated-serving test infrastructure to launch multiple context and generation server instances in parallel, adds a new multi-instance test, and updates test lists/CI entries to include the new tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant MultiPopen
participant ContextServer[i]
participant GenServer[i]
participant DisaggServer
participant Client
Tester->>MultiPopen: build per-instance envs/args, start all ctx/gen servers
MultiPopen->>ContextServer[i]: start context server instance (parallel)
MultiPopen->>GenServer[i]: start generation server instance (parallel)
Tester->>DisaggServer: launch disaggregated server configured with per-instance URLs
Tester->>Client: create client and send requests
Client->>DisaggServer: forward request
DisaggServer->>ContextServer[i]: route context work
DisaggServer->>GenServer[i]: route generation work
DisaggServer-->>Client: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/integration/defs/accuracy/test_disaggregated_serving.py (3)
118-123: LGTM with a minor suggestion for robustness.The URL and port extraction logic works for the expected format. Consider using
urllib.parsefor more robust URL parsing if the URL formats become more complex in the future.
167-182: Consider removing the empty finally block.The multi_popen context manager is well-structured for managing multiple processes, but the empty
finallyblock (line 181) serves no purpose and should be removed for cleaner code.- finally: - pass
183-237: Address unused variable assignments.The variables
server_processes(line 184) anddisaggregated_server(line 189) are assigned but never used, as flagged by static analysis. Either use these variables for monitoring/logging purposes or remove the assignments to clean up the code.If the variables are not needed, apply this diff:
- with multi_popen(ctx_servers + gen_servers) as server_processes: + with multi_popen(ctx_servers + gen_servers): with popen([ trtllm_serve_path, "disaggregated", "-c", disaggregated_serving_config_path, "--server_start_timeout", "3600" - ]) as disaggregated_server: + ]):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/defs/accuracy/test_disaggregated_serving.py(2 hunks)tests/integration/test_lists/qa/llm_function_full.txt(1 hunks)tests/integration/test_lists/test-db/l0_dgx_h100.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile = ...).
Python global variables should use upper snake_case and prefix G (e.g., G_MY_GLOBAL = ...).
Python constants should use upper snake_case (e.g., MY_CONSTANT = ...).
Avoid shadowing variables declared in an outer scope in Python.
Initialize all externally visible members of a class in the constructor in Python.
For interfaces that may be used outside a file, prefer docstrings over comments in Python.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for classes and functions in Python, which can be parsed by Sphinx.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the docstring for the class.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
📚 Learning: in tensorrt-llm, test files (files under tests/ directories) do not require nvidia copyright headers...
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Applied to files:
tests/integration/test_lists/qa/llm_function_full.txt
📚 Learning: in tensorrt-llm testing, it's common to have both cli flow tests (test_cli_flow.py) and pytorch api ...
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/test_lists/qa/llm_function_full.txttests/integration/test_lists/test-db/l0_dgx_h100.ymltests/integration/defs/accuracy/test_disaggregated_serving.py
🧬 Code Graph Analysis (1)
tests/integration/defs/accuracy/test_disaggregated_serving.py (6)
tests/integration/defs/trt_test_alternative.py (1)
popen(179-198)tests/unittest/_torch/auto_deploy/unit/singlegpu/utils/test_config.py (1)
temp_dir(67-70)tests/integration/defs/disaggregated/test_workers.py (2)
send_request(92-107)send_request(229-236)tensorrt_llm/llmapi/llm.py (4)
prompt(79-80)RequestOutput(47-85)_from_generation_result(66-76)generate_async(307-441)tensorrt_llm/executor/result.py (1)
CompletionOutput(73-132)tests/integration/defs/accuracy/accuracy_core.py (2)
evaluate(146-199)evaluate(678-688)
🪛 Ruff (0.12.2)
tests/integration/defs/accuracy/test_disaggregated_serving.py
184-184: Local variable server_processes is assigned to but never used
Remove assignment to unused variable server_processes
(F841)
189-189: Local variable disaggregated_server is assigned to but never used
Remove assignment to unused variable disaggregated_server
(F841)
🔇 Additional comments (4)
tests/integration/test_lists/qa/llm_function_full.txt (1)
529-529: Duplicate check passed; noTIMEOUTneeded in this sectionI ran a grep over
llm_function_full.txtand confirmed there’s only one entry ofaccuracy/test_disaggregated_serving.py::TestLlama3_1_8BInstruct::test_multi_instanceand that none of the other
accuracy/test_disaggregated_serving.pyentries in this block use inlineTIMEOUTannotations. To keep the convention consistent, you can omit adding a timeout here. Otherwise, this addition looks correct.tests/integration/defs/accuracy/test_disaggregated_serving.py (3)
124-145: LGTM! Well-structured multi-instance context server setup.The implementation correctly handles GPU allocation across multiple context server instances, ensuring no GPU overlap through proper offset tracking. The environment setup and argument construction are appropriate.
146-166: LGTM! Consistent generation server setup pattern.The generation server setup mirrors the context server pattern appropriately, with proper GPU offset continuation to ensure non-overlapping GPU allocation across all server instances.
445-488: LGTM! Well-designed multi-instance test.The new test method effectively validates multi-instance disaggregated serving with:
- Appropriate resource requirements (4 GPUs minimum)
- Minimal parallelism configuration for testing
- Coverage of both MMLU and GSM8K accuracy tasks
- Consistent structure with existing test methods
|
I just tested asymmetric multi-instance (ctx 2 gen 1 and ctx 1 gen 2), also passing. |
|
I've made it easier to add multi-instance tests, they can be added through |
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/defs/accuracy/test_disaggregated_serving.py (2)
118-123: Consider more robust URL parsing.The current URL parsing logic using
url.split(":")[1]is fragile and could fail with malformed URLs or different URL formats.- ctx_ports = [int(url.split(":")[1]) for url in ctx_urls] - gen_ports = [int(url.split(":")[1]) for url in gen_urls] + from urllib.parse import urlparse + + def extract_port(url): + if "://" not in url: + url = "http://" + url # Add scheme for parsing + parsed = urlparse(url) + return parsed.port or (443 if parsed.scheme == "https" else 80) + + ctx_ports = [extract_port(url) for url in ctx_urls] + gen_ports = [extract_port(url) for url in gen_urls]
249-251: Fix line length violation.The line exceeds the 120-character limit. Consider breaking it into multiple lines for better readability.
pytest.fail( - f"Not enough devices for {ctx_instances} ctx instances (ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances (gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}" + f"Not enough devices for {ctx_instances} ctx instances " + f"(ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances " + f"(gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}" )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/defs/accuracy/test_disaggregated_serving.py(3 hunks)tests/integration/test_lists/qa/llm_function_full.txt(1 hunks)tests/integration/test_lists/test-db/l0_dgx_h100.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration/test_lists/test-db/l0_dgx_h100.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_lists/qa/llm_function_full.txt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
📚 Learning: in tensorrt-llm testing, it's common to have both cli flow tests (test_cli_flow.py) and pytorch api ...
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
🪛 Ruff (0.12.2)
tests/integration/defs/accuracy/test_disaggregated_serving.py
181-181: Undefined name logger
(F821)
250-250: Line too long (209 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (5)
tests/integration/defs/accuracy/test_disaggregated_serving.py (5)
124-166: Well-structured multi-instance server setup.The GPU assignment logic with offset calculation ensures proper resource allocation across multiple server instances. The environment and argument preparation is correctly implemented.
186-241: Clean integration of multi-instance server launching.The refactored launch logic effectively combines all server instances using the
multi_popencontext manager while preserving existing health check and client functionality.
276-291: Clean dynamic URL generation.The port assignment strategy effectively prevents conflicts between context and generation server instances by using separate ranges and spacing.
445-445: Backward compatibility maintained.Existing test correctly updated to pass instance counts while maintaining single-instance behavior.
455-459: Well-designed multi-instance test.The new test effectively validates multi-instance functionality with appropriate device requirements and minimal parallelism configuration to isolate the multi-instance behavior.
|
PR_Github #14336 [ run ] triggered by Bot |
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_disaggregated_serving.py (1)
244-251: Well-designed multi-instance support with proper resource validation.The function signature enhancement correctly adds instance count parameters, and the early GPU availability check prevents runtime failures. However, the error message exceeds line length limits.
Apply this diff to fix the line length violation:
- pytest.fail( - f"Not enough devices for {ctx_instances} ctx instances (ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances (gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}" - ) + pytest.fail( + f"Not enough devices for {ctx_instances} ctx instances " + f"(ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances " + f"(gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/defs/accuracy/test_disaggregated_serving.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile).
Python global variables should use upper snake_case and prefix G (e.g., G_MY_GLOBAL).
Python constants should use upper snake_case (e.g., MY_CONSTANT).
Avoid shadowing variables declared in an outer scope in Python.
Initialize all externally visible members of a Python class in the constructor.
For interfaces that may be used outside a Python file, prefer docstrings over comments.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the class docstring.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
📚 Learning: in tensorrt-llm testing, it's common to have both cli flow tests (test_cli_flow.py) and pytorch api ...
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
🪛 Ruff (0.12.2)
tests/integration/defs/accuracy/test_disaggregated_serving.py
250-250: Line too long (209 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (7)
tests/integration/defs/accuracy/test_disaggregated_serving.py (7)
118-123: LGTM! Clean URL and port extraction for multi-instance support.The refactor correctly extracts URLs and ports from the disaggregated server configuration, enabling dynamic multi-instance setups instead of hardcoded single instances.
124-166: Well-implemented GPU allocation and server configuration for multiple instances.The logic properly handles GPU device assignment by incrementally allocating ranges for each server instance, ensuring no GPU conflicts between context and generation servers. The environment variable setup and argument construction are correct.
167-185: Good introduction of multi_popen context manager.The new context manager properly handles the lifecycle of multiple server processes, with appropriate error handling using print statements (correctly addressing the previous logger issue).
186-241: Excellent refactor to support multi-instance disaggregated serving.The main context manager now correctly orchestrates multiple server instances using the new multi_popen helper. The client setup and request handling logic remain clean and functional.
276-291: Clean dynamic URL generation for multi-instance configuration.The port allocation scheme using incremental offsets (8001 + i2, 8002 + i2) ensures no port conflicts between instances, and the server configuration construction is well-structured.
455-459: Excellent addition of multi-instance test coverage.The new test method effectively validates multi-instance disaggregated serving with 2 context and 2 generation instances, using minimal parallelism to keep resource requirements reasonable while still testing the core functionality.
445-445: Good backward compatibility maintenance.The existing test methods are correctly updated to explicitly pass instance count parameters (1, 1) to maintain single-instance behavior while supporting the new multi-instance API.
Also applies to: 453-453
|
PR_Github #14337 [ run ] triggered by Bot |
|
PR_Github #14336 [ run ] completed with state |
|
PR_Github #14337 [ run ] completed with state |
|
/bot run --only-multi-gpu-test |
|
PR_Github #14358 [ run ] triggered by Bot |
|
PR_Github #14358 [ run ] completed with state |
|
@raayandhar could you target |
|
/bot run --disable-fail-fast |
|
PR_Github #14487 [ run ] triggered by Bot |
|
PR_Github #14487 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #14513 [ run ] triggered by Bot |
|
PR_Github #14513 [ run ] completed with state |
|
@raayandhar could you rebase and resolve the merge conflicts? Thanks. |
raayandhar
force-pushed
the
multi_inst_disagg_test
branch
from
c976c43 to
28f4e2d
Compare
Signed-off-by: raayandhar <[email protected]>
Signed-off-by: raayandhar <[email protected]>
Signed-off-by: raayandhar <[email protected]>
raayandhar
force-pushed
the
multi_inst_disagg_test
branch
from
d7f27f2 to
bb65652
Compare
|
PR_Github #14836 [ reuse-pipeline ] triggered by Bot |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration/defs/accuracy/test_disaggregated_serving.py (1)
193-201: Infinite health check loop still needs timeout (duplicate issue).The infinite
while True:loop for health checking can cause tests to hang indefinitely if servers never become healthy. This was flagged in previous reviews but remains unaddressed.Add a timeout to prevent indefinite hangs:
- while True: + deadline = time.time() + 120 # 2 minutes + while time.time() < deadline: time.sleep(1) try: print("Checking health endpoint") response = requests.get("http://localhost:8000/health") if response.status_code == 200: break except requests.exceptions.ConnectionError: continue + else: + raise RuntimeError("Disaggregated server failed to become healthy within 2 minutes")
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/test_disaggregated_serving.py (1)
244-251: Fix line length violation in error message.The GPU calculation logic is correct, but the error message on line 250 exceeds the 120-character limit.
Apply this diff to fix the line length:
- f"Not enough devices for {ctx_instances} ctx instances (ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances (gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}" + f"Not enough devices for {ctx_instances} ctx instances " + f"(ctx_pp={ctx_pp}*ctx_tp={ctx_tp}) + {gen_instances} gen instances " + f"(gen_pp={gen_pp}*gen_tp={gen_tp}), total: {total_ctx_gpus + total_gen_gpus}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/defs/accuracy/test_disaggregated_serving.py(3 hunks)tests/integration/test_lists/qa/llm_function_full.txt(1 hunks)tests/integration/test_lists/test-db/l0_dgx_h100.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/test_lists/qa/llm_function_full.txt
- tests/integration/test_lists/test-db/l0_dgx_h100.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile).
Python global variables should use upper snake_case and prefix G (e.g., G_MY_GLOBAL).
Python constants should use upper snake_case (e.g., MY_CONSTANT).
Avoid shadowing variables declared in an outer scope in Python.
Initialize all externally visible members of a Python class in the constructor.
For interfaces that may be used outside a Python file, prefer docstrings over comments.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the class docstring.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
tests/integration/defs/accuracy/test_disaggregated_serving.py
🪛 Ruff (0.12.2)
tests/integration/defs/accuracy/test_disaggregated_serving.py
250-250: Line too long (209 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (6)
tests/integration/defs/accuracy/test_disaggregated_serving.py (6)
118-123: LGTM! Clean URL and port extraction.The URL extraction and port parsing logic is straightforward and correctly handles the expected "host:port" format.
124-166: Excellent GPU allocation strategy for multi-instance deployment.The sequential GPU allocation using
current_gpu_offsetensures no GPU overlap between instances. The environment setup withTRTLLM_USE_UCX_KVCACHEand per-instanceCUDA_VISIBLE_DEVICESis appropriate for disaggregated serving.
167-185: Good fix for the logger issue from previous review.The
multi_popencontext manager correctly usesprint()instead of the previously undefinedloggervariable. TheExitStackapproach for managing multiple process contexts is appropriate and clean.
276-291: Smart port allocation strategy prevents conflicts.The URL generation with 2-port increments ensures context servers (8001, 8003, 8005...) and generation servers (8002, 8004, 8006...) never conflict. The updated config structure with
num_instancesandurlslists properly supports multi-instance deployments.
445-445: Good backward compatibility maintenance.Adding
ctx_instances=1, gen_instances=1to existing test calls maintains backward compatibility while enabling the new multi-instance functionality.
455-459: Well-designed multi-instance test.The new
test_multi_instancetest effectively validates multi-instance deployment with 2 context and 2 generation servers. The 4-GPU requirement is correctly marked with@pytest.mark.skip_less_device(4).
|
PR_Github #14836 [ reuse-pipeline ] completed with state |
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…VIDIA#6674) Signed-off-by: raayandhar <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
5 participants
Summary by CodeRabbit
New Features
Tests
Description
We should have a multi-instance dis-aggregated serving accuracy test. Previously
launch_disaggregated_llmhardcoded ports and only expected single instance setups. I've made changes to this function so we can support arbitrary multi-instance tests.Test Coverage
Added a new test
test_multi_instancefor this, passes locally on 4xH200s. Also testedtest_ngramto also double check that when we have single instance ctx/gen it recovers the previous setup.GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.