[https://nvbugs/5375594][fix] fix oom issue on structural_tag test case #6838
Conversation
📝 WalkthroughWalkthroughUpdates test configurations and fixtures: removes one test waiver, revises two unit tests to set guided_decoding_backend: xgrammar, adjusts server CLI args, restructures one test to validate tool call JSON via jsonschema, and changes a fixture’s signature. No production code or public API changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Signed-off-by: nv-guomingz <[email protected]>
nv-guomingz
force-pushed
the
user/guomingz/fix_nvbug_5375646
branch
from
2dae890 to
7fe0d40
Compare
|
/bot run --stage-list "A10-PyTorch-1" |
|
PR_Github #15008 [ run ] triggered by Bot |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (2)
22-37: Avoid temp file collisions in parallel runs; use a unique temp filename.Using a fixed filename in the global temp directory can collide across parallel pytest workers or other tests, causing flaky behavior. Prefer mkstemp or NamedTemporaryFile(delete=False) to generate a unique, race-free path.
Apply this minimal diff to harden the fixture (and prefer safe_dump):
def temp_extra_llm_api_options_file(request): - temp_dir = tempfile.gettempdir() - temp_file_path = os.path.join(temp_dir, "extra_llm_api_options.yaml") + fd, temp_file_path = tempfile.mkstemp(prefix="extra_llm_api_options_", suffix=".yaml") + os.close(fd) try: extra_llm_api_options_dict = { "guided_decoding_backend": "xgrammar", "max_batch_size": 32 } - with open(temp_file_path, 'w') as f: - yaml.dump(extra_llm_api_options_dict, f) + with open(temp_file_path, 'w') as f: + yaml.safe_dump(extra_llm_api_options_dict, f) yield temp_file_path finally: if os.path.exists(temp_file_path): os.remove(temp_file_path)
1-2: Add NVIDIA copyright header
Please prepend the current-year NVIDIA header—matching the existing format in other Python files—to tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py:+# Copyright 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
🧹 Nitpick comments (1)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (1)
16-19: Fixture ids is unused (not parametrized) and also mismatched with returned model name.Either remove ids or parametrize the fixture properly. If you keep a single, fixed model, dropping ids avoids confusion.
Apply one of:
-@pytest.fixture(scope="module", ids=["TinyLlama-1.1B-Chat"]) +@pytest.fixture(scope="module") def model_name(): return "llama-3.1-model/Llama-3.1-8B-Instruct"Or align the id:
-@pytest.fixture(scope="module", ids=["TinyLlama-1.1B-Chat"]) +@pytest.fixture(scope="module", ids=["Llama-3.1-8B-Instruct"]) def model_name(): return "llama-3.1-model/Llama-3.1-8B-Instruct"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/test_lists/waives.txt(0 hunks)tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
⏰ 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 (2)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (2)
26-29: Adding max_batch_size=32 is a sensible, targeted mitigation for OOM in this test path.Capping request batch size at the server level for this test should prevent spikes that lead to OOM without altering test semantics.
41-45: Confirmed — extra_llm_api_options 'max_batch_size' is read and appliedShort summary: serve.py loads the YAML and merges it into the runtime args; the merge applies top-level keys (including max_batch_size) and build_config, and the LLM/runtime code uses args.max_batch_size (falling back to build_config.max_batch_size).
Files to inspect (evidence):
- tensorrt_llm/commands/serve.py — reads the YAML and merges into llm args (see handling around lines ~316–321).
- tensorrt_llm/llmapi/llm_args.py — update_llm_args_with_extra_dict merges top-level keys and converts build_config (function around ~2364–2392).
- tensorrt_llm/llmapi/llm.py — LLM reads self.args.max_batch_size and falls back to build_config.max_batch_size (uses at ~614–617 and other places where runtime max_batch_size is applied).
- tensorrt_llm/runtime/model_runner_cpp.py — C++ runner uses the provided max_batch_size (defaults/asserts vs model_config.max_batch_size around ~358–361).
Conclusion: no code changes required — extra_llm_api_options max_batch_size is consumed and enforced by the backend.
|
PR_Github #15008 [ run ] completed with state |
|
/bot run |
|
PR_Github #15032 [ run ] triggered by Bot |
Signed-off-by: Enwei Zhu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (2)
25-37: Make temp file name unique and use safe_dump to avoid cross-test collisionsUsing a fixed filename in the system temp directory can collide across parallel test runs or modules. Prefer mkstemp to create a unique file and yaml.safe_dump for safety.
Apply this diff:
def temp_extra_llm_api_options_file(): - temp_dir = tempfile.gettempdir() - temp_file_path = os.path.join(temp_dir, "extra_llm_api_options.yaml") + fd, temp_file_path = tempfile.mkstemp( + prefix="extra_llm_api_options_", + suffix=".yaml" + ) + os.close(fd) try: extra_llm_api_options_dict = {"guided_decoding_backend": "xgrammar"} - with open(temp_file_path, 'w') as f: - yaml.dump(extra_llm_api_options_dict, f) + with open(temp_file_path, 'w', encoding='utf-8') as f: + yaml.safe_dump(extra_llm_api_options_dict, f, sort_keys=True) yield temp_file_path finally: if os.path.exists(temp_file_path): os.remove(temp_file_path)
122-147: Resolve instruction conflict and improve prompt clarity; embed schemas as JSONThe system prompt asks to “Only call one function at a time” but the test expects both functions to be called. Also, embed the schemas as JSON (not Python dict repr) for clarity. Fix grammar.
Apply this diff:
- system_prompt = f""" + system_prompt = f""" # Tool Instructions - Always execute python code in messages that you share. - When looking for real time information use relevant functions if available else fallback to brave_search You have access to the following functions: Use the function 'get_current_weather' to: Get the current weather in a given location -{tool_get_current_weather["function"]} +{json.dumps(tool_get_current_weather["function"], ensure_ascii=False)} Use the function 'get_current_date' to: Get the current date and time for a given timezone -{tool_get_current_date["function"]} +{json.dumps(tool_get_current_date["function"], ensure_ascii=False)} -If a you choose to call a function ONLY reply in the following format: +If you choose to call a function, ONLY reply in the following format: <{{start_tag}}={{function_name}}>{{parameters}}{{end_tag}} where start_tag => `<function` parameters => a JSON dict with the function argument name as key and function argument value as value. end_tag => `</function>` Here is an example, <function=example_function_name>{{"example_name": "example_value"}}</function> Reminder: - Function calls MUST follow the specified format - Required parameters MUST be specified -- Only call one function at a time -- Put the entire function call reply on one line +- Call both functions by emitting two separate function-call lines (one per function) in the same reply +- Put each function call on a single line - Always add your sources when using search results to answer the user query You are a helpful assistant."""Additionally, to reduce test flakiness, consider setting temperature=0 on the completion request (outside this block).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
⏰ 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/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (3)
148-156: LGTM: Two-part messaging improves claritySplitting system vs user prompts is clearer and should guide the model better.
1-2: Missing NVIDIA copyright headerPer repository guidelines, prepend the NVIDIA copyright header to all Python source files.
Apply this diff:
+# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. # Adapted from # https://github.com/vllm-project/vllm/blob/aae6927be06dedbda39c6b0c30f6aa3242b84388/tests/entrypoints/openai/test_chat.py⛔ Skipped due to learnings
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.
44-47: Validate CLI Flags in RemoteOpenAIServer
Both--max_batch_sizeand--extra_llm_api_optionsare defined and accepted by the server’s CLI:•
--max_batch_sizeis registered (underscore style) in tensorrt_llm/commands/serve.py and build.py via click and argparse
•--extra_llm_api_optionsis supported in the llm-launch command (used by RemoteOpenAIServer)No changes needed.
Signed-off-by: Enwei Zhu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
requirements-dev.txt(1 hunks)
⏰ 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 (1)
requirements-dev.txt (1)
27-27: LGTM on adding jsonschema for test-time schema validation.This aligns with the updated tests that validate payloads with jsonschema.
|
PR_Github #15032 [ run ] completed with state |
|
/bot run --stage-list "A10-PyTorch-1" |
|
PR_Github #15059 [ run ] triggered by Bot |
|
PR_Github #15059 [ run ] completed with state |
Signed-off-by: Enwei Zhu <[email protected]>
|
/bot run --stage-list "A10-PyTorch-1" |
1 similar comment
|
/bot run --stage-list "A10-PyTorch-1" |
|
PR_Github #15076 [ run ] triggered by Bot |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (1)
25-38: Avoid temp file collisions; use a unique NamedTemporaryFile and safe_dumpWriting to a fixed filename in the global temp dir is racy and can collide under parallel CI. Use NamedTemporaryFile(delete=False) and yaml.safe_dump.
Apply this diff:
-def temp_extra_llm_api_options_file(): - temp_dir = tempfile.gettempdir() - temp_file_path = os.path.join(temp_dir, "extra_llm_api_options.yaml") - try: - extra_llm_api_options_dict = {"guided_decoding_backend": "xgrammar"} - - with open(temp_file_path, 'w') as f: - yaml.dump(extra_llm_api_options_dict, f) - - yield temp_file_path - finally: - if os.path.exists(temp_file_path): - os.remove(temp_file_path) +def temp_extra_llm_api_options_file(): + # Avoid cross-test collisions by using a unique temp file per run. + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as tf: + yaml.safe_dump({"guided_decoding_backend": "xgrammar"}, tf) + temp_file_path = tf.name + try: + yield temp_file_path + finally: + if os.path.exists(temp_file_path): + os.remove(temp_file_path)
♻️ Duplicate comments (2)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (2)
3-9: Confirm jsonschema is declared in test/dev dependenciesNew import jsonschema requires a dev/test dependency. AI summary claims it was added to requirements-dev.txt; verify it exists to avoid CI import errors.
Run:
#!/bin/bash # Verify 'jsonschema' is declared in dependency files. fd -a 'requirements*.txt' 'pyproject.toml' 'setup.cfg' 'environment*.yml' 'Pipfile*' 2>/dev/null | while read -r f; do echo "---- $f" rg -n --fixed-strings 'jsonschema' "$f" || echo "jsonschema: not found in $f" done
188-197: Harden tag parsing and JSON validation; avoid [\S\s] and handle errorsGuard against missing tags and invalid JSON, use re.DOTALL, and de-duplicate via a loop.
Apply this diff:
- match = re.search(r'<function=get_current_weather>([\S\s]+?)</function>', - message.content) - params = json.loads(match.group(1)) - jsonschema.validate(params, - tool_get_current_weather["function"]["parameters"]) - - match = re.search(r'<function=get_current_date>([\S\s]+?)</function>', - message.content) - params = json.loads(match.group(1)) - jsonschema.validate(params, tool_get_current_date["function"]["parameters"]) + validations = [ + ("get_current_weather", + tool_get_current_weather["function"]["parameters"]), + ("get_current_date", + tool_get_current_date["function"]["parameters"]), + ] + for func_name, schema in validations: + match = re.search( + rf"<function={func_name}>(.+?)</function>", + message.content, + flags=re.DOTALL, + ) + assert match is not None, f"Missing function tag for {func_name}" + try: + params = json.loads(match.group(1)) + except json.JSONDecodeError as e: + pytest.fail(f"Invalid JSON for {func_name}: {e}") + jsonschema.validate(instance=params, schema=schema)
🧹 Nitpick comments (1)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (1)
123-148: Fix grammar and clarify multiple-function behavior to match test intentThe line “If a you choose…” has a typo, and “Only call one function at a time” conflicts with the user prompt (date + weather) and the test expecting both tags.
Apply this diff:
-If a you choose to call a function ONLY reply in the following format: +If you choose to call a function, reply ONLY in the following format: @@ - - Only call one function at a time + - You may call multiple functions; output each function call on its own line in a single assistant message
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/unittest/llmapi/apps/_test_openai_chat_structural_tag.py
⏰ 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/unittest/llmapi/apps/_test_openai_chat_structural_tag.py (3)
44-48: Good call: constrain server resources to prevent OOM on A10/A30Adding max_batch_size/max_seq_len/max_num_tokens aligns with the PR’s OOM objective and is a pragmatic mitigation.
1-2: Add required NVIDIA copyright header at file topPer coding guidelines, prepend the NVIDIA copyright header (current year) to all Python source files.
Apply this diff:
+ # Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # http://www.apache.org/licenses/LICENSE-2.0 + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. + # Adapted from # https://github.com/vllm-project/vllm/blob/aae6927be06dedbda39c6b0c30f6aa3242b84388/tests/entrypoints/openai/test_chat.py⛔ Skipped due to learnings
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.
163-163: max_completion_tokens correctly maps to max_tokens
TheChatCompletionRequestmodel inopenai_protocol.pydefinesmax_completion_tokens: int = Field(default=None, validation_alias='max_tokens')so using
max_completion_tokensin tests is intentional and will serialize as the officialmax_tokensparameter. No changes required.
|
PR_Github #15077 [ run ] triggered by Bot |
|
PR_Github #15076 [ run ] completed with state |
Signed-off-by: Enwei Zhu <[email protected]>
|
/bot run --stage-list "A10-PyTorch-1" |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
tests/unittest/llmapi/apps/_test_openai_chat_json.py (3)
79-80: Fix Python 3.8-incompatible type hints and incorrect return annotation.
- list[...] and dict[...] generics require Python 3.9+. This repo targets Python 3.8+, so use typing.List/Dict/Tuple or add from future import annotations.
- The function’s return annotation declares dict[str, Any], but it returns a tuple (dict, str).
Apply this diff:
-from typing import Any +from typing import Any, Dict, List, Tuple @@ - def _create_and_validate_response( - messages: list[dict[str, Any]]) -> dict[str, Any]: + def _create_and_validate_response( + messages: List[Dict[str, Any]]) -> Tuple[Dict[str, Any], str]: @@ - return message_json, message.content + return message_json, message.contentAlso applies to: 101-101, 6-6
24-37: Avoid file collisions/race conditions in temp file fixture; use tmp_path_factory and safe_dump.Using a fixed filename in the OS temp dir can race across parallel test workers and leaves cross-run artifacts. Prefer a unique path via tmp_path_factory. Also, prefer yaml.safe_dump.
Apply this diff:
-@pytest.fixture(scope="module") -def temp_extra_llm_api_options_file(request): - temp_dir = tempfile.gettempdir() - temp_file_path = os.path.join(temp_dir, "extra_llm_api_options.yaml") +@pytest.fixture(scope="module") +def temp_extra_llm_api_options_file(tmp_path_factory): + temp_dir = tmp_path_factory.mktemp("llmapi_options") + temp_file_path = temp_dir / "extra_llm_api_options.yaml" try: - extra_llm_api_options_dict = {"guided_decoding_backend": "xgrammar"} - - with open(temp_file_path, "w") as f: - yaml.dump(extra_llm_api_options_dict, f) - - yield temp_file_path + extra_llm_api_options_dict = {"guided_decoding_backend": "xgrammar"} + with open(temp_file_path, "w") as f: + yaml.safe_dump(extra_llm_api_options_dict, f) + yield str(temp_file_path) finally: - if os.path.exists(temp_file_path): - os.remove(temp_file_path) + if temp_file_path.exists(): + temp_file_path.unlink()
1-2: Add NVIDIA copyright header.This repository requires the NVIDIA copyright header at the top of all source files.
+# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. + # Adapted from # https://github.com/vllm-project/vllm/blob/aae6927be06dedbda39c6b0c30f6aa3242b84388/tests/entrypoints/openai/test_chat.py
🧹 Nitpick comments (1)
tests/unittest/llmapi/apps/_test_openai_chat_json.py (1)
84-84: Lower max_tokens to reduce memory/time without affecting test intent.For TinyLlama, max_tokens=1000 is excessive for generating a small JSON object and can increase memory footprint. 256 is more than enough.
- max_tokens=1000, + max_tokens=256,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittest/llmapi/apps/_test_openai_chat_json.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/unittest/llmapi/apps/_test_openai_chat_json.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/unittest/llmapi/apps/_test_openai_chat_json.py
⏰ 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 (2)
tests/unittest/llmapi/apps/_test_openai_chat_json.py (2)
29-29: Change to xgrammar backend looks good.Setting guided_decoding_backend to "xgrammar" aligns with the recent structural_tag test updates and should reduce OOM risk versus unconstrained decoding.
24-45: Validation of “xgrammar” Support in LLM Server
The LLM API dataclass ([tensorrt_llm/llmapi/llm_args.py:1202]) declares
guided_decoding_backend: Optional[Literal["xgrammar", "llguidance"]], so “xgrammar” is an accepted literal.In the Python executor (
tensorrt_llm/llmapi/llm.py), both in the initialization path and in the backend‐switching path, if
args.guided_decoding_backend == "xgrammar", the code builds a validGuidedDecodingConfigfor XGrammar; any other non‐null value
besides “llguidance” raises aValueError.In the Triton C++ backend ([model_instance_state.cc]), only “xgrammar” is allowed; passing anything else throws an error.
The CLI server (
trtllm-serve) reads--extra_llm_api_optionsintollm_argsviaupdate_llm_args_with_extra_dict, ensuring
guided_decoding_backend: "xgrammar"is forwarded to the Python LLM constructor.The RemoteOpenAIServer fixture simply passes
["--extra_llm_api_options", file]totrtllm-serve, so the option is correctly forwarded.All checks confirm that “xgrammar” is recognized end‐to‐end and will not silently no‐op. No changes required.
|
PR_Github #15093 [ run ] triggered by Bot |
|
PR_Github #15148 [ reuse-pipeline ] triggered by Bot |
|
PR_Github #15148 [ reuse-pipeline ] completed with state |
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (NVIDIA#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…se (#6838) Signed-off-by: nv-guomingz <[email protected]> Signed-off-by: Enwei Zhu <[email protected]> Co-authored-by: Enwei Zhu <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
Summary by CodeRabbit
Description
Test Coverage
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.
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.