[TRTLLM-6420][feat] add support for Eclairv2 model - cherry-pick changes and minor fix #6493
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for a new "eclair" model variant, integrating a RADIO vision encoder with an MBart decoder. This includes new command-line flags, model configuration and weight conversion logic, model loading and initialization routines, and engine building processes. Additional dependencies and utility imports are added to support the new model. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CheckpointConverter
participant EngineBuilder
participant RadioWithNeck
participant VisionEncoderDecoderModel
participant Processor
User->>CLI: Run with --eclair_radio or --model_type eclair
CLI->>CheckpointConverter: Parse args, detect eclair_radio
CheckpointConverter->>CheckpointConverter: Parse config, adjust for eclair_radio
CheckpointConverter->>CheckpointConverter: Pad vocab, adjust weights
CheckpointConverter->>RadioWithNeck: Instantiate vision encoder
CheckpointConverter->>VisionEncoderDecoderModel: Initialize decoder
CheckpointConverter->>Processor: Setup processor, extend tokenizer
CheckpointConverter->>VisionEncoderDecoderModel: Load weights, adjust embeddings
CLI->>EngineBuilder: Build engine with model_type eclair
EngineBuilder->>build_eclair_engine: Call build_eclair_engine(args)
build_eclair_engine->>RadioWithNeck: Load and wrap RADIO model
build_eclair_engine->>VisionEncoderDecoderModel: Replace encoder, adjust decoder
build_eclair_engine->>Processor: Load processor, resize embeddings
build_eclair_engine->>EngineBuilder: Export ONNX, build TensorRT engine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
yibinl-nvidia
changed the title
|
/bot run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
examples/models/core/multimodal/requirements-eclair.txt (1)
1-1: Consider pinning the timm version for reproducibility.While the
timmdependency is appropriate for the RADIO vision encoder, consider specifying a version to ensure reproducible builds and avoid potential compatibility issues.-timm +timm>=0.9.0,<1.0.0tensorrt_llm/tools/multimodal_builder.py (2)
35-46: Consider maintaining alphabetical order in model type choices.The new "eclair" model type should be placed between "cogvlm" and "fuyu" to maintain alphabetical ordering consistency.
choices=[ 'blip2', 'llava', 'llava_next', 'llava_onevision', 'llava_onevision_lmms', 'vila', 'nougat', 'cogvlm', 'fuyu', 'pix2struct', 'neva', 'kosmos-2', 'video-neva', 'phi-3-vision', 'phi-4-multimodal', 'mllama', 'internvl', 'qwen2_vl', - 'internlm-xcomposer2', 'qwen2_audio', 'pixtral', 'eclair' + 'internlm-xcomposer2', 'qwen2_audio', 'eclair', 'pixtral' ],
1748-1785: Consider adding architecture documentation.The
RadioWithNeckclass implements a specific vision encoder architecture, but lacks documentation about its design choices. Consider adding docstrings to explain:
- Why 1280→1024 channel reduction is used
- The purpose of the (1,4) kernel convolution
- The expected input/output tensor shapes
class RadioWithNeck(torch.nn.Module): + """Custom vision encoder combining RADIO with additional neck layers. + + Architecture: + - RADIO encoder (v2.5-h) for feature extraction + - Conv1d: 1280→1024 channel reduction + - LayerNorm + - Conv2d: (1,4) kernel for spatial downsampling + - LayerNorm + + Input: (B, 3, H, W) image tensor + Output: (B, H*W/256, 1024) feature tensor + """ def __init__(self):examples/models/core/enc_dec/convert_checkpoint.py (2)
1514-1514: Document the HTTP 403 workaround.The line
torch.hub._validate_not_a_forked_repo = lambda a, b, c: Trueis overriding an internal torch.hub function to avoid HTTP 403 errors. This workaround should be properly documented with a comment explaining why it's necessary and any potential risks.- torch.hub._validate_not_a_forked_repo = lambda a, b, c: True # avoid HTTP 403 error + # WORKAROUND: Override torch.hub validation to avoid HTTP 403 errors + # when loading RADIO model from NVlabs repository + torch.hub._validate_not_a_forked_repo = lambda a, b, c: True
1507-1545: Consider extracting RadioWithNeck class to a separate module.The
RadioWithNeckclass definition inside theget_modelfunction makes the code harder to maintain and test. Consider moving this class to a separate module for better organization and reusability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/models/core/enc_dec/convert_checkpoint.py(11 hunks)examples/models/core/multimodal/requirements-eclair.txt(1 hunks)tensorrt_llm/models/enc_dec/model.py(2 hunks)tensorrt_llm/tools/multimodal_builder.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile).
Python global variables should use upper snake_case and prefix G (e.g., G_MY_GLOBAL).
Python constants should use upper snake_case (e.g., MY_CONSTANT).
Avoid shadowing variables declared in an outer scope in Python.
Initialize all externally visible members of a Python class in the constructor.
For interfaces that may be used outside a file, prefer docstrings over comments in Python.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for 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 reflection.
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/models/enc_dec/model.pytensorrt_llm/tools/multimodal_builder.pyexamples/models/core/enc_dec/convert_checkpoint.py
**/*.{cpp,h,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/models/enc_dec/model.pytensorrt_llm/tools/multimodal_builder.pyexamples/models/core/enc_dec/convert_checkpoint.py
🧠 Learnings (2)
tensorrt_llm/models/enc_dec/model.py (2)
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.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-07-30T06:11:42.362Z
Learning: Applies to **/*.py : The code developed for TensorRT-LLM should conform to Python 3.8+.
tensorrt_llm/tools/multimodal_builder.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)
examples/models/core/enc_dec/convert_checkpoint.py
1594-1594: Local variable device is assigned to but never used
Remove assignment to unused variable device
(F841)
1598-1598: Local variable checkpoint_path is assigned to but never used
Remove assignment to unused variable checkpoint_path
(F841)
🔇 Additional comments (12)
tensorrt_llm/models/enc_dec/model.py (2)
23-24: LGTM! Proper import addition for vocabulary padding.The addition of
pad_vocab_sizeimport is appropriate and follows the existing import pattern in the utilities section.
1160-1164: LGTM! Correct implementation of vocabulary size padding for tensor parallelism.The vocabulary padding logic correctly uses
pad_vocab_sizeto ensure the vocabulary size is divisible by the tensor parallel size, which is essential for proper tensor parallelism support. This change aligns well with the checkpoint conversion logic mentioned in the AI summary.tensorrt_llm/tools/multimodal_builder.py (2)
28-28: LGTM!The import for
safetensorsis appropriate and follows the existing import organization pattern.
147-148: LGTM!The conditional branch for the "eclair" model type is correctly implemented and follows the existing pattern.
examples/models/core/enc_dec/convert_checkpoint.py (8)
17-17: Import statement looks good.The addition of
NougatProcessorto the imports is appropriate for the new eclair_radio model support.
21-21: Import follows namespace convention.Good adherence to the coding guideline of maintaining namespace when importing (
from tensorrt_llm._utils import pad_vocab_size).
34-36: Constant definition is clear and follows naming conventions.The constant
ECLAIR_RADIO_MAX_POSITION_EMBEDDINGS = 20000follows the upper snake_case naming convention for constants as specified in the coding guidelines.
626-636: Configuration parsing updates are well-structured.The changes to
parse_bart_configfunction properly handle the eclair_radio model:
- Correctly uses the full model config for decoder when
args.eclair_radiois true- Maintains consistency with nougat model handling pattern
- Appropriately overrides
n_positionswith the defined constantAlso applies to: 638-639, 775-776, 779-782
968-983: Vocabulary padding implementation is correct and necessary.The addition of vocabulary padding logic ensures proper tensor parallelism when the vocabulary size is not divisible by
mapping.tp_size. The implementation correctly:
- Uses the
pad_vocab_sizeutility to calculate padded size- Applies zero padding to the weight tensor
- Updates the vocab_size for subsequent reshaping
Note: The local
import torchat line 968 is acceptable for lazy loading but could be moved to the top-level imports for consistency.
1546-1583: Tokenizer configuration is comprehensive and well-structured.The
get_processorfunction properly configures the tokenizer with:
- Special tokens for different output modes (plain, markdown, OCR, etc.)
- Coordinate tokens for bounding box support (1024x1280 resolution)
- Class tokens for document element classification
The implementation correctly adds tokens and updates the tokenizer attributes.
1669-1670: Checkpoint conversion logic properly handles eclair_radio model.The updates to skip encoder conversion for eclair_radio models are consistent with the handling of nougat and pix2struct models, which is appropriate since eclair_radio uses a vision encoder that doesn't require the standard text encoder conversion.
Also applies to: 1806-1807, 1816-1817
1879-1881: Command-line argument follows established pattern.The new
--eclair_radioargument is properly added with the same description as--nougat, maintaining consistency in the CLI interface.
|
/bot run |
|
PR_Github #13574 [ run ] triggered by Bot |
|
PR_Github #13574 [ run ] completed with state |
|
/bot run |
|
PR_Github #13580 [ run ] triggered by Bot |
|
PR_Github #13580 [ run ] completed with state |
yibinl-nvidia
force-pushed
the
aw632-eclair-v2
branch
from
f1fe84b to
42d77e4
Compare
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tensorrt_llm/tools/multimodal_builder.py (1)
1746-1810: Address the previously identified issues.The same issues identified in the previous review still need to be addressed:
- Network dependency: The
torch.hub.loadcall requires network access and could fail- Hard-coded dimensions: Image dimensions (2048, 1648) should be configurable
- Temporary fix needs tracking: Add TODO comment for the fused attention workaround
- Dtype inconsistency: Model uses bfloat16 but dummy input uses float16
Please refer to the previous review comments for the detailed implementation suggestions.
examples/models/core/enc_dec/convert_checkpoint.py (1)
1594-1594: Remove unused variables flagged by static analysis.The variables
device(line 1594) andcheckpoint_path(lines 1598-1601) are assigned but never used. These should be removed to clean up the code.- device, d_model = model.device, model.config.decoder.d_model + d_model = model.config.decoder.d_model with torch.inference_mode(): - # Inspect checkpoint shapes - checkpoint_path = os.path.join(args.model_dir, - "model.safetensors")Also applies to: 1598-1601
🧹 Nitpick comments (1)
examples/models/core/enc_dec/convert_checkpoint.py (1)
506-618: Well-implemented eclair model setup with some cleanup opportunities.The eclair_radio model implementation is comprehensive and follows good practices:
- Clear separation of RadioWithNeck encoder module
- Proper processor setup with special tokens
- Correct integration with nougat-base architecture
Consider these improvements:
- Remove commented code (lines 1600-1602): Clean up the commented safetensors inspection code
- Add error handling for the torch.hub.load and safetensors loading operations
- Document the HTTP 403 workaround (line 1514): Add a comment explaining why the validation override is needed
- # with safetensors.safe_open(checkpoint_path, framework="pt") as f: - # if "decoder.model.decoder.embed_tokens.weight" in f.keys(): - # embed_shape = f.get_tensor("decoder.model.decoder.embed_tokens.weight").shape
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/models/core/enc_dec/convert_checkpoint.py(11 hunks)examples/models/core/multimodal/requirements-eclair.txt(1 hunks)tensorrt_llm/models/enc_dec/model.py(2 hunks)tensorrt_llm/tools/multimodal_builder.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/models/core/multimodal/requirements-eclair.txt
- tensorrt_llm/models/enc_dec/model.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+.
Indent Python code with 4 spaces. Do not use tabs.
Always maintain the namespace when importing in Python, even if only one class or function from a module is used.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case, 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 file, prefer docstrings over comments in Python.
Comments in Python should be reserved for code within a function, or interfaces that are local to a file.
Use Google style docstrings for classes and functions in Python, which can be parsed by Sphinx.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the docstring for the class.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.
Files:
tensorrt_llm/tools/multimodal_builder.pyexamples/models/core/enc_dec/convert_checkpoint.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/tools/multimodal_builder.pyexamples/models/core/enc_dec/convert_checkpoint.py
🧠 Learnings (1)
tensorrt_llm/tools/multimodal_builder.py (2)
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.
Learnt from: amitz-nv
PR: #5616
File: tensorrt_llm/executor/worker.py:375-384
Timestamp: 2025-07-17T09:01:27.402Z
Learning: In tensorrt_llm/executor/worker.py, the LoRA adapter cache optimization logic that checks is_adapter_in_cpu_cache() and conditionally passes None for weights/config has a known race condition issue that cannot be solved with simple error handling or verification checks. This is a known limitation that requires a more comprehensive solution.
🪛 Ruff (0.12.2)
examples/models/core/enc_dec/convert_checkpoint.py
1594-1594: Local variable device is assigned to but never used
Remove assignment to unused variable device
(F841)
1598-1598: Local variable checkpoint_path is assigned to but never used
Remove assignment to unused variable checkpoint_path
(F841)
⏰ 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 (9)
tensorrt_llm/tools/multimodal_builder.py (3)
28-28: LGTM!The safetensors import addition is appropriate and necessary for the new eclair model loading functionality.
34-46: LGTM!The addition of 'eclair' to the model_type choices is correctly implemented and follows the existing pattern.
147-148: LGTM!The elif condition for eclair model type is correctly implemented and follows the existing pattern in the build method.
examples/models/core/enc_dec/convert_checkpoint.py (6)
17-17: LGTM! New imports support eclair model functionality.The
NougatProcessorandpad_vocab_sizeimports are correctly added to support the new eclair model variant with proper processor setup and vocabulary padding for tensor parallelism.Also applies to: 21-21
34-36: LGTM! Well-defined constant for eclair model configuration.The
ECLAIR_RADIO_MAX_POSITION_EMBEDDINGSconstant is properly defined and used consistently throughout the codebase for position embedding sizing in the eclair model.
626-637: LGTM! Configuration parsing properly handles eclair model.The configuration parsing changes correctly handle the eclair_radio model by:
- Using root config instead of decoder-specific config when needed
- Following the established pattern similar to nougat model handling
- Properly overriding position embeddings with the defined constant
The logic is consistent and follows the expected behavior for vision encoder + mbart decoder models.
Also applies to: 638-638, 775-775, 780-782
1669-1670: LGTM! Consistent encoder conversion exclusion logic.The conditions properly exclude encoder conversion for eclair_radio models, following the established pattern for nougat models. This is correct since eclair_radio uses a RADIO vision encoder that doesn't require the standard checkpoint conversion process.
Also applies to: 1805-1806, 1816-1817
968-983: LGTM! Proper vocabulary padding for tensor parallelism alignment.The lm_head weight padding logic correctly handles cases where vocabulary size is not divisible by tensor parallelism size:
- Uses the appropriate
pad_vocab_sizeutility function- Pads with zeros to maintain model correctness
- Updates vocab_size for consistent downstream operations
This is essential for proper tensor parallelism alignment.
1879-1881: LGTM! Clear command line argument for eclair model.The
--eclair_radioargument is properly defined with appropriate help text that accurately describes the model architecture as using a vision encoder with mbart decoder.
|
PR_Github #13624 [ run ] triggered by Bot |
|
PR_Github #13624 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #13700 [ run ] triggered by Bot |
|
PR_Github #13700 [ run ] completed with state |
yibinl-nvidia
force-pushed
the
aw632-eclair-v2
branch
from
42d77e4 to
5c76e24
Compare
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/models/core/enc_dec/convert_checkpoint.py (1)
1594-1594: Remove unused variables flagged by static analysis.The variables
device(line 1594) andcheckpoint_path(line 1598) are assigned but never used. These should be removed to clean up the code.Apply this diff to remove the unused variables:
- device, d_model = model.device, model.config.decoder.d_model + d_model = model.config.decoder.d_model with torch.inference_mode(): - # Inspect checkpoint shapes - checkpoint_path = os.path.join(args.model_dir, - "model.safetensors") - # with safetensors.safe_open(checkpoint_path, framework="pt") as f: - # if "decoder.model.decoder.embed_tokens.weight" in f.keys(): - # embed_shape = f.get_tensor("decoder.model.decoder.embed_tokens.weight").shape safetensors.torch.load_model(model, os.path.join( args.model_dir, "model.safetensors"), strict=False)Also applies to: 1598-1601
🧹 Nitpick comments (1)
examples/models/core/enc_dec/convert_checkpoint.py (1)
509-544: Consider improving the RadioWithNeck class implementation.While the implementation is functional, consider these improvements:
- Add docstrings to document the class and its forward method
- Replace the torch.hub hack with a more robust solution for HTTP 403 errors
- Define magic numbers as constants (1280, 1024, kernel sizes, etc.)
Example improvements:
+# Vision encoder dimensions +RADIO_FEATURE_DIM = 1280 +NECK_HIDDEN_DIM = 1024 +CONV2D_KERNEL_SIZE = (1, 4) +CONV2D_STRIDE = (1, 4) class RadioWithNeck(torch.nn.Module): + """Vision encoder that combines RADIO model with neck layers for feature processing.""" def __init__(self): super().__init__() - torch.hub._validate_not_a_forked_repo = lambda a, b, c: True # avoid HTTP 403 error + # TODO: Replace with proper authentication or offline model loading + torch.hub._validate_not_a_forked_repo = lambda a, b, c: True self.model_encoder = torch.hub.load("NVlabs/RADIO", "radio_model", version="radio_v2.5-h") self.model_encoder.summary_idxs = torch.tensor(4) - self.conv1 = torch.nn.Conv1d(1280, 1024, 1) - self.layer_norm1 = torch.nn.LayerNorm(1024, eps=1e-6, elementwise_affine=True) + self.conv1 = torch.nn.Conv1d(RADIO_FEATURE_DIM, NECK_HIDDEN_DIM, 1) + self.layer_norm1 = torch.nn.LayerNorm(NECK_HIDDEN_DIM, eps=1e-6, elementwise_affine=True)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/models/core/enc_dec/convert_checkpoint.py(11 hunks)examples/models/core/multimodal/requirements-eclair.txt(1 hunks)tensorrt_llm/models/enc_dec/model.py(2 hunks)tensorrt_llm/tools/multimodal_builder.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/models/core/multimodal/requirements-eclair.txt
- tensorrt_llm/models/enc_dec/model.py
- tensorrt_llm/tools/multimodal_builder.py
🧰 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:
examples/models/core/enc_dec/convert_checkpoint.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:
examples/models/core/enc_dec/convert_checkpoint.py
🪛 Ruff (0.12.2)
examples/models/core/enc_dec/convert_checkpoint.py
1594-1594: Local variable device is assigned to but never used
Remove assignment to unused variable device
(F841)
1598-1598: Local variable checkpoint_path is assigned to but never used
Remove assignment to unused variable checkpoint_path
(F841)
🔇 Additional comments (5)
examples/models/core/enc_dec/convert_checkpoint.py (5)
17-17: LGTM! Required imports for eclair_radio functionality.The new imports are properly used later in the code -
NougatProcessorfor processor setup andpad_vocab_sizefor vocabulary padding in tensor parallelism.Also applies to: 21-21
34-36: LGTM! Well-defined constant for eclair_radio configuration.The constant follows proper naming conventions and provides a clear, reusable value for the maximum position embeddings in the eclair_radio model.
626-642: LGTM! Configuration parsing properly handles eclair_radio model.The changes correctly follow the established pattern for handling vision encoder + decoder models like nougat, with appropriate conditional logic and configuration overrides for the eclair_radio variant.
Also applies to: 775-782
968-983: LGTM! Proper vocabulary padding for tensor parallelism compatibility.The implementation correctly pads the vocabulary size and
lm_head.weighttensor with zeros when needed for tensor parallelism, following established patterns for distributed training compatibility.
1669-1670: LGTM! Consistent conditional logic for eclair_radio model handling.The conditional checks correctly group
eclair_radiowithnougatto skip encoder conversion, following the established pattern for vision encoder + decoder models.Also applies to: 1805-1806, 1816-1817
|
PR_Github #13729 [ run ] triggered by Bot |
|
PR_Github #13729 [ run ] completed with state |
yibinl-nvidia
force-pushed
the
aw632-eclair-v2
branch
2 times, most recently
from
5c76e24 to
be2c4d5
Compare
yibinl-nvidia
force-pushed
the
aw632-eclair-v2
branch
from
2794bcc to
a2d1b4d
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #14561 [ run ] triggered by Bot |
|
PR_Github #14561 [ run ] completed with state |
yibinl-nvidia
changed the title
yibinl-nvidia
changed the title
Signed-off-by: Andrew Wang <[email protected]>
Signed-off-by: Andrew Wang <[email protected]>
Signed-off-by: Andrew Wang <[email protected]>
Signed-off-by: Andrew Wang <[email protected]>
Signed-off-by: Andrew Wang <[email protected]>
Signed-off-by: Yibin Li <[email protected]>
Signed-off-by: Yibin Li <[email protected]>
Signed-off-by: Yibin Li <[email protected]>
Signed-off-by: Yibin Li <[email protected]>
nv-guomingz
force-pushed
the
aw632-eclair-v2
branch
from
a2d1b4d to
80438b2
Compare
|
/bot reuse-pipeline |
|
PR_Github #14660 [ reuse-pipeline ] triggered by Bot |
|
PR_Github #14660 [ reuse-pipeline ] completed with state |
Summary by CodeRabbit
--eclair_radio) to enable the new model variant.Description
Cherry-pick changes from #5686.
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.