-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[https://nvbugs/5527655][test] Add test case for RCCA 5527655 #9511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[https://nvbugs/5527655][test] Add test case for RCCA 5527655 #9511
Conversation
try to use b200 to load this one to speed up a little bit add test case for rcca 5527655 Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
e61562d to
1f39185
Compare
📝 WalkthroughWalkthroughAdds a YAML runtime configuration for DeepSeek-R1-FP4 and a matching integration test that launches the local serve binary, waits for readiness, performs an OpenAI chat completion against the server, and registers the test in the QA list. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Integration Test
participant FS as Files (YAML, Test List)
participant Serve as trtllm-serve
participant Client as Local OpenAI Client
Note over Test,FS: Setup
Test->>FS: read DeepSeek-R1-FP4.yml & model path
Test->>Serve: start process (popen) with config
Serve-->>Test: process started (pid)
alt server becomes ready
Serve-->>Test: readiness probe success
Note over Test,Client: Run inference
Test->>Client: send chat completion request to local endpoint
Client-->>Test: completion response
Test->>Serve: terminate process
Serve-->>Test: exit
else timeout / failure
Serve-->>Test: no readiness (timeout)
Test->>Serve: terminate process
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml(1 hunks)tests/integration/defs/examples/serve/test_serve.py(2 hunks)tests/integration/test_lists/qa/llm_function_core.txt(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 (e.g., usefrom package.subpackage import fooand thenfoo.SomeClass()instead offrom package.subpackage.foo import SomeClass)
Python filenames should use snake_case (e.g.,some_file.py)
Python class names should use PascalCase (e.g.,class SomeClass)
Python function and method names should use snake_case (e.g.,def my_awesome_function():)
Python local variable names should use snake_case, with prefixkfor variable names that start with a number (e.g.,k_99th_percentile = ...)
Python global variables should use upper snake_case with prefixG(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 Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments 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
Python attributes and variables can be documented inline with type and description (e.g.,self.x = 5followed by"""<type>: Description of 'x'""")
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of specific errors possible instead of catching all exceptions
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block to implement the logic
Files:
tests/integration/defs/examples/serve/test_serve.py
**/*.{cpp,h,cu,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code files should contain an NVIDIA copyright header that includes the current year at the top
Files:
tests/integration/defs/examples/serve/test_serve.py
🧠 Learnings (8)
📚 Learning: 2025-08-29T14:07:45.863Z
Learnt from: EmmaQiaoCh
Repo: NVIDIA/TensorRT-LLM PR: 7370
File: tests/unittest/trt/model_api/test_model_quantization.py:24-27
Timestamp: 2025-08-29T14:07:45.863Z
Learning: In TensorRT-LLM's CI infrastructure, pytest skip markers (pytest.mark.skip) are properly honored even when test files have __main__ blocks that call test functions directly. The testing system correctly skips tests without requiring modifications to the __main__ block execution pattern.
Applied to files:
tests/integration/defs/examples/serve/test_serve.py
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
Repo: NVIDIA/TensorRT-LLM PR: 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/examples/serve/test_serve.pytests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-09-09T09:40:45.658Z
Learnt from: fredricz-20070104
Repo: NVIDIA/TensorRT-LLM PR: 7645
File: tests/integration/test_lists/qa/llm_function_core.txt:648-648
Timestamp: 2025-09-09T09:40:45.658Z
Learning: In TensorRT-LLM test lists, it's common and intentional for the same test to appear in multiple test list files when they serve different purposes (e.g., llm_function_core.txt for comprehensive core functionality testing and llm_function_core_sanity.txt for quick sanity checks). This duplication allows tests to be run in different testing contexts.
Applied to files:
tests/integration/defs/examples/serve/test_serve.pytests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-09-17T02:48:52.732Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7781
File: tests/integration/test_lists/waives.txt:313-313
Timestamp: 2025-09-17T02:48:52.732Z
Learning: In TensorRT-LLM, `tests/integration/test_lists/waives.txt` is specifically for waiving/skipping tests, while other test list files like those in `test-db/` and `qa/` directories are for different test execution contexts (pre-merge, post-merge, QA tests). The same test appearing in both waives.txt and execution list files is intentional - the test is part of test suites but will be skipped due to the waiver.
Applied to files:
tests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-08-13T11:07:11.772Z
Learnt from: Funatiq
Repo: NVIDIA/TensorRT-LLM PR: 6754
File: tests/integration/test_lists/test-db/l0_a30.yml:41-47
Timestamp: 2025-08-13T11:07:11.772Z
Learning: In TensorRT-LLM test configuration files like tests/integration/test_lists/test-db/l0_a30.yml, TIMEOUT values are specified in minutes, not seconds.
Applied to files:
tests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 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_core.txt
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which can contain default `cuda_graph_config` values, so `llm_args` may already have this config before the extra options processing.
Applied to files:
tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM's bench configuration, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which is a Dict[str, Any] that can contain default values including `cuda_graph_config`, making the fallback `llm_args["cuda_graph_config"]` safe to use.
Applied to files:
tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml
🧬 Code graph analysis (1)
tests/integration/defs/examples/serve/test_serve.py (2)
tests/integration/defs/conftest.py (2)
llm_models_root(80-94)serve_test_root(2636-2640)tests/integration/defs/trt_test_alternative.py (2)
print_info(300-306)popen(199-218)
🪛 Ruff (0.14.6)
tests/integration/defs/examples/serve/test_serve.py
159-159: Possible binding to all interfaces
(S104)
⏰ 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 (3)
tests/integration/test_lists/qa/llm_function_core.txt (1)
758-758: New DeepSeek-R1-FP4 serve QA entry looks consistentThe test list entry name matches the new test function and the TIMEOUT (120) aligns with other long-running examples/serve tests in this file. No issues from the list perspective.
tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml (1)
1-18: DeepSeek-R1-FP4 extra LLM API options YAML is well-structuredConfig keys and nesting (cuda_graph_config, moe_config, speculative_config, kv_cache_config, etc.) look consistent with existing perf/extra_llm_api_options usage, and values line up with the new serve test’s CLI arguments. No changes needed.
tests/integration/defs/examples/serve/test_serve.py (1)
3-6: Import updates for new skip marker are appropriateAdding
pytestand importingskip_pre_hopper/skip_post_blackwell_ultrais consistent with the new decorators on the DeepSeek-R1-FP4 test; no issues here.
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
| check_openai_chat_completion(model_name=model_name) | ||
|
|
||
|
|
||
| @skip_pre_hopper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fp4 does not support on hopper, use skip_pre_blackwell?
| def test_extra_llm_api_options_for_deepseek_r1_fp4(serve_test_root): | ||
| test_configs_root = f"{serve_test_root}/test_configs" | ||
|
|
||
| # moe backend = CUTLASS which only supports fp8 blockscale on Hopper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is kind of misleading
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Add test case for RCCA 5527655.