[TRTLLM-11318][feat] move VisualGen APIs to a separate dir#12538
Open
zhenhuaw-me wants to merge 1 commit intoNVIDIA:mainfrom
Open
[TRTLLM-11318][feat] move VisualGen APIs to a separate dir#12538zhenhuaw-me wants to merge 1 commit intoNVIDIA:mainfrom
zhenhuaw-me wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
requested review from
FrankD412,
hchings,
kaiyux,
laikhtewari,
tburt-nv and
yuanjingx87
zhenhuaw-me
requested review from
QiJune,
Superjomn,
chang-l,
litaotju and
schetlur-nv
Member
Author
|
/bot run |
Contributor
📝 WalkthroughWalkthroughThis pull request relocates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/__init__.py`:
- Line 136: The file header year was not updated when this file was modified;
open the module that exports VisualGen and VisualGenParams (the top of
tensorrt_llm/__init__.py where VisualGen, VisualGenParams are imported) and
update the copyright header year range (replace 2024 with the current year) to
reflect the modification year.
In `@tensorrt_llm/serve/openai_server.py`:
- Line 40: Replace direct class/function imports with module-level imports and
update all usages to reference the module namespace: change "from
tensorrt_llm.llmapi import MultimodalEncoder, tracing" to "import
tensorrt_llm.llmapi as llmapi" and update references to llmapi.MultimodalEncoder
and llmapi.tracing.extract_trace_headers(...); similarly import the visual_gen
module (e.g., "import tensorrt_llm.visual_gen as visual_gen") and replace
VisualGen and VisualGenParams usages with visual_gen.VisualGen and
visual_gen.VisualGenParams so all symbols are accessed through their module
namespaces.
In `@tensorrt_llm/serve/visual_gen_utils.py`:
- Line 12: This file (tensorrt_llm/serve/visual_gen_utils.py) is missing the
required NVIDIA Apache/SPDX header; add the standard NVIDIA Apache 2.0 header
block (including SPDX-License-Identifier: Apache-2.0 and the copyright notice
with the year of the latest meaningful modification) at the top of the file
above the existing imports (e.g., above the line importing VisualGenParams),
ensuring the header format matches other TensorRT-LLM source files.
In `@tensorrt_llm/visual_gen/__init__.py`:
- Line 17: The __all__ export list in __init__.py is not sorted in isort-style
order; update the module-level __all__ to an alphabetically sorted list of the
exported names (VisualGen, VisualGenParams, MediaOutput) so it satisfies Ruff
RUF022 — locate the __all__ declaration in tensorrt_llm.visual_gen.__init__ and
reorder the entries into isort-style alphabetical order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f025dcb3-6d47-4ad3-850f-cd14ee11a2f6
📒 Files selected for processing (10)
.github/CODEOWNERSdocs/source/models/visual-generation.mdtensorrt_llm/__init__.pytensorrt_llm/bench/benchmark/visual_gen.pytensorrt_llm/llmapi/__init__.pytensorrt_llm/serve/openai_server.pytensorrt_llm/serve/visual_gen_utils.pytensorrt_llm/visual_gen/__init__.pytensorrt_llm/visual_gen/visual_gen.pytests/unittest/_torch/visual_gen/test_visual_gen_args.py
💤 Files with no reviewable changes (1)
- tensorrt_llm/llmapi/init.py
| from .python_plugin import PluginBase | ||
| from .sampling_params import SamplingParams | ||
| from .version import __version__ | ||
| from .visual_gen import VisualGen, VisualGenParams |
Contributor
There was a problem hiding this comment.
Update the copyright year on this modified file.
The file changed in this PR (Line 136), but the header at Line 1 still ends at 2024.
📝 Proposed fix
-# SPDX-FileCopyrightText: Copyright (c) 2022-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-FileCopyrightText: Copyright (c) 2022-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from .visual_gen import VisualGen, VisualGenParams | |
| # SPDX-FileCopyrightText: Copyright (c) 2022-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/__init__.py` at line 136, The file header year was not updated
when this file was modified; open the module that exports VisualGen and
VisualGenParams (the top of tensorrt_llm/__init__.py where VisualGen,
VisualGenParams are imported) and update the copyright header year range
(replace 2024 with the current year) to reflect the modification year.
| from tensorrt_llm.llmapi import DisaggregatedParams as LlmDisaggregatedParams | ||
| from tensorrt_llm.llmapi import (MultimodalEncoder, VisualGen, VisualGenParams, | ||
| tracing) | ||
| from tensorrt_llm.llmapi import MultimodalEncoder, tracing |
Contributor
There was a problem hiding this comment.
Use module-level imports instead of importing classes directly.
Line 40 and Line 89 import symbols directly from packages, which conflicts with the repo’s namespace-import rule. Please import modules and reference symbols through module namespaces.
Proposed direction
-from tensorrt_llm.llmapi import MultimodalEncoder, tracing
+from tensorrt_llm import llmapi
...
-from tensorrt_llm.visual_gen import VisualGen, VisualGenParams
+from tensorrt_llm import visual_genThen update usages, e.g.:
MultimodalEncoder→llmapi.MultimodalEncodertracing.extract_trace_headers(...)→llmapi.tracing.extract_trace_headers(...)VisualGen/VisualGenParams→visual_gen.VisualGen/visual_gen.VisualGenParams
As per coding guidelines, "When importing in Python, always maintain the namespace. Import the module, not individual classes or functions..."
Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/serve/openai_server.py` at line 40, Replace direct
class/function imports with module-level imports and update all usages to
reference the module namespace: change "from tensorrt_llm.llmapi import
MultimodalEncoder, tracing" to "import tensorrt_llm.llmapi as llmapi" and update
references to llmapi.MultimodalEncoder and
llmapi.tracing.extract_trace_headers(...); similarly import the visual_gen
module (e.g., "import tensorrt_llm.visual_gen as visual_gen") and replace
VisualGen and VisualGenParams usages with visual_gen.VisualGen and
visual_gen.VisualGenParams so all symbols are accessed through their module
namespaces.
| ImageGenerationRequest, | ||
| VideoGenerationRequest, | ||
| ) | ||
| from tensorrt_llm.visual_gen import VisualGenParams |
Contributor
There was a problem hiding this comment.
Add the required NVIDIA Apache header to this modified Python file.
This file is modified in this PR (Line 12) but lacks the required SPDX/Apache header block.
📝 Proposed fix
+ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+ # SPDX-License-Identifier: Apache-2.0
+ #
+ # 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.
+
import asyncio
import base64
import os📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from tensorrt_llm.visual_gen import VisualGenParams | |
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| # SPDX-License-Identifier: Apache-2.0 | |
| # | |
| # 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. | |
| import asyncio | |
| import base64 | |
| import os | |
| from tensorrt_llm.visual_gen import VisualGenParams |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/serve/visual_gen_utils.py` at line 12, This file
(tensorrt_llm/serve/visual_gen_utils.py) is missing the required NVIDIA
Apache/SPDX header; add the standard NVIDIA Apache 2.0 header block (including
SPDX-License-Identifier: Apache-2.0 and the copyright notice with the year of
the latest meaningful modification) at the top of the file above the existing
imports (e.g., above the line importing VisualGenParams), ensuring the header
format matches other TensorRT-LLM source files.
| # limitations under the License. | ||
| from .visual_gen import MediaOutput, VisualGen, VisualGenParams | ||
|
|
||
| __all__ = ["VisualGen", "VisualGenParams", "MediaOutput"] |
Contributor
There was a problem hiding this comment.
Sort __all__ to satisfy Ruff RUF022.
__all__ at Line 17 is not sorted in isort-style order.
🔧 Proposed fix
-__all__ = ["VisualGen", "VisualGenParams", "MediaOutput"]
+__all__ = ["MediaOutput", "VisualGen", "VisualGenParams"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| __all__ = ["VisualGen", "VisualGenParams", "MediaOutput"] | |
| __all__ = ["MediaOutput", "VisualGen", "VisualGenParams"] |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 17-17: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/visual_gen/__init__.py` at line 17, The __all__ export list in
__init__.py is not sorted in isort-style order; update the module-level __all__
to an alphabetically sorted list of the exported names (VisualGen,
VisualGenParams, MediaOutput) so it satisfies Ruff RUF022 — locate the __all__
declaration in tensorrt_llm.visual_gen.__init__ and reorder the entries into
isort-style alphabetical order.
Collaborator
|
PR_Github #40319 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #40319 [ run ] completed with state
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
tensorrt_llm.visual_gen. Users importing from the top-leveltensorrt_llmpackage are unaffected.Chores
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.