chore: disallow arbitrary arguments in llm_args.xxxConfigs #6367
Conversation
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StrictBaseModel
participant ConfigClass
User->>ConfigClass: Instantiate with known fields
ConfigClass->>StrictBaseModel: Validate fields
StrictBaseModel-->>User: Success
User->>ConfigClass: Instantiate with unknown field(s)
ConfigClass->>StrictBaseModel: Validate fields
StrictBaseModel-->>User: Raise ValidationError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Superjomn
force-pushed
the
fix-config-arbitrary-arg
branch
from
cb75553 to
c8ed32e
Compare
|
/bot run |
Superjomn
requested review from
litaotju,
kaiyux and
nv-guomingz
and removed request for
syuoni
![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
🧹 Nitpick comments (2)
tensorrt_llm/llmapi/llm_args.py (2)
64-71: Good implementation of strict validation base class.The
StrictBaseModelprovides a clean way to enforce strict field validation across all configuration classes. Consider reformatting the docstring to a single line as suggested by the linter.class StrictBaseModel(BaseModel): - """ - A base model that forbids arbitrary fields. - """ + """A base model that forbids arbitrary fields."""
133-136: Consistent application of strict validation.The update to inherit from
StrictBaseModelis correctly applied. Consider fixing the docstring format here as well.class MoeConfig(StrictBaseModel): - """ - Configuration for MoE. - """ + """Configuration for MoE."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tensorrt_llm/llmapi/llm_args.py(18 hunks)tests/unittest/llmapi/test_llm_args.py(1 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 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/unittest/llmapi/test_llm_args.pytensorrt_llm/llmapi/llm_args.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/unittest/llmapi/test_llm_args.pytensorrt_llm/llmapi/llm_args.py
🪛 Ruff (0.12.2)
tests/unittest/llmapi/test_llm_args.py
483-483: CudaGraphConfig may be undefined, or defined from star imports
(F405)
490-490: CudaGraphConfig may be undefined, or defined from star imports
(F405)
496-496: MoeConfig may be undefined, or defined from star imports
(F405)
503-503: MoeConfig may be undefined, or defined from star imports
(F405)
509-509: CalibConfig may be undefined, or defined from star imports
(F405)
516-516: CalibConfig may be undefined, or defined from star imports
(F405)
522-522: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
528-528: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
534-534: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
537-537: Avoid equality comparisons to True; use config.enable_batch_size_tuning: for truth checks
Replace with config.enable_batch_size_tuning
(E712)
542-542: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
566-566: PeftCacheConfig may be undefined, or defined from star imports
(F405)
574-574: PeftCacheConfig may be undefined, or defined from star imports
(F405)
581-581: KvCacheConfig may be undefined, or defined from star imports
(F405)
582-582: Avoid equality comparisons to True; use config.enable_block_reuse: for truth checks
Replace with config.enable_block_reuse
(E712)
588-588: KvCacheConfig may be undefined, or defined from star imports
(F405)
595-595: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
597-597: Avoid equality comparisons to True; use config.multi_block_mode: for truth checks
Replace with config.multi_block_mode
(E712)
598-598: Avoid equality comparisons to False; use not config.cuda_graph_mode: for false checks
Replace with not config.cuda_graph_mode
(E712)
603-603: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
610-610: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
618-618: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
624-624: TorchCompileConfig may be undefined, or defined from star imports
(F405)
626-626: Avoid equality comparisons to True; use config.enable_fullgraph: for truth checks
Replace with config.enable_fullgraph
(E712)
627-627: Avoid equality comparisons to False; use not config.enable_inductor: for false checks
Replace with not config.enable_inductor
(E712)
632-632: TorchCompileConfig may be undefined, or defined from star imports
(F405)
639-639: TrtLlmArgs may be undefined, or defined from star imports
(F405)
646-646: TrtLlmArgs may be undefined, or defined from star imports
(F405)
652-652: TorchLlmArgs may be undefined, or defined from star imports
(F405)
659-659: TorchLlmArgs may be undefined, or defined from star imports
(F405)
668-668: KvCacheConfig may be undefined, or defined from star imports
(F405)
684-684: StrictBaseModel may be undefined, or defined from star imports
(F405)
687-687: StrictBaseModel may be undefined, or defined from star imports
(F405)
tensorrt_llm/llmapi/llm_args.py
65-66: One-line docstring should fit on one line
Reformat to one line
(D200)
132-133: One-line docstring should fit on one line
Reformat to one line
(D200)
🔇 Additional comments (8)
tensorrt_llm/llmapi/llm_args.py (7)
73-131: Well-structured centralization of CUDA graph batch size generation logic.The migration to
StrictBaseModeland the addition of the static method_generate_cuda_graph_batch_sizeseffectively centralizes previously duplicated logic. The implementation handles both padding and non-padding cases appropriately with clear batch size distributions.
290-313: Correct handling of dispatch parameter in DecodingBaseConfig.The removal of
"decoding_type"from the data dictionary before instantiating the specific config class is the right approach to prevent validation errors while maintaining the dispatch logic.
509-511: Good enforcement of inheritance requirement in decorator.The assertion ensures that only classes inheriting from
StrictBaseModelcan use themirror_pybind_fieldsdecorator, maintaining consistency across the codebase.
961-968: Proper migration to StrictBaseModel with appropriate config handling.The inheritance from
StrictBaseModelis correctly applied. The removal of the explicitmodel_configis appropriate sinceStrictBaseModelalready provides the necessary configuration.
1344-1361: Improved defensive programming in build_config initialization.The use of
getattrwith a default value before checking is a safer approach that handles edge cases better than direct attribute access.
1798-1831: Consistent application of StrictBaseModel to TorchCompileConfig.The migration to
StrictBaseModelis correctly applied toTorchCompileConfig, ensuring consistent validation behavior across all configuration classes.
2048-2063: Correct usage of centralized batch size generation method.The calls to
CudaGraphConfig._generate_cuda_graph_batch_sizesproperly utilize the newly centralized static method, eliminating code duplication.tests/unittest/llmapi/test_llm_args.py (1)
477-701: Excellent comprehensive test coverage for strict validation enforcement.The
TestStrictBaseModelArbitraryArgsclass provides thorough validation of the new strict field validation behavior. The tests:
- Cover all configuration classes that were updated
- Follow a consistent and clear testing pattern
- Properly verify both acceptance of valid fields and rejection of arbitrary fields
- Include edge cases like nested configurations and inheritance validation
This ensures the strict validation feature works as intended across the entire configuration hierarchy.
|
PR_Github #13007 [ run ] triggered by Bot |
Superjomn
force-pushed
the
fix-config-arbitrary-arg
branch
from
c8ed32e to
8bbd45e
Compare
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unittest/llmapi/test_llm_args.py (4)
531-547: Good test with minor style improvement opportunity.The test correctly validates DynamicBatchConfig, but consider improving the boolean comparison style.
Apply this diff to improve boolean comparison style:
- assert config.enable_batch_size_tuning == True + assert config.enable_batch_size_tuning
578-591: Good test with minor style improvement opportunity.The test correctly validates KvCacheConfig, but consider improving the boolean comparison style.
Apply this diff to improve boolean comparison style:
- assert config.enable_block_reuse == True + assert config.enable_block_reuse
592-606: Good test with minor style improvements needed.The test correctly validates ExtendedRuntimePerfKnobConfig, but consider improving the boolean comparison style.
Apply this diff to improve boolean comparison style:
- assert config.multi_block_mode == True - assert config.cuda_graph_mode == False + assert config.multi_block_mode + assert not config.cuda_graph_mode
621-635: Good test with minor style improvements needed.The test correctly validates TorchCompileConfig, but consider improving the boolean comparison style.
Apply this diff to improve boolean comparison style:
- assert config.enable_fullgraph == True - assert config.enable_inductor == False + assert config.enable_fullgraph + assert not config.enable_inductortensorrt_llm/llmapi/llm_args.py (1)
64-71: Excellent implementation of strict validation enforcement.The
StrictBaseModelclass provides a clean way to globally enforce Pydantic'sextra="forbid"policy. This will prevent unexpected fields in configuration models, improving type safety and catching configuration errors early.Consider reformatting the docstring to one line as suggested by the static analysis:
- """ - A base model that forbids arbitrary fields. - """ + """A base model that forbids arbitrary fields."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tensorrt_llm/llmapi/llm_args.py(18 hunks)tests/unittest/llmapi/test_llm_args.py(1 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 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:
tensorrt_llm/llmapi/llm_args.pytests/unittest/llmapi/test_llm_args.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:
tensorrt_llm/llmapi/llm_args.pytests/unittest/llmapi/test_llm_args.py
🪛 Ruff (0.12.2)
tensorrt_llm/llmapi/llm_args.py
65-66: One-line docstring should fit on one line
Reformat to one line
(D200)
132-133: One-line docstring should fit on one line
Reformat to one line
(D200)
tests/unittest/llmapi/test_llm_args.py
483-483: CudaGraphConfig may be undefined, or defined from star imports
(F405)
490-490: CudaGraphConfig may be undefined, or defined from star imports
(F405)
496-496: MoeConfig may be undefined, or defined from star imports
(F405)
503-503: MoeConfig may be undefined, or defined from star imports
(F405)
509-509: CalibConfig may be undefined, or defined from star imports
(F405)
516-516: CalibConfig may be undefined, or defined from star imports
(F405)
522-522: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
528-528: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
534-534: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
537-537: Avoid equality comparisons to True; use config.enable_batch_size_tuning: for truth checks
Replace with config.enable_batch_size_tuning
(E712)
542-542: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
566-566: PeftCacheConfig may be undefined, or defined from star imports
(F405)
574-574: PeftCacheConfig may be undefined, or defined from star imports
(F405)
581-581: KvCacheConfig may be undefined, or defined from star imports
(F405)
582-582: Avoid equality comparisons to True; use config.enable_block_reuse: for truth checks
Replace with config.enable_block_reuse
(E712)
588-588: KvCacheConfig may be undefined, or defined from star imports
(F405)
595-595: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
597-597: Avoid equality comparisons to True; use config.multi_block_mode: for truth checks
Replace with config.multi_block_mode
(E712)
598-598: Avoid equality comparisons to False; use not config.cuda_graph_mode: for false checks
Replace with not config.cuda_graph_mode
(E712)
603-603: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
610-610: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
618-618: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
624-624: TorchCompileConfig may be undefined, or defined from star imports
(F405)
626-626: Avoid equality comparisons to True; use config.enable_fullgraph: for truth checks
Replace with config.enable_fullgraph
(E712)
627-627: Avoid equality comparisons to False; use not config.enable_inductor: for false checks
Replace with not config.enable_inductor
(E712)
632-632: TorchCompileConfig may be undefined, or defined from star imports
(F405)
639-639: TrtLlmArgs may be undefined, or defined from star imports
(F405)
646-646: TrtLlmArgs may be undefined, or defined from star imports
(F405)
652-652: TorchLlmArgs may be undefined, or defined from star imports
(F405)
659-659: TorchLlmArgs may be undefined, or defined from star imports
(F405)
668-668: KvCacheConfig may be undefined, or defined from star imports
(F405)
684-684: StrictBaseModel may be undefined, or defined from star imports
(F405)
687-687: StrictBaseModel may be undefined, or defined from star imports
(F405)
⏰ 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 (20)
tests/unittest/llmapi/test_llm_args.py (12)
477-479: Excellent test class structure and documentation.The test class is well-structured with a clear docstring explaining its purpose. This directly supports the PR objective of disallowing arbitrary arguments in llm_args.Configs.
480-492: Well-structured test for CudaGraphConfig validation.The test correctly validates both positive and negative cases, ensuring that valid arguments work while arbitrary arguments are properly rejected.
493-505: Consistent test implementation for MoeConfig.Good test coverage following the established pattern for validating strict argument enforcement.
506-518: Consistent CalibConfig validation test.The test properly validates the strict argument enforcement for CalibConfig.
519-530: Good test coverage for DecodingBaseConfig.The test correctly validates strict argument enforcement following the established pattern.
548-562: Well-implemented SchedulerConfig test.The test properly validates strict argument enforcement for SchedulerConfig.
563-577: Consistent PeftCacheConfig validation test.The test correctly validates strict argument enforcement following the established pattern.
607-620: Well-implemented CacheTransceiverConfig test.The test properly validates strict argument enforcement for CacheTransceiverConfig.
636-648: Well-implemented TrtLlmArgs test.The test properly validates strict argument enforcement for the main TrtLlmArgs class.
649-662: Consistent TorchLlmArgs validation test.The test correctly validates strict argument enforcement for TorchLlmArgs.
663-680: Excellent additional test coverage for nested configurations.This test ensures that the strict validation behavior is properly inherited in nested configuration scenarios.
681-700: Excellent comprehensive test of the StrictBaseModel foundation.This test is particularly valuable as it validates the core mechanism that enables all the strict validation behavior. Testing both the configuration (
extra="forbid") and the actual inheritance behavior provides strong coverage of the base functionality.tensorrt_llm/llmapi/llm_args.py (8)
73-73: Good migration to strict validation.The change from
BaseModeltoStrictBaseModelis consistent with the PR objective to disallow arbitrary fields in configuration classes.
100-131: Well-implemented centralization of batch size generation logic.The static method effectively centralizes CUDA graph batch size generation with clear documentation and proper logic:
- Handles different batch size distributions based on
enable_padding- Includes powers of 2 up to
max_batch_size- Properly filters, sorts, and ensures
max_batch_sizeis includedThis improves code maintainability by removing duplication.
133-133: Consistent migration to strict validation across all configuration classes.All configuration classes have been systematically updated to inherit from
StrictBaseModel, ensuring consistent enforcement of theextra="forbid"policy throughout the codebase. This will help catch configuration errors early and improve type safety.Also applies to: 238-238, 290-290, 611-611, 637-637, 659-659, 787-787, 870-870, 901-901, 961-961
510-510: Good enforcement of strict validation in decorator.Updating the assertion to check for
StrictBaseModelensures that all classes using the pybind mirroring functionality also enforce strict field validation, maintaining consistency across the codebase.
311-311: Proper cleanup of dispatch field for strict validation.Removing the "decoding_type" field after using it for class dispatch prevents it from being passed to the constructor, which would now cause validation errors with the strict base model. This is a necessary fix for the new validation behavior.
1350-1367: Improved safety in build config initialization.The changes provide better error handling:
- Using
getattr(self, "build_config", None)prevents potential AttributeError- The assertion with a descriptive error message helps with debugging if
build_configis not properly initializedThis is good defensive programming that makes the code more robust.
1804-1804: Consistent migration to strict validation.The
TorchCompileConfigclass has been updated to inherit fromStrictBaseModel, maintaining consistency with other configuration classes in the codebase.
2054-2054: Consistent usage of centralized batch size generation.The method calls have been updated to use the centralized
CudaGraphConfig._generate_cuda_graph_batch_sizesmethod, which is consistent with the refactoring that moved this logic to the appropriate class. This improves code organization and maintainability.Also applies to: 2066-2066
|
PR_Github #13085 [ run ] triggered by Bot |
|
PR_Github #13085 [ run ] completed with state |
|
/bot run |
|
PR_Github #13103 [ run ] triggered by Bot |
|
PR_Github #13103 [ run ] completed with state |
Superjomn
force-pushed
the
fix-config-arbitrary-arg
branch
from
8bbd45e to
840b788
Compare
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unittest/llmapi/test_llm_args.py (1)
477-701: Excellent comprehensive test coverage for strict validation enforcement.The new test class thoroughly validates that all configuration classes properly reject arbitrary arguments while accepting valid ones. The test structure is consistent and covers the key scenarios well.
Consider these minor style improvements:
- assert config.enable_batch_size_tuning == True + assert config.enable_batch_size_tuning - assert config.enable_block_reuse == True + assert config.enable_block_reuse - assert config.multi_block_mode == True + assert config.multi_block_mode - assert config.enable_fullgraph == True + assert config.enable_fullgraph - assert config.cuda_graph_mode == False + assert not config.cuda_graph_mode - assert config.enable_inductor == False + assert not config.enable_inductorThe static analysis warnings about undefined imports from star imports are expected given the
from tensorrt_llm.llmapi.llm_args import *pattern used consistently throughout this test file.tensorrt_llm/llmapi/llm_args.py (1)
64-71: LGTM! Good practice to enforce strict validation.The
StrictBaseModelimplementation correctly uses Pydantic'sextra="forbid"to prevent arbitrary fields, which will help catch configuration errors early.Consider reformatting the docstring to one line as suggested by static analysis:
- """ - A base model that forbids arbitrary fields. - """ + """A base model that forbids arbitrary fields."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tensorrt_llm/llmapi/llm_args.py(18 hunks)tests/unittest/_torch/test_beam_search.py(1 hunks)tests/unittest/llmapi/test_llm_args.py(1 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 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/unittest/_torch/test_beam_search.pytensorrt_llm/llmapi/llm_args.pytests/unittest/llmapi/test_llm_args.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/unittest/_torch/test_beam_search.pytensorrt_llm/llmapi/llm_args.pytests/unittest/llmapi/test_llm_args.py
🪛 Ruff (0.12.2)
tensorrt_llm/llmapi/llm_args.py
65-66: One-line docstring should fit on one line
Reformat to one line
(D200)
132-133: One-line docstring should fit on one line
Reformat to one line
(D200)
tests/unittest/llmapi/test_llm_args.py
483-483: CudaGraphConfig may be undefined, or defined from star imports
(F405)
490-490: CudaGraphConfig may be undefined, or defined from star imports
(F405)
496-496: MoeConfig may be undefined, or defined from star imports
(F405)
503-503: MoeConfig may be undefined, or defined from star imports
(F405)
509-509: CalibConfig may be undefined, or defined from star imports
(F405)
516-516: CalibConfig may be undefined, or defined from star imports
(F405)
522-522: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
528-528: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
534-534: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
537-537: Avoid equality comparisons to True; use config.enable_batch_size_tuning: for truth checks
Replace with config.enable_batch_size_tuning
(E712)
542-542: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
566-566: PeftCacheConfig may be undefined, or defined from star imports
(F405)
574-574: PeftCacheConfig may be undefined, or defined from star imports
(F405)
581-581: KvCacheConfig may be undefined, or defined from star imports
(F405)
582-582: Avoid equality comparisons to True; use config.enable_block_reuse: for truth checks
Replace with config.enable_block_reuse
(E712)
588-588: KvCacheConfig may be undefined, or defined from star imports
(F405)
595-595: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
597-597: Avoid equality comparisons to True; use config.multi_block_mode: for truth checks
Replace with config.multi_block_mode
(E712)
598-598: Avoid equality comparisons to False; use not config.cuda_graph_mode: for false checks
Replace with not config.cuda_graph_mode
(E712)
603-603: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
610-610: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
618-618: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
624-624: TorchCompileConfig may be undefined, or defined from star imports
(F405)
626-626: Avoid equality comparisons to True; use config.enable_fullgraph: for truth checks
Replace with config.enable_fullgraph
(E712)
627-627: Avoid equality comparisons to False; use not config.enable_inductor: for false checks
Replace with not config.enable_inductor
(E712)
632-632: TorchCompileConfig may be undefined, or defined from star imports
(F405)
639-639: TrtLlmArgs may be undefined, or defined from star imports
(F405)
646-646: TrtLlmArgs may be undefined, or defined from star imports
(F405)
652-652: TorchLlmArgs may be undefined, or defined from star imports
(F405)
659-659: TorchLlmArgs may be undefined, or defined from star imports
(F405)
668-668: KvCacheConfig may be undefined, or defined from star imports
(F405)
684-684: StrictBaseModel may be undefined, or defined from star imports
(F405)
687-687: StrictBaseModel may be undefined, or defined from star imports
(F405)
🔇 Additional comments (12)
tests/unittest/_torch/test_beam_search.py (1)
66-66: Approve: Default Constructor Enables CUDA GraphsThe
CudaGraphConfigclass no longer defines an explicitenabledfield—providing an instance itself signifies that CUDA graphs are enabled. Removing the now-unsupportedenabled=Trueargument aligns with the newStrictBaseModelvalidation and preserves existing test behavior. No further changes are required.tests/unittest/llmapi/test_llm_args.py (2)
681-685: Well-designed test for StrictBaseModel inheritance verification.The test properly validates that the base model configuration enforces
extra="forbid"and demonstrates the inheritance pattern working correctly.
663-680: Good coverage of nested configuration validation.The nested configuration tests ensure that the strict validation applies at all levels of the configuration hierarchy, which is crucial for comprehensive argument validation.
tensorrt_llm/llmapi/llm_args.py (9)
73-73: LGTM! Consistent with strict validation enforcement.The inheritance change from
BaseModeltoStrictBaseModelensures CUDA graph configuration rejects unexpected fields.
100-131: LGTM! Good centralization of batch size generation logic.The static method correctly consolidates the CUDA graph batch size generation logic with comprehensive documentation. The implementation handles both padding and non-padding cases appropriately.
133-133: LGTM! Consistent strict validation enforcement across all config classes.All configuration classes now inherit from
StrictBaseModel, ensuring consistent validation behavior and preventing arbitrary fields across the configuration system.Also applies to: 238-238, 290-290, 611-611, 637-637, 659-659, 787-787, 870-870, 901-901
311-311: LGTM! Correct handling of dispatch key removal.Removing the "decoding_type" key before instantiating the specific config subclass is the right approach, as this key is only used for dispatch logic and shouldn't be passed to the constructor.
510-510: LGTM! Consistent with strict validation requirements.The assertion now correctly requires
StrictBaseModelinstead ofBaseModel, ensuring all classes using this decorator have strict validation enabled.
961-961: LGTM! Consistent strict validation for base LLM args.The inheritance change ensures
BaseLlmArgsand all its subclasses reject arbitrary fields, maintaining consistency across the configuration system.
1344-1361: LGTM! More defensive build config initialization.Using
getattrwith a default value is more robust than direct attribute access and prevents potential AttributeErrors while maintaining the same logic flow.
1798-1798: LGTM! Consistent strict validation for torch compile config.The inheritance change ensures
TorchCompileConfigfollows the same strict validation pattern as other configuration classes.
2048-2049: LGTM! Proper use of centralized batch size generation.The method calls now correctly use the centralized
CudaGraphConfig._generate_cuda_graph_batch_sizesmethod, eliminating code duplication and improving maintainability.Also applies to: 2060-2061
|
PR_Github #13135 [ run ] triggered by Bot |
|
PR_Github #13135 [ run ] completed with state |
Signed-off-by: Superjomn <[email protected]>
Superjomn
force-pushed
the
fix-config-arbitrary-arg
branch
from
840b788 to
c40a4e0
Compare
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tensorrt_llm/llmapi/llm_args.py (1)
95-96: Minor: Reformat one-line docstrings.The one-line docstrings should fit on a single line per PEP 257:
- """ - A base model that forbids arbitrary fields. - """ + """A base model that forbids arbitrary fields.""" - """ - Configuration for CUDA graphs. - """ + """Configuration for CUDA graphs.""" - """ - Configuration for MoE. - """ + """Configuration for MoE."""Also applies to: 104-105, 165-166
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tensorrt_llm/llmapi/llm_args.py(18 hunks)tests/unittest/llmapi/test_llm_args.py(1 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, and 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 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/unittest/llmapi/test_llm_args.pytensorrt_llm/llmapi/llm_args.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/unittest/llmapi/test_llm_args.pytensorrt_llm/llmapi/llm_args.py
🧠 Learnings (2)
tests/unittest/llmapi/test_llm_args.py (1)
Learnt from: moraxu
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.
tensorrt_llm/llmapi/llm_args.py (1)
Learnt from: moraxu
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.
🪛 Ruff (0.12.2)
tests/unittest/llmapi/test_llm_args.py
483-483: CudaGraphConfig may be undefined, or defined from star imports
(F405)
490-490: CudaGraphConfig may be undefined, or defined from star imports
(F405)
496-496: MoeConfig may be undefined, or defined from star imports
(F405)
503-503: MoeConfig may be undefined, or defined from star imports
(F405)
509-509: CalibConfig may be undefined, or defined from star imports
(F405)
516-516: CalibConfig may be undefined, or defined from star imports
(F405)
522-522: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
528-528: DecodingBaseConfig may be undefined, or defined from star imports
(F405)
534-534: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
537-537: Avoid equality comparisons to True; use config.enable_batch_size_tuning: for truth checks
Replace with config.enable_batch_size_tuning
(E712)
542-542: DynamicBatchConfig may be undefined, or defined from star imports
(F405)
566-566: PeftCacheConfig may be undefined, or defined from star imports
(F405)
574-574: PeftCacheConfig may be undefined, or defined from star imports
(F405)
581-581: KvCacheConfig may be undefined, or defined from star imports
(F405)
582-582: Avoid equality comparisons to True; use config.enable_block_reuse: for truth checks
Replace with config.enable_block_reuse
(E712)
588-588: KvCacheConfig may be undefined, or defined from star imports
(F405)
595-595: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
597-597: Avoid equality comparisons to True; use config.multi_block_mode: for truth checks
Replace with config.multi_block_mode
(E712)
598-598: Avoid equality comparisons to False; use not config.cuda_graph_mode: for false checks
Replace with not config.cuda_graph_mode
(E712)
603-603: ExtendedRuntimePerfKnobConfig may be undefined, or defined from star imports
(F405)
610-610: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
618-618: CacheTransceiverConfig may be undefined, or defined from star imports
(F405)
624-624: TorchCompileConfig may be undefined, or defined from star imports
(F405)
626-626: Avoid equality comparisons to True; use config.enable_fullgraph: for truth checks
Replace with config.enable_fullgraph
(E712)
627-627: Avoid equality comparisons to False; use not config.enable_inductor: for false checks
Replace with not config.enable_inductor
(E712)
632-632: TorchCompileConfig may be undefined, or defined from star imports
(F405)
639-639: TrtLlmArgs may be undefined, or defined from star imports
(F405)
646-646: TrtLlmArgs may be undefined, or defined from star imports
(F405)
652-652: TorchLlmArgs may be undefined, or defined from star imports
(F405)
659-659: TorchLlmArgs may be undefined, or defined from star imports
(F405)
668-668: KvCacheConfig may be undefined, or defined from star imports
(F405)
684-684: StrictBaseModel may be undefined, or defined from star imports
(F405)
687-687: StrictBaseModel may be undefined, or defined from star imports
(F405)
tensorrt_llm/llmapi/llm_args.py
95-96: One-line docstring should fit on one line
Reformat to one line
(D200)
103-104: One-line docstring should fit on one line
Reformat to one line
(D200)
162-163: One-line docstring should fit on one line
Reformat to one line
(D200)
541-541: Line too long (152 > 120)
(E501)
2143-2143: Line too long (124 > 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 (8)
tensorrt_llm/llmapi/llm_args.py (7)
95-102: LGTM! Good addition for strict validation.The
StrictBaseModelclass correctly setsextra = "forbid"to prevent arbitrary fields from being accepted in all derived configuration classes. This will help catch configuration errors early.
131-162: LGTM! Well-implemented batch size generation logic.The static method correctly centralizes the CUDA graph batch size generation logic. The implementation handles both padding enabled and disabled cases appropriately, and ensures the max_batch_size is always included in the final list.
104-104: Consistent application of strict validation across all config classes.All configuration classes have been properly updated to inherit from
StrictBaseModel, ensuring uniform strict validation behavior across the codebase.Also applies to: 164-164, 269-269, 321-321, 642-642, 668-668, 690-690, 818-818, 901-901, 932-932, 992-992, 1859-1859
342-342: Good fix for decoding config instantiation.Removing the
"decoding_type"key before instantiation prevents validation errors since the specific decoding config subclasses don't have this field.
541-541: Correct assertion update for StrictBaseModel.The assertion properly ensures that only classes inheriting from
StrictBaseModelcan use themirror_pybind_fieldsdecorator, maintaining consistency with the new strict validation approach.
1399-1416: Safe attribute access pattern.Using
getattr(self, "build_config", None)is the correct approach to avoid AttributeError whenbuild_configmight not be set yet during initialization.
2131-2132: Correct usage of centralized batch size generation.The calls to
CudaGraphConfig._generate_cuda_graph_batch_sizesproperly use the newly centralized static method, eliminating code duplication.Also applies to: 2143-2144
tests/unittest/llmapi/test_llm_args.py (1)
477-700: Excellent test coverage for strict validation!The
TestStrictBaseModelArbitraryArgsclass provides comprehensive test coverage for the new strict validation behavior:
- Tests all configuration classes that now inherit from
StrictBaseModel- Verifies both acceptance of valid arguments and rejection of arbitrary fields
- Includes tests for nested configurations
- Tests the base
StrictBaseModelbehavior directlyThe test implementation is thorough and well-structured.
|
PR_Github #13299 [ run ] triggered by Bot |
|
PR_Github #13299 [ run ] completed with state |
|
/bot run |
|
PR_Github #13326 [ run ] triggered by Bot |
Superjomn
changed the title
|
PR_Github #13326 [ run ] completed with state |
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Lanyu Liao <[email protected]>
Signed-off-by: Superjomn <[email protected]>
Summary by CodeRabbit
New Features
Tests
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.