[TRTLLM-12491][feat] Align VisualGen serve request schema with VisualGenParams#14733
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive tensor payload serialization support to TensorRT-LLM's visual generation service, enabling ChangesVisual Generation Request Validation, Tensor Support, and Parameter Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/serve/visual_gen_utils.py (1)
163-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed base64 at the request boundary.
Both decode paths call
base64.b64decode()with its default behavior, so malformed payloads can slip through as garbage bytes and fail much later in the pipeline. Decode withvalidate=Trueand translatebinascii.Errorinto aValueErrorhere instead.🛡️ Proposed fix
+import binascii import base64 import os import shutil @@ +def _decode_base64_payload(value: str, field_name: str) -> bytes: + try: + return base64.b64decode(value, validate=True) + except binascii.Error as exc: + raise ValueError( + f"{field_name} must be valid base64-encoded data" + ) from exc + + def parse_visual_gen_params( @@ ref_path = os.path.join(media_storage_path, f"{request_id}_reference.png") if isinstance(request.input_reference, str): with open(ref_path, "wb") as f: - f.write(base64.b64decode(request.input_reference)) + f.write( + _decode_base64_payload( + request.input_reference, "input_reference" + ) + ) @@ if request.image is not None: if isinstance(request.image, list): - params.image = [base64.b64decode(image) for image in request.image] + params.image = [ + _decode_base64_payload(image, "image") + for image in request.image + ] else: - params.image = [base64.b64decode(request.image)] + params.image = [_decode_base64_payload(request.image, "image")]Run this to confirm the current call sites are relying on the stdlib default and are not passing
validate=True:#!/bin/bash set -euo pipefail python - <<'PY' import base64 import inspect print(inspect.signature(base64.b64decode)) PY rg -n 'b64decode\(' tensorrt_llm/serve/visual_gen_utils.pyAlso applies to: 200-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/serve/visual_gen_utils.py` around lines 163 - 165, The base64 decoding of request.input_reference in visual_gen_utils.py should reject malformed input: change the call to base64.b64decode(request.input_reference, validate=True) and wrap it in a try/except that catches binascii.Error and raises a ValueError (with a clear message) before writing to ref_path; apply the same change to the other decode site handling request.input_reference (around the decode at lines ~200-203) so both decode paths validate input and surface a ValueError at the request boundary.
🧹 Nitpick comments (3)
tests/unittest/_torch/visual_gen/test_visual_gen_params.py (1)
1189-1274: ⚡ Quick winCoverage is still insufficient for the public validation path.
These additions stop at
DiffusionResponse. The two new API contracts intensorrt_llm/visual_gen/visual_gen.py—VisualGen.validate_request_params()and single-promptVisualGenResultre-raisingVisualGenValidationError—still need direct coverage. Please add cases intests/unittest/_torch/visual_gen/test_visual_gen_params.pyor a neighboringtests/unittest/_torch/visual_gen/test_visual_gen.py. As per coding guidelinestests/**: Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/visual_gen/test_visual_gen_params.py` around lines 1189 - 1274, Add unit tests that directly exercise the public validation API: call VisualGen.validate_request_params(...) with params that trigger a validation failure and assert it raises VisualGenValidationError containing structured fields (error_reason, error_param, error_details including pipeline name). Also add a test that runs the single-prompt code path which constructs/returns a VisualGenResult that should re-raise VisualGenValidationError for invalid params and verify the exception propagates and carries the same structured fields; reference the VisualGen.validate_request_params, VisualGenResult, and VisualGenValidationError symbols when locating where to add these cases.tensorrt_llm/serve/openai_server.py (1)
2060-2066: ⚡ Quick winNarrow the terminal catch in this endpoint.
Catching every
Exceptionhere will turn unrelated bugs in the new tensor/image serialization path into generic 500 payloads, which makes regressions much harder to spot. Catch the expected storage/encoding failures explicitly and let the rest propagate through FastAPI's normal error handling.
As per coding guidelines, "Avoid broad exception handling — catch specific exceptions, not bareexcept:" and "When using try-except blocks, limit the except clause to the smallest set of errors possible."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/serve/openai_server.py` around lines 2060 - 2066, The current broad except Exception in the endpoint should be narrowed to only the expected storage/encoding-related errors so unrelated bugs bubble up; replace "except Exception as e:" with an explicit tuple of expected exceptions (e.g. UnicodeEncodeError, ValueError, TypeError, OSError and any project-specific errors like ImageSerializationError or StorageError if those exist) and keep the logger.error(...) + return self.create_error_response(...) inside that narrowed except, but for all other exceptions simply re-raise (use "raise") so FastAPI handles them normally.tests/unittest/_torch/visual_gen/test_visual_gen_utils.py (1)
229-255: ⚡ Quick winAdd malformed-base64 coverage for the new decode paths.
tests/unittest/_torch/visual_gen/test_visual_gen_utils.pycovers happy-path materialization and missingmedia_storage_path, but it still misses the case that would catch malformedinput_reference/ImageEditRequest.imagepayloads early. Please add directValueErrorassertions for those two request shapes once the request-boundary validation is tightened.As per coding guidelines "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/visual_gen/test_visual_gen_utils.py` around lines 229 - 255, Add tests that assert malformed base64 payloads raise ValueError: create two new test cases in the TestInputReferenceMaterialization class that call parse_visual_gen_params with (a) a VideoGenerationRequest whose input_reference is an invalid base64 string and (b) an ImageEditRequest (or equivalent request shape) whose image field is invalid base64; pass a valid media_storage_path (tmp_path) and assert pytest.raises(ValueError, match="input_reference" or "image" respectively) to ensure parse_visual_gen_params rejects malformed payloads early. Use the existing _StubVisualGen generator and the same invocation pattern as the other tests so the new tests mirror test_base64_reference_written_to_disk and test_missing_media_storage_path_raises.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/visual_gen/serve/async_video_gen.py`:
- Around line 36-37: Rename the parameter and local variable named format in
test_async_video_generation to format_ to avoid builtin shadowing (Ruff A002);
update the function signature to use format_, replace local references ("if
format in ...", "actual_ext = f'.{format}'", and the dict entry "format":
format) to use format_, and update any callers (e.g., pass format_=args.format)
while keeping the outgoing JSON/request payload key as "format" (so the dict
remains "format": format_).
In `@examples/visual_gen/serve/sync_image_gen.py`:
- Around line 31-33: Rename the function parameter `format` in
`test_image_generation` to avoid shadowing the builtin (e.g., `format_`) and
update all references accordingly: change the `format` parameter declaration,
replace uses like `extra_body={"format": format}` with `extra_body={"format":
format_}`, replace `ext_map[format]` with `ext_map[format_]`, and update the
caller site that passes this argument (e.g., replace `format=args.format` with
`format_=args.format`) so all forwards and lookups use `format_`.
In `@examples/visual_gen/serve/sync_video_gen.py`:
- Around line 35-36: The function parameter named format should be renamed to
format_ to avoid shadowing the built-in format() (change the parameter in the
function signature and all references inside the function), update all places
that construct request payloads to keep the JSON key "format" but use the new
variable (e.g., "format": format_), and update the call site(s) including
test_sync_video_generation to pass format_ (or the same literal) accordingly;
search for references to format in this function and replace them with format_
(including the payloads at the lines noted) and ensure imports/other symbols are
unchanged.
In `@tensorrt_llm/_torch/visual_gen/executor.py`:
- Around line 199-203: The error message appended to messages incorrectly says
the parameter "will be silently ignored" even though the branch raises
VisualGenValidationError; update the message in the messages.append call to
state that the field (field_name) is rejected for the given pipeline
(pipeline_name) because it is not in default_generation_params and will cause a
VisualGenValidationError, so callers are not misled about silent ignoring.
Ensure the new text references field_name, pipeline_name and
default_generation_params to make intent clear.
In `@tensorrt_llm/media/tensor_payload.py`:
- Around line 233-235: The code accesses TENSOR_FORMAT_EXTENSIONS[fmt] before
validating fmt, which raises KeyError when path has no suffix; update the logic
in the function containing this snippet (the block using variables target, fmt
and TENSOR_FORMAT_EXTENSIONS, and called by serialize_visual_gen_output) to
first validate fmt is a supported key (e.g., if fmt not in
TENSOR_FORMAT_EXTENSIONS) and raise the same ValueError used by
serialize_visual_gen_output for invalid formats, then only call
TENSOR_FORMAT_EXTENSIONS[fmt] to normalize the suffix and proceed to
target.parent.mkdir(...).
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 613-641: The current _create_visual_gen_validation_error_response
in openai_server.py flattens VisualGenValidationError to a simple message and
loses structured metadata like reason/param/details; replace the generic
create_error_response call with a dedicated renderer (e.g.,
render_visual_gen_validation_error or VisualGenValidationErrorRenderer) that
detects instances of VisualGenValidationError, serializes and preserves its
fields (message, reason, param, details) into the JSON envelope, and returns the
correct HTTP status (use the intended 400/422 per spec). Update
_create_visual_gen_validation_error_response to call that renderer instead of
create_error_response, and apply the same renderer invocation in the video
routes handlers that currently rely on the generic path so the structured
validation transport is preserved end-to-end.
In `@tensorrt_llm/serve/openai_video_routes.py`:
- Around line 500-510: The fallback video lookup in the GET handler sets
video_path using job.output_path or by probing _KNOWN_VIDEO_OUTPUT_SUFFIXES for
"{video_id}{ext}", but it fails to probe the batch-indexed name
"{video_id}_0{ext}" used for non-tensor async jobs; as a result video_path
remains None and requests can 404 even when the first output exists. Update the
probe loop in the method that sets video_path (the block referencing video_path,
job.output_path and _KNOWN_VIDEO_OUTPUT_SUFFIXES) to also check for a candidate
with f"{video_id}_0{ext}" (same approach as delete_video()), picking that path
if it exists so the GET /v1/videos/{id}/content handler can recover stale or
missing job.output_path entries.
In `@tensorrt_llm/serve/visual_gen_utils.py`:
- Around line 92-97: The parameter named `id` in the function
`parse_visual_gen_params` shadows a Python builtin and should be renamed (e.g.,
to `request_id`) to satisfy Ruff A002; update the function signature from `id:
str` to `request_id: str`, replace every reference to `id` inside
`parse_visual_gen_params` (including where `VisualGenParams` is constructed) to
use `request_id`, and make the same rename for the other occurrence(s)
referenced around lines 162-163 so all local uses and internal variable
references match the new `request_id` name.
In `@tensorrt_llm/visual_gen/output.py`:
- Around line 289-319: The linter flags the parameter name format (Ruff A002) in
VisualGenOutput.save and VisualGenOutput.save_bytes but renaming it would be an
API break; update both method signatures to suppress the specific lint rule by
appending a targeted "# noqa: A002" to the format parameter declaration (i.e.,
the parameter named format in the def save(...) and def save_bytes(...)
signatures) so the name is preserved while silencing the Ruff warning.
In `@tensorrt_llm/visual_gen/visual_gen.py`:
- Around line 812-817: The code uses type(self).__name__ when calling
validate_visual_gen_params which always yields "VisualGen" and causes mismatched
pipeline identity; change the pipeline_name argument to use the resolved
identity on the instance (e.g. self.executor.pipeline_name) and fall back to an
instance field if needed: supply pipeline_name=getattr(self.executor,
"pipeline_name", getattr(self, "pipeline_name", type(self).__name__)); update
the validate_visual_gen_params call to use that expression so coordinator- and
worker-side validation share the same pipeline name.
In `@tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py`:
- Around line 1838-1840: The test uses a hardcoded fallback "/tmp/_vg" when
reading video_client.app.state.__dict__.get("media_storage_path", "/tmp/_vg"),
which triggers Ruff S108; update the test to use the tmp_path fixture instead
and set the environment/config value TRTLLM_MEDIA_STORAGE_PATH from tmp_path (or
assign tmp_path.as_posix()) before creating/initializing video_client so
video_client.app.state.__dict__.get("media_storage_path", ...) resolves to the
fixture-backed temp directory rather than the hardcoded "/tmp/_vg".
In `@tests/unittest/_torch/visual_gen/test_visual_gen_params.py`:
- Around line 763-764: The pytest.raises calls in test_visual_gen_params.py use
non-raw string regex patterns (e.g. match="image_cond_strength.*not use it")
which triggers RUF043; update those match= arguments to raw string literals
(prefix with r) for the pytest.raises invocations that call
self._validate(executor, req) so the regex metacharacters are treated literally
(e.g. change match="..." to match=r"...") for both occurrences around the
_validate test cases.
---
Outside diff comments:
In `@tensorrt_llm/serve/visual_gen_utils.py`:
- Around line 163-165: The base64 decoding of request.input_reference in
visual_gen_utils.py should reject malformed input: change the call to
base64.b64decode(request.input_reference, validate=True) and wrap it in a
try/except that catches binascii.Error and raises a ValueError (with a clear
message) before writing to ref_path; apply the same change to the other decode
site handling request.input_reference (around the decode at lines ~200-203) so
both decode paths validate input and surface a ValueError at the request
boundary.
---
Nitpick comments:
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 2060-2066: The current broad except Exception in the endpoint
should be narrowed to only the expected storage/encoding-related errors so
unrelated bugs bubble up; replace "except Exception as e:" with an explicit
tuple of expected exceptions (e.g. UnicodeEncodeError, ValueError, TypeError,
OSError and any project-specific errors like ImageSerializationError or
StorageError if those exist) and keep the logger.error(...) + return
self.create_error_response(...) inside that narrowed except, but for all other
exceptions simply re-raise (use "raise") so FastAPI handles them normally.
In `@tests/unittest/_torch/visual_gen/test_visual_gen_params.py`:
- Around line 1189-1274: Add unit tests that directly exercise the public
validation API: call VisualGen.validate_request_params(...) with params that
trigger a validation failure and assert it raises VisualGenValidationError
containing structured fields (error_reason, error_param, error_details including
pipeline name). Also add a test that runs the single-prompt code path which
constructs/returns a VisualGenResult that should re-raise
VisualGenValidationError for invalid params and verify the exception propagates
and carries the same structured fields; reference the
VisualGen.validate_request_params, VisualGenResult, and VisualGenValidationError
symbols when locating where to add these cases.
In `@tests/unittest/_torch/visual_gen/test_visual_gen_utils.py`:
- Around line 229-255: Add tests that assert malformed base64 payloads raise
ValueError: create two new test cases in the TestInputReferenceMaterialization
class that call parse_visual_gen_params with (a) a VideoGenerationRequest whose
input_reference is an invalid base64 string and (b) an ImageEditRequest (or
equivalent request shape) whose image field is invalid base64; pass a valid
media_storage_path (tmp_path) and assert pytest.raises(ValueError,
match="input_reference" or "image" respectively) to ensure
parse_visual_gen_params rejects malformed payloads early. Use the existing
_StubVisualGen generator and the same invocation pattern as the other tests so
the new tests mirror test_base64_reference_written_to_disk and
test_missing_media_storage_path_raises.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6af8a09-d290-45ed-9379-01a686b461c6
📒 Files selected for processing (29)
docs/source/models/visual-generation.mdexamples/visual_gen/serve/README.mdexamples/visual_gen/serve/async_video_gen.pyexamples/visual_gen/serve/sync_image_gen.pyexamples/visual_gen/serve/sync_video_gen.pytensorrt_llm/_torch/visual_gen/executor.pytensorrt_llm/_torch/visual_gen/models/flux/pipeline_flux.pytensorrt_llm/_torch/visual_gen/models/flux/pipeline_flux2.pytensorrt_llm/_torch/visual_gen/models/ltx2/pipeline_ltx2.pytensorrt_llm/_torch/visual_gen/models/ltx2/pipeline_ltx2_two_stages.pytensorrt_llm/_torch/visual_gen/models/wan/defaults.pytensorrt_llm/_torch/visual_gen/models/wan/pipeline_wan.pytensorrt_llm/_torch/visual_gen/models/wan/pipeline_wan_i2v.pytensorrt_llm/_torch/visual_gen/output.pytensorrt_llm/media/tensor_payload.pytensorrt_llm/serve/openai_protocol.pytensorrt_llm/serve/openai_server.pytensorrt_llm/serve/openai_video_routes.pytensorrt_llm/serve/visual_gen_utils.pytensorrt_llm/visual_gen/output.pytensorrt_llm/visual_gen/params.pytensorrt_llm/visual_gen/visual_gen.pytests/unittest/_torch/visual_gen/test_tensor_payload.pytests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.pytests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.pytests/unittest/_torch/visual_gen/test_utils.pytests/unittest/_torch/visual_gen/test_visual_gen_args.pytests/unittest/_torch/visual_gen/test_visual_gen_params.pytests/unittest/_torch/visual_gen/test_visual_gen_utils.py
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
May 29, 2026
…sor-format dispatch, drop 'renamed from' wording Address self-review notes from PR NVIDIA#14733: keep the diff focused, drop over-engineered single-use dicts, and remove historical-narration wording from a public README. Doc and wording cleanup: - Revert all edits to docs/source/models/visual-generation.md (separate doc refactor planned, out of scope here). - Drop the "(renamed from `output_format`)" phrasing on the serve example README; the diff and git log are the migration history. Tensor-format dispatch simplification: - Remove `TENSOR_FORMAT_EXTENSIONS` and `TENSOR_FORMAT_MEDIA_TYPES` module-level dicts in `tensorrt_llm/media/tensor_payload.py`. Both are 2-entry single-use lookups; `f".{fmt}"` and a literal `"application/octet-stream"` are clearer at the four call sites in openai_server.py and openai_video_routes.py. - Inline `_TENSOR_SUFFIX_TO_FORMAT` in `tensorrt_llm/visual_gen/output.py::_infer_format_from_path` as a one-line `_suffix_format` helper that recognizes the two known tensor suffixes directly. - Trim the "where the format is handled" sentence from `VisualGenOutput.save`'s `path:` docstring — the internal routing between `tensorrt_llm.media.encoding` and `tensorrt_llm.media.tensor_payload` is an implementation detail callers don't need. Verified on GB200: full visual_gen unit subsystem 359 passed / 0 failed in 60.33s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
May 29, 2026
… use stock ValueError Per self-review feedback on PR NVIDIA#14733: no custom exception layer on top of Python's standard error types. Validation failures now raise plain ``ValueError`` with the same multi-line message; the structured ``reason`` / ``param`` / ``details`` fields are removed. Core changes - Remove ``VisualGenValidationError`` class and the ``Literal`` / ``field`` imports it required. - Replace ``DiffusionResponse.error_reason`` / ``error_param`` / ``error_details`` with a single ``is_validation_error: bool`` discriminator. Same on ``VisualGenOutput``. The coordinator-side awaiter uses it to re-raise as ``ValueError`` (-> HTTP 400) vs ``RuntimeError`` (-> HTTP 500); routes catch the standard types directly and no longer import the custom class. - ``validate_visual_gen_params`` raises ``ValueError`` with the multi-line per-violation message. The per-category accumulator lists feeding into ``details`` are gone; the flat ``messages`` list builds the same human-readable content. Fix the "silently ignored" wording on the universal-field branch since that branch does raise. - READY signal now propagates ``pipeline_name`` so coordinator-side pre-validation uses the actual loaded pipeline's class name (was always "VisualGen"). Closes the worker/coordinator parity gap that CodeRabbit #10 flagged. Cascade (all driven by the drop) - Remove ``VisualGen.validate_request_params`` method. Async video route calls module-level ``validate_visual_gen_params`` directly with the propagated executor metadata. - ``VisualGenResult._resolved_value`` dispatches on ``out.is_validation_error`` (ValueError vs RuntimeError). - Sync image, sync video, and async video routes drop their ``except VisualGenValidationError`` arm; the existing ``except ValueError`` arm handles validation failures the same way (HTTP 400 + message). Tests - ``TestVisualGenValidationErrorStructured`` -> ``TestValidateVisualGenParamsMessages``: assert ``ValueError`` with expected substrings instead of structured fields. - ``TestValidationErrorTransport`` switches to asserting ``is_validation_error``. - ``TestRouteVisualGenValidationError`` -> ``TestRouteEngineValidationError``: sync routes inject a stock ``ValueError`` on the mock; async route triggers a real validator rejection by sending an unknown extra_params key against the mock's executor metadata. - ``MockVisualGen`` gains an ``executor`` ``SimpleNamespace`` with ``pipeline_name`` / ``default_generation_params`` / ``extra_param_specs`` so the async pre-flight call works end-to-end. Verified on GB200: full visual_gen unit subsystem 357 passed / 0 failed in 60.67s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
May 29, 2026
…sor-format dispatch, drop 'renamed from' wording Address self-review notes from PR NVIDIA#14733: keep the diff focused, drop over-engineered single-use dicts, and remove historical-narration wording from a public README. Doc and wording cleanup: - Revert all edits to docs/source/models/visual-generation.md (separate doc refactor planned, out of scope here). - Drop the "(renamed from `output_format`)" phrasing on the serve example README; the diff and git log are the migration history. Tensor-format dispatch simplification: - Remove `TENSOR_FORMAT_EXTENSIONS` and `TENSOR_FORMAT_MEDIA_TYPES` module-level dicts in `tensorrt_llm/media/tensor_payload.py`. Both are 2-entry single-use lookups; `f".{fmt}"` and a literal `"application/octet-stream"` are clearer at the four call sites in openai_server.py and openai_video_routes.py. - Inline `_TENSOR_SUFFIX_TO_FORMAT` in `tensorrt_llm/visual_gen/output.py::_infer_format_from_path` as a one-line `_suffix_format` helper that recognizes the two known tensor suffixes directly. - Trim the "where the format is handled" sentence from `VisualGenOutput.save`'s `path:` docstring — the internal routing between `tensorrt_llm.media.encoding` and `tensorrt_llm.media.tensor_payload` is an implementation detail callers don't need. Verified on GB200: full visual_gen unit subsystem 359 passed / 0 failed in 60.33s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
May 29, 2026
… use stock ValueError Per self-review feedback on PR NVIDIA#14733: no custom exception layer on top of Python's standard error types. Validation failures now raise plain ``ValueError`` with the same multi-line message; the structured ``reason`` / ``param`` / ``details`` fields are removed. Core changes - Remove ``VisualGenValidationError`` class and the ``Literal`` / ``field`` imports it required. - Replace ``DiffusionResponse.error_reason`` / ``error_param`` / ``error_details`` with a single ``is_validation_error: bool`` discriminator. Same on ``VisualGenOutput``. The coordinator-side awaiter uses it to re-raise as ``ValueError`` (-> HTTP 400) vs ``RuntimeError`` (-> HTTP 500); routes catch the standard types directly and no longer import the custom class. - ``validate_visual_gen_params`` raises ``ValueError`` with the multi-line per-violation message. The per-category accumulator lists feeding into ``details`` are gone; the flat ``messages`` list builds the same human-readable content. Fix the "silently ignored" wording on the universal-field branch since that branch does raise. - READY signal now propagates ``pipeline_name`` so coordinator-side pre-validation uses the actual loaded pipeline's class name (was always "VisualGen"). Closes the worker/coordinator parity gap that CodeRabbit #10 flagged. Cascade (all driven by the drop) - Remove ``VisualGen.validate_request_params`` method. Async video route calls module-level ``validate_visual_gen_params`` directly with the propagated executor metadata. - ``VisualGenResult._resolved_value`` dispatches on ``out.is_validation_error`` (ValueError vs RuntimeError). - Sync image, sync video, and async video routes drop their ``except VisualGenValidationError`` arm; the existing ``except ValueError`` arm handles validation failures the same way (HTTP 400 + message). Tests - ``TestVisualGenValidationErrorStructured`` -> ``TestValidateVisualGenParamsMessages``: assert ``ValueError`` with expected substrings instead of structured fields. - ``TestValidationErrorTransport`` switches to asserting ``is_validation_error``. - ``TestRouteVisualGenValidationError`` -> ``TestRouteEngineValidationError``: sync routes inject a stock ``ValueError`` on the mock; async route triggers a real validator rejection by sending an unknown extra_params key against the mock's executor metadata. - ``MockVisualGen`` gains an ``executor`` ``SimpleNamespace`` with ``pipeline_name`` / ``default_generation_params`` / ``extra_param_specs`` so the async pre-flight call works end-to-end. Verified on GB200: full visual_gen unit subsystem 357 passed / 0 failed in 60.67s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
May 29, 2026
…d_strength to extra_params, clamp seed Third round of self-review feedback on PR NVIDIA#14733. Three code-fix threads + one CodeRabbit follow-up; conflict-resolution merge done in this round too. Pipeline forward() signature reorder - Moved `seed: int` to the second positional argument (right after `prompt`, or after `image` for `WanImageToVideoPipeline`) across flux / flux2 / wan / wan_i2v / ltx2 / ltx2_two_stages, and dropped the bare `*,` keyword-only marker. Callers (`infer()` and `_run_warmup`) already used kwargs only, so this is a mechanical internal-API change. - Aligned `Cosmos3OmniMoTPipeline.forward` with the same convention — it still carried `seed: int = 42` as a stale default. image_cond_strength moved to per-pipeline extra_params - Cross-framework check confirmed that diffusers, vllm-omni, and SGLang Diffusion all treat conditioning-strength knobs as pipeline-specific (diffusers exposes them only on `LTXConditionPipeline` / SD img2img / SVD; vllm-omni and SGLang route them through generic `diffusers_kwargs` bags). None treats `image_cond_strength` as a first-class request field. - Dropped `image_cond_strength` from `VisualGenParams`, from `VideoGenerationRequest`, from the `visual_gen_utils` translation, from `_GENERATION_CONFIG_FIELDS`, and from the Wan-defaults carve-out (which let `get_wan_default_params` lose the `include_i2v` argument entirely). - Added the field to `LTX2Pipeline.extra_param_specs` (`default=1.0`); both `LTX2Pipeline.infer()` and `LTX2TwoStagesPipeline.infer()` now read it from `req.params.extra_params["image_cond_strength"]`. Wan pipelines reject the key via the existing unknown-`extra_params` path. - Updated the LTX-2 example script so `--image_cond_strength` flows into `extra_params` instead of the dropped top-level field, and trimmed the serve README's per-request control list. Seed clamp + N-image RNG semantics doc - Introduced `MAX_UINT32_SEED = 2**32 - 1` and applied `ge=0, le=MAX_UINT32_SEED` on `VisualGenParams.seed` and on the three openai_protocol video / image / edit request schemas. - Coordinator-rank random seed materialization moved from `secrets.randbits(63)` to `secrets.randbits(32)` so engine-drawn seeds stay inside the OpenAI DALL-E range that vllm-omni adopts. - Documented the chosen `num_images_per_prompt > 1` semantics in the `VisualGenParams.seed` description: diffusers/vllm-omni style (single `torch.Generator(seed=s)` driving N latents from one RNG stream), not SGLang's `[s, s+1, …]` per-image expansion. - Three new unit tests pin the range bounds. Conflict resolution - Rebased the three serve-parity commits onto origin/main, resolving conflicts in `openai_video_routes.py` against the `build_visual_gen_timing_headers` introduction from NVIDIA#14176. Kept the upstream cleanup that uses local `generation` / `denoise` variables. Tests - `test_visual_gen_params.py`: three obsolete top-level image_cond_strength tests replaced with two extra_params variants; three new tests for seed bounds. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
…nly; tighten params invariant Fourth round of self-review feedback on PR NVIDIA#14733. Three threads addressed in this commit; the parallel discussion on the ``is_validation_error`` discriminator (threads on ``DiffusionResponse.is_validation_error`` and ``VisualGenOutput.is_validation_error``) is kept open pending a separate decision on the attribute's name vs existence. Seed clamp moved off the Python API - ``VisualGenParams.seed`` no longer carries ``ge=0, le=MAX_UINT32_SEED``. The Python API accepts the full int64 range that ``torch.Generator.manual_seed`` supports. - The UINT32 clamp now lives only on the three OpenAI request schemas in ``openai_protocol.py`` (``ImageGenerationRequest``, ``VideoGenerationRequest``, ``ImageEditRequest``) — the HTTP boundary is where OpenAI DALL-E compatibility matters, matching vllm-omni's ``api_server.py``. - ``MAX_UINT32_SEED`` moved into ``openai_protocol.py`` where it is consumed; ``visual_gen.params`` no longer exports it. - Engine fallback reverted from ``secrets.randbits(32)`` to ``secrets.randbits(63)`` — the engine's own seeds don't owe the serve-side range. - Three seed-bounds unit tests moved off ``VisualGenParams`` onto ``ImageGenerationRequest`` / ``VideoGenerationRequest`` to match where the constraint now lives. The ``VisualGenParams`` side keeps one test that exercises the wider int64 acceptance. ``req.params`` invariant tightened - ``VisualGen.generate_async`` now materializes a fresh ``VisualGenParams`` when the caller passes ``params=None``, so ``DiffusionRequest.params`` is always a concrete object by the time the executor sees it. - Dropped the ``if req.params is None: ...`` guards from ``DiffusionExecutor._resolve_seed`` and ``._merge_defaults``; ``None`` at those stages is now a bug, not an expected fallback. - Updated three tests that previously exercised the defensive ``params=None`` path to use ``params=VisualGenParams()`` (matching the new contract) and added a positive test that the enqueue site materializes the default. Comment trim on ``DiffusionResponse.is_validation_error`` - Dropped the "→ HTTP 400" / "→ HTTP 500" wording from the docstring on ``executor.py``; the comment now describes only the Python-level ``ValueError`` vs ``RuntimeError`` dispatch on the coordinator-side awaiter. The HTTP mapping is a serve concern and does not belong on the internal IPC dataclass. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
…sor-format dispatch, drop 'renamed from' wording Address self-review notes from PR NVIDIA#14733: keep the diff focused, drop over-engineered single-use dicts, and remove historical-narration wording from a public README. Doc and wording cleanup: - Revert all edits to docs/source/models/visual-generation.md (separate doc refactor planned, out of scope here). - Drop the "(renamed from `output_format`)" phrasing on the serve example README; the diff and git log are the migration history. Tensor-format dispatch simplification: - Remove `TENSOR_FORMAT_EXTENSIONS` and `TENSOR_FORMAT_MEDIA_TYPES` module-level dicts in `tensorrt_llm/media/tensor_payload.py`. Both are 2-entry single-use lookups; `f".{fmt}"` and a literal `"application/octet-stream"` are clearer at the four call sites in openai_server.py and openai_video_routes.py. - Inline `_TENSOR_SUFFIX_TO_FORMAT` in `tensorrt_llm/visual_gen/output.py::_infer_format_from_path` as a one-line `_suffix_format` helper that recognizes the two known tensor suffixes directly. - Trim the "where the format is handled" sentence from `VisualGenOutput.save`'s `path:` docstring — the internal routing between `tensorrt_llm.media.encoding` and `tensorrt_llm.media.tensor_payload` is an implementation detail callers don't need. Verified on GB200: full visual_gen unit subsystem 359 passed / 0 failed in 60.33s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
… use stock ValueError Per self-review feedback on PR NVIDIA#14733: no custom exception layer on top of Python's standard error types. Validation failures now raise plain ``ValueError`` with the same multi-line message; the structured ``reason`` / ``param`` / ``details`` fields are removed. Core changes - Remove ``VisualGenValidationError`` class and the ``Literal`` / ``field`` imports it required. - Replace ``DiffusionResponse.error_reason`` / ``error_param`` / ``error_details`` with a single ``is_validation_error: bool`` discriminator. Same on ``VisualGenOutput``. The coordinator-side awaiter uses it to re-raise as ``ValueError`` (-> HTTP 400) vs ``RuntimeError`` (-> HTTP 500); routes catch the standard types directly and no longer import the custom class. - ``validate_visual_gen_params`` raises ``ValueError`` with the multi-line per-violation message. The per-category accumulator lists feeding into ``details`` are gone; the flat ``messages`` list builds the same human-readable content. Fix the "silently ignored" wording on the universal-field branch since that branch does raise. - READY signal now propagates ``pipeline_name`` so coordinator-side pre-validation uses the actual loaded pipeline's class name (was always "VisualGen"). Closes the worker/coordinator parity gap that CodeRabbit #10 flagged. Cascade (all driven by the drop) - Remove ``VisualGen.validate_request_params`` method. Async video route calls module-level ``validate_visual_gen_params`` directly with the propagated executor metadata. - ``VisualGenResult._resolved_value`` dispatches on ``out.is_validation_error`` (ValueError vs RuntimeError). - Sync image, sync video, and async video routes drop their ``except VisualGenValidationError`` arm; the existing ``except ValueError`` arm handles validation failures the same way (HTTP 400 + message). Tests - ``TestVisualGenValidationErrorStructured`` -> ``TestValidateVisualGenParamsMessages``: assert ``ValueError`` with expected substrings instead of structured fields. - ``TestValidationErrorTransport`` switches to asserting ``is_validation_error``. - ``TestRouteVisualGenValidationError`` -> ``TestRouteEngineValidationError``: sync routes inject a stock ``ValueError`` on the mock; async route triggers a real validator rejection by sending an unknown extra_params key against the mock's executor metadata. - ``MockVisualGen`` gains an ``executor`` ``SimpleNamespace`` with ``pipeline_name`` / ``default_generation_params`` / ``extra_param_specs`` so the async pre-flight call works end-to-end. Verified on GB200: full visual_gen unit subsystem 357 passed / 0 failed in 60.67s. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
…d_strength to extra_params, clamp seed Third round of self-review feedback on PR NVIDIA#14733. Three code-fix threads + one CodeRabbit follow-up; conflict-resolution merge done in this round too. Pipeline forward() signature reorder - Moved `seed: int` to the second positional argument (right after `prompt`, or after `image` for `WanImageToVideoPipeline`) across flux / flux2 / wan / wan_i2v / ltx2 / ltx2_two_stages, and dropped the bare `*,` keyword-only marker. Callers (`infer()` and `_run_warmup`) already used kwargs only, so this is a mechanical internal-API change. - Aligned `Cosmos3OmniMoTPipeline.forward` with the same convention — it still carried `seed: int = 42` as a stale default. image_cond_strength moved to per-pipeline extra_params - Cross-framework check confirmed that diffusers, vllm-omni, and SGLang Diffusion all treat conditioning-strength knobs as pipeline-specific (diffusers exposes them only on `LTXConditionPipeline` / SD img2img / SVD; vllm-omni and SGLang route them through generic `diffusers_kwargs` bags). None treats `image_cond_strength` as a first-class request field. - Dropped `image_cond_strength` from `VisualGenParams`, from `VideoGenerationRequest`, from the `visual_gen_utils` translation, from `_GENERATION_CONFIG_FIELDS`, and from the Wan-defaults carve-out (which let `get_wan_default_params` lose the `include_i2v` argument entirely). - Added the field to `LTX2Pipeline.extra_param_specs` (`default=1.0`); both `LTX2Pipeline.infer()` and `LTX2TwoStagesPipeline.infer()` now read it from `req.params.extra_params["image_cond_strength"]`. Wan pipelines reject the key via the existing unknown-`extra_params` path. - Updated the LTX-2 example script so `--image_cond_strength` flows into `extra_params` instead of the dropped top-level field, and trimmed the serve README's per-request control list. Seed clamp + N-image RNG semantics doc - Introduced `MAX_UINT32_SEED = 2**32 - 1` and applied `ge=0, le=MAX_UINT32_SEED` on `VisualGenParams.seed` and on the three openai_protocol video / image / edit request schemas. - Coordinator-rank random seed materialization moved from `secrets.randbits(63)` to `secrets.randbits(32)` so engine-drawn seeds stay inside the OpenAI DALL-E range that vllm-omni adopts. - Documented the chosen `num_images_per_prompt > 1` semantics in the `VisualGenParams.seed` description: diffusers/vllm-omni style (single `torch.Generator(seed=s)` driving N latents from one RNG stream), not SGLang's `[s, s+1, …]` per-image expansion. - Three new unit tests pin the range bounds. Conflict resolution - Rebased the three serve-parity commits onto origin/main, resolving conflicts in `openai_video_routes.py` against the `build_visual_gen_timing_headers` introduction from NVIDIA#14176. Kept the upstream cleanup that uses local `generation` / `denoise` variables. Tests - `test_visual_gen_params.py`: three obsolete top-level image_cond_strength tests replaced with two extra_params variants; three new tests for seed bounds. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
…nly; tighten params invariant Fourth round of self-review feedback on PR NVIDIA#14733. Three threads addressed in this commit; the parallel discussion on the ``is_validation_error`` discriminator (threads on ``DiffusionResponse.is_validation_error`` and ``VisualGenOutput.is_validation_error``) is kept open pending a separate decision on the attribute's name vs existence. Seed clamp moved off the Python API - ``VisualGenParams.seed`` no longer carries ``ge=0, le=MAX_UINT32_SEED``. The Python API accepts the full int64 range that ``torch.Generator.manual_seed`` supports. - The UINT32 clamp now lives only on the three OpenAI request schemas in ``openai_protocol.py`` (``ImageGenerationRequest``, ``VideoGenerationRequest``, ``ImageEditRequest``) — the HTTP boundary is where OpenAI DALL-E compatibility matters, matching vllm-omni's ``api_server.py``. - ``MAX_UINT32_SEED`` moved into ``openai_protocol.py`` where it is consumed; ``visual_gen.params`` no longer exports it. - Engine fallback reverted from ``secrets.randbits(32)`` to ``secrets.randbits(63)`` — the engine's own seeds don't owe the serve-side range. - Three seed-bounds unit tests moved off ``VisualGenParams`` onto ``ImageGenerationRequest`` / ``VideoGenerationRequest`` to match where the constraint now lives. The ``VisualGenParams`` side keeps one test that exercises the wider int64 acceptance. ``req.params`` invariant tightened - ``VisualGen.generate_async`` now materializes a fresh ``VisualGenParams`` when the caller passes ``params=None``, so ``DiffusionRequest.params`` is always a concrete object by the time the executor sees it. - Dropped the ``if req.params is None: ...`` guards from ``DiffusionExecutor._resolve_seed`` and ``._merge_defaults``; ``None`` at those stages is now a bug, not an expected fallback. - Updated three tests that previously exercised the defensive ``params=None`` path to use ``params=VisualGenParams()`` (matching the new contract) and added a positive test that the enqueue site materializes the default. Comment trim on ``DiffusionResponse.is_validation_error`` - Dropped the "→ HTTP 400" / "→ HTTP 500" wording from the docstring on ``executor.py``; the comment now describes only the Python-level ``ValueError`` vs ``RuntimeError`` dispatch on the coordinator-side awaiter. The HTTP mapping is a serve concern and does not belong on the internal IPC dataclass. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 1, 2026
…nator; drop is_validation_error Fifth round of self-review feedback on PR NVIDIA#14733. Addresses the two parallel threads that were held open from round 4 (on ``DiffusionResponse.is_validation_error`` and ``VisualGenOutput.is_validation_error``) by removing the discriminator entirely and moving validation to the caller's process. Validation now lives on the coordinator side - ``VisualGen.generate_async`` calls ``validate_visual_gen_params`` against the executor's READY-signal-cached ``{pipeline_name, default_generation_params, extra_param_specs}`` before the request crosses the IPC boundary. Raising in the caller's process means ``ValueError`` reaches the user as a natural Python exception; no cross-process discriminator needed. - The worker no longer validates request params. ``DiffusionExecutor`` loses ``_validate_request`` and its invocation in ``_merge_defaults``. The async video route still pre-flights validation synchronously so malformed requests become HTTP 400 before the job is queued (not after the background task fails). is_validation_error attribute removed - Dropped from ``DiffusionResponse`` (worker → coordinator IPC type). - Dropped from ``VisualGenOutput`` (public per-request output). - Dropped propagation in ``to_visual_gen_output`` / ``split_visual_gen_output``. - ``VisualGenResult._resolved_value`` uniformly re-raises engine failures as ``RuntimeError``; any ``ValueError`` the user catches comes synchronously from ``generate_async`` validation, matching the ``tensorrt_llm.LLM`` convention and how vllm-omni / SGLang's multimodal_gen schemas behave (validation at API entry, runtime failures encoded on the response). validate_visual_gen_params moved to its natural home - Relocated the function and its helpers (``_TYPE_MAP``, ``_GENERATION_CONFIG_FIELDS``) from ``tensorrt_llm/_torch/visual_gen/executor.py`` (the internal worker layer) into ``tensorrt_llm/visual_gen/params.py`` (alongside ``VisualGenParams``). The validator no longer has any worker-side caller, so its previous location in ``_torch`` was misleading. - Updated imports in ``VisualGen.generate_async`` and in the async video route. The function continues to duck-type ``ExtraParamSchema`` via ``spec.type`` / ``spec.range`` so no import from the internal layer is needed. Tests - Removed ``TestValidationErrorTransport`` (the cross-process discriminator no longer exists). Replaced with ``TestEngineFailureTransport``, which exercises the engine-failure → ``RuntimeError``-flavored ``error_msg`` path. - ``TestRequestValidation`` / ``TestValidateVisualGenParamsMessages`` now call ``validate_visual_gen_params`` directly instead of routing through the removed ``DiffusionExecutor._validate_request``. - Dropped the obsolete worker-side process_request validation-error test. The route-level synchronous validation tests are unchanged — ``MockVisualGen.generate_async`` already raises the same ``ValueError`` the real validator produces. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
commented
Jun 2, 2026
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 2, 2026
…on default params; dedup format constants Sixth round of self-review feedback on PR NVIDIA#14733. Five Cat-1 fixes; the two Cat-3 design questions (batched-tensor save layout, raw-bytes transport for tensor formats) are answered on the threads with no code change pending further discussion. Skip validation when the caller passes no params - ``VisualGen.generate_async`` now branches on whether ``params`` is supplied: with user params it validates + snapshots; with ``None`` it builds a fresh ``VisualGenParams`` from ``self.default_params`` (which already materializes the pipeline's declared defaults + extra-param defaults from the executor's READY-signal cache) and skips validation, since there's nothing user-supplied to flag. - Previous behavior built an empty ``VisualGenParams()`` and let the worker fill defaults; now the coordinator owns the full default materialization on the no-params path, matching the symmetry the ``default_params`` property already provides. Tensor-format constants deduplicated - ``visual_gen/output.py:_infer_format_from_path`` now derives its recognized suffixes from ``tensorrt_llm.media.tensor_payload.TENSOR_FORMATS`` instead of an inline ``(".safetensors", ".pt")`` tuple. ``TENSOR_FORMATS`` is the single source of truth. OpenAI server format helpers dropped - Removed ``_IMAGE_FORMAT_TO_PIL`` — Pillow's format name is just the upper-case form of our request token, so ``pil_format = request.format.upper()`` replaces the dict. - Removed ``_IMAGE_FORMAT_TO_EXT`` — the on-disk extension matches the request token directly. ``.jpeg`` is interchangeable with ``.jpg`` for Pillow, the OS, and the OpenAI API. ``ext = f".{request.format}"`` replaces the dict. - The ``Literal["png", "webp", "jpeg", "safetensors", "pt"]`` on ``ImageGenerationRequest.format`` remains the supported-format gate. Model-mismatch warning rewording - Dropped the "trtllm-serve is single-model per process and the field is informational only" phrasing from the request-vs-loaded-model warning. The new message — "the model field is logged but ignored" — conveys the observable behavior without the architectural detail. Verified: 251 / 251 focused VisualGen unit tests pass on GB200. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
added a commit
to zhenhuaw-me/TensorRT-LLM
that referenced
this pull request
Jun 2, 2026
…ng; raise on batched-save-with-single-path Seventh round of self-review feedback on PR NVIDIA#14733. Two changes folded into one commit: drop the ``pipeline_name`` plumbing, and tighten the implicit-slice behavior of ``save_image`` / ``save_video`` / tensor save when a batched tensor is paired with a single path. Drop pipeline_name plumbing - Removed ``pipeline_name`` parameter from ``validate_visual_gen_params(...)``. - Removed ``pipeline_name`` field from the worker's READY signal payload (``DiffusionExecutor._load_pipeline``). - Removed ``self.pipeline_name`` attribute on ``DiffusionRemoteClient`` and its READY-handler population in ``_wait_ready``. - Removed ``pipeline_name=...`` keyword from the two validator call sites (``VisualGen.generate_async`` and the async video route). - Error message: ``"Parameter validation failed for {pipeline_name}:\n - ..."`` → ``"Parameter validation failed:\n - ..."``. The per-bullet suffixes drop the pipeline name too, in favor of ``"the loaded pipeline"`` where useful. - Trade-off: validation messages lose the per-pipeline namespace. In practice the label was the pipeline class name (LTX2Pipeline, WanImageToVideoPipeline, …), which leaked an implementation detail without helping the user; trtllm-serve also loads one pipeline per process, so the label was a per-process constant. Single path + batched tensor now raises ValueError - ``save_image`` and ``save_video`` in ``tensorrt_llm/media/encoding.py`` used to silently slice ``image[0]`` / ``video[0]`` when handed a batched tensor with a single path, dropping the remaining items. They now raise ``ValueError`` when ``batch_size > 1`` and accept the ``batch_size == 1`` case (unambiguous unwrap). - ``VisualGenOutput._save_tensor_payload`` mirrors the rule on the tensor-format save path. - Matches the convention in vllm-omni's codec layer (``_encode_video_bytes`` raises on batched 5-D tensors) and diffusers' ``BaseOutput`` (no ``.save()``; callers iterate explicitly). - Callers that want one file out of a batch can do ``output.save_bytes(format=..., batch_index=i)`` and write the slice themselves, or pass a list of N paths to write N files. OpenAI video routes follow the new contract - Sync and async video tensor-format paths in ``openai_video_routes.py`` now build ``[{video_id}_{i}.{fmt}` for i in range(batch_size)]`` and call ``output.save(tensor_paths, ...)``, matching the encoder-format path. The first saved file is returned as the route's primary download (OpenAI sync video API does not define a multi-file response yet — TRTLLM-11579); the async route records all paths on ``output_paths``. - ``get_video_content``'s recovery probe now tries both the bare ``{vid}{ext}`` and the batch-indexed ``{vid}_0{ext}`` filename shapes, matching the convention ``delete_video`` already uses (closes the CodeRabbit follow-up deferred in round 3). Tests - ``test_tensor_payload.TestSingleSavePath``: ``test_batched_image_writes_first_item_only`` → ``test_batched_image_single_path_raises``; ``test_batched_video_writes_first_item_and_audio`` → ``test_batched_video_single_path_raises``. The unbatched and ``batch == 1`` save paths still pass through unchanged. - ``test_visual_gen_params``: dropped ``pipeline_name=...`` kwarg from the test helpers; updated two message-content assertions to match the label-free error string. - ``MockVisualGen.executor`` SimpleNamespace no longer carries the ``pipeline_name`` attribute; the route preflight only reads ``default_generation_params`` / ``extra_param_specs``. - ``_make_validation_error`` helper builds the new label-free shape. Verified: 292 / 292 focused VisualGen unit tests pass on GB200. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
Collaborator
|
PR_Github #52695 [ run ] completed with state |
Collaborator
|
PR_Github #52741 [ run ] completed with state
|
Member
Author
|
/bot run |
Member
Author
|
/bot kill |
Collaborator
|
PR_Github #52899 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #52900 [ kill ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #52899 [ run ] completed with state |
Collaborator
|
PR_Github #52900 [ kill ] completed with state |
…GenParams
Aligns the trtllm-serve visual-generation routes (image / sync video /
async video / image-edit) with the public VisualGenParams Python API,
and tightens the Python API surface around parameter validation, error
propagation, output saving, and seed handling.
VisualGenParams (under tensorrt_llm/visual_gen/params.py) is the single
source of truth for per-request generation parameters. Universal fields
default to None ("use the loaded pipeline's default"); model-specific
knobs live in extra_params; validate_visual_gen_params runs synchronously
at VisualGen.generate_async entry against the loaded pipeline's
default_generation_params and extra_param_specs.
The OpenAI request schemas (ImageGenerationRequest, VideoGenerationRequest)
map 1:1 to VisualGenParams. Unknown top-level fields return HTTP 422 via
extra="forbid"; per-request denoise controls (seed / num_inference_steps /
guidance_scale / max_sequence_length / negative_prompt); model-specific
knobs flow through extra_params. The /v1/images/edits route stays
registered but returns HTTP 501 (no in-tree pipeline implements editing)
and no longer parses a typed body.
format is a Literal covering image/video encoders plus tensor formats
"safetensors" / "pt". The tensor formats carry every populated modality
(image / video / audio) plus scalar metadata in one payload. For
safetensors, scalar metadata is stored both as 0-d tensors (survives
safetensors.torch.load) and in the file header (survives safe_open
metadata access).
Pipeline forward signatures (flux / flux2 / wan / wan_i2v / ltx2 /
ltx2_two_stages / cosmos3) take seed: int as the 2nd positional parameter.
Engine fallback materializes a fresh seed via secrets.randbits(63) on the
coordinator rank before torch.distributed.broadcast_object_list, so every
rank sees the same resolved seed and determinism survives multi-rank runs.
VisualGenOutput.save(path) writes one logical output: unbatched and
batch == 1 tensors save as-is; batch > 1 paired with a single path raises
ValueError (matching diffusers' caller-iterates contract). Callers who
want N files pass a list of N paths.
Round 2 of chang-l review feedback folded in:
- DiffusionRemoteClient moved from tensorrt_llm/visual_gen/visual_gen.py
to tensorrt_llm/_torch/visual_gen/executor.py, alongside its IPC
counterpart DiffusionExecutor. Class was already not in __all__ and
not re-exported from tensorrt_llm.visual_gen; the move makes the
internal status structural rather than merely conventional.
- VisualGenOutput.save_bytes renamed to _save_bytes — the public output
API now exposes only save(); in-memory bytes serialization is reserved
for the b64_json transport in openai_server.
- safetensors metadata hybrid: scalars written both as 0-d tensors and
in the file header, so safetensors.torch.load(bytes) returns them
alongside the media tensors instead of dropping them.
- ImageEditRequest schema dropped (route still returns 501). The schema
carried legacy fields (guidance_rescale, validate_size field-validator,
required n) inconsistent with the 1:1 VisualGenParams alignment; a
fresh schema lands when an edit-capable pipeline lands.
- ValueError → HTTP 400 mapping in the image + video sync routes is now
scoped to the parse + generate region only. Server-side ValueErrors
from infer_batch_size / output._save_bytes / output.save fall through
to the outer catch-all and surface as HTTP 500 InternalServerError.
- visual_gen_utils.parse_visual_gen_params raises ValueError when
request.seconds is set but params.frame_rate cannot be resolved, so the
duration is no longer silently dropped.
Tests
- tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py:
added test_image_route_serialization_value_error_returns_500,
test_image_edit_no_body_returns_not_implemented,
test_seconds_without_frame_rate_returns_400.
- tests/unittest/_torch/visual_gen/test_tensor_payload.py:
safetensors-branch metadata asserts now exercise the 0-d tensor path
in addition to the file header.
- tests/unittest/_torch/visual_gen/multi_gpu/test_visual_gen_multinode.py:
patch targets repointed at the new module home for DiffusionRemoteClient.
- tests/unittest/media/test_encoding.py: replaced the obsolete
test_save_image_strips_batch_dim with rejects-B>1 + accepts-B==1
variants matching save_image's tightened contract.
Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
Member
Author
|
/bot run |
Collaborator
|
PR_Github #53025 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #53025 [ run ] completed with state
|
Member
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #53051 [ run ] triggered by Bot. Commit: |
zhenhuaw-me
enabled auto-merge (squash)
June 9, 2026 14:19
Signed-off-by: Zhenhua Wang <4936589+zhenhuaw-me@users.noreply.github.com>
Member
Author
|
/bot kill |
Collaborator
|
PR_Github #53205 [ kill ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #53051 [ run ] completed with state |
Collaborator
|
PR_Github #53205 [ kill ] completed with state |
Member
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #53233 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #53233 [ run ] completed with state
|
Member
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #53319 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #53319 [ 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
4 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.
Description
Aligns the
trtllm-servevisual-generation routes (image / sync video / async video / image-edit) with the publicVisualGenParamsPython API, and tightens the Python API surface around parameter validation, error propagation, output saving, and seed handling.VisualGenParams(undertensorrt_llm/visual_gen/params.py) is now the single source of truth for per-request generation parameters. Universal fields default toNone("use the loaded pipeline's default"); model-specific knobs live inextra_params(LTX-2 exposesimage_cond_strength/output_type/guidance_rescale/stg_*/modality_scale/rescale_scale/guidance_skip_step/enhance_prompt; Wan 2.2 A14B exposesguidance_scale_2/boundary_ratio; Wan I2V exposeslast_image).validate_visual_gen_paramsruns synchronously atVisualGen.generate_asyncentry — checkingVisualGenParamsagainst the loaded pipeline'sdefault_generation_paramsandextra_param_specs(cached on the executor from the worker's READY signal). Bad-request failures raiseValueErrorin the caller's process; engine-side failures raiseRuntimeErrorwhen the result is awaited. Matches thetensorrt_llm.LLMconvention and how vllm-omni / SGLang multimodal_gen behave.ImageGenerationRequest,VideoGenerationRequest) map 1:1 toVisualGenParams. Unknown top-level fields return HTTP 422 viaextra="forbid"; per-request denoise controls (seed/num_inference_steps/guidance_scale/max_sequence_length/negative_prompt); model-specific knobs flow throughextra_paramsand are validated against the loaded pipeline's specs. The/v1/images/editsroute stays registered but returns HTTP 501 (no in-tree pipeline implements editing) and no longer parses a typed request body.formatis aLiteralcovering image/video encoders plus tensor formats"safetensors"/"pt". The tensor formats carry every populated modality (image / video / audio) plus scalar metadata in one payload (tensorrt_llm/media/tensor_payload.py) so consumers get a single file per logical output. Routes catchValueErrorfrom validation andpipeline.infer()and render the LLM error envelope at HTTP 400;RuntimeErrorbecomes HTTP 500.forwardsignatures (flux / flux2 / wan / wan_i2v / ltx2 / ltx2_two_stages / cosmos3) takeseed: intas the 2nd positional parameter (afterprompt, or afterimageforWanImageToVideoPipeline). Engine fallback materializes a fresh seed viasecrets.randbits(63)on the coordinator rank beforetorch.distributed.broadcast_object_list, so every rank sees the same resolved seed and determinism survives multi-rank runs.VisualGenOutput.save(path)writes one logical output: unbatched andbatch == 1tensors save as-is;batch > 1paired with a single path raisesValueError(matching diffusers' caller-iterates contract and vllm-omni's codec-layer rule). Callers who want N files pass a list of N paths; callers who want one item out of a batch useoutput.save_bytes(format=..., batch_index=i). The legacy implicit-slice-to-first behavior insave_image/save_video/ tensor save is removed.{video_id}_{i}.{fmt};get_video_content's recovery probe accepts both the bare and batch-indexed filename shapes.One open thread on the PR is deferred to a follow-up (TRTLLM-13143): the media-transport contract for
response_format(raw bytes vs. b64) when paired with tensor formats.Test Coverage
tests/unittest/_torch/visual_gen/cover the public API (params validation, output transport, save dispatch, default merging), the route-level rendering (image / sync video / async video / image-edit / get-content / delete), schema 1:1 mapping withVisualGenParams, and tensor payload roundtrip (safetensorsandpt, batched and unbatched, with metadata overrides).<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
safetensors,pt) for image and video generation.formatparameter for specifying output encoding across generation requests.Bug Fixes
Documentation
<!-- review_stack_entry_start -->
[!Review Change Stack](https://app.coderabbit.ai/change-stack/NVIDIA/TensorRT-LLM/pull/14733?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->