[TRTLLM-13143][feat] BREAKING: VisualGen serving API: response_format support path and modify async job status - #17490
Conversation
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe visual generation API renames the synchronous video route to ChangesVisual generation API
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds path-based video responses and route compatibility; the remaining concern is limited to strengthening exact response-envelope assertions in tests, with no actionable merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tensorrt_llm/serve/openai_protocol.py (1)
1576-1576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new transport fields in the generated schema.
Add
Field(description=...)to the changedresponse_formatfields and toImageObject.path. This keeps the OpenAPI schema clear for clients that discover the new transport through the API.Proposed change
- response_format: Literal["url", "b64_json", "path"] = "url" + response_format: Literal["url", "b64_json", "path"] = Field( + default="url", + description="Response transport: URL, inline base64 JSON, or server-side path.", + ) ... - path: Optional[str] = None + path: Optional[str] = Field( + default=None, + description="Server-side path to the generated image for co-located clients.", + ) ... - response_format: Literal["file", "path"] = "file" + response_format: Literal["file", "path"] = Field( + default="file", + description="Response transport: file download or server-side path JSON.", + )As per coding guidelines, “For Pydantic fields, use
Field(description=...).”Also applies to: 1645-1645, 1676-1676
🤖 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_protocol.py` at line 1576, Add Field(description=...) metadata to the response_format fields at the referenced locations and to ImageObject.path, preserving their existing types and defaults. Use concise descriptions that document each supported transport value and the path field’s purpose so the generated OpenAPI schema clearly exposes the new transport options.Source: Coding guidelines
tensorrt_llm/serve/openai_video_routes.py (1)
67-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a precise path-compatible type for
paths.The helper receives both
list[Path]andlist[str]. Annotate it asSequence[str | Path]and importSequencefromcollections.abc.🤖 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_video_routes.py` around lines 67 - 80, Update _path_json_video_response to annotate paths as Sequence[str | Path], and import Sequence from collections.abc. Preserve the existing string conversion and response construction.Source: Coding guidelines
🤖 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 `@tests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py`:
- Line 250: Update the request URL used by both synchronous tests, test_t2v_sync
and test_ti2v_sync, to call /v1/videos/sync instead of /v1/videos/generations.
Keep the existing request payloads and assertions unchanged.
---
Nitpick comments:
In `@tensorrt_llm/serve/openai_protocol.py`:
- Line 1576: Add Field(description=...) metadata to the response_format fields
at the referenced locations and to ImageObject.path, preserving their existing
types and defaults. Use concise descriptions that document each supported
transport value and the path field’s purpose so the generated OpenAPI schema
clearly exposes the new transport options.
In `@tensorrt_llm/serve/openai_video_routes.py`:
- Around line 67-80: Update _path_json_video_response to annotate paths as
Sequence[str | Path], and import Sequence from collections.abc. Preserve the
existing string conversion and response construction.
🪄 Autofix
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: 272622ef-9d6c-4399-981d-e72f0096369b
📒 Files selected for processing (10)
docs/source/models/visual-generation.mdexamples/visual_gen/serve/README.mdexamples/visual_gen/serve/sync_video_gen.pytensorrt_llm/serve/openai_protocol.pytensorrt_llm/serve/openai_server.pytensorrt_llm/serve/openai_video_routes.pytensorrt_llm/serve/scripts/benchmark_visual_gen.pytests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.pytests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.pytests/unittest/_torch/visual_gen/test_visual_gen_utils.py
|
PR_Github #65216 [ run ] triggered by Bot. Commit: |
8008f24 to
d88c099
Compare
|
PR_Github #65216 [ run ] completed with state
|
83f8ef1 to
2ed0835
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py (1)
1707-1752: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject removed video response formats.
Add parameterized requests with
response_format="url"andresponse_format="b64_json". Assert HTTP 422. The current tests verify acceptedfileandpathvalues, but they do not enforce removal of the old values.The PR objective restricts video
response_formattofileandpath.Also applies to: 1802-1837
🤖 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_trtllm_serve_endpoints.py` around lines 1707 - 1752, Add parameterized tests alongside test_sync_tensor_file_returns_file_with_correct_suffix and test_sync_tensor_path_returns_readable_output_path that submit response_format values "url" and "b64_json" for a supported tensor format, then assert the endpoint returns HTTP 422. Ensure the tests explicitly enforce that only the existing "file" and "path" response formats are accepted.
🤖 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 `@tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py`:
- Around line 801-809: Add a test near TestVideoGenerationSync that posts to the
removed /v1/videos/generations route and asserts a 404 response, ensuring the
old synchronous endpoint remains unavailable after the rename.
- Around line 556-573: Update test_image_generation_pt_path to assert that
obj["b64_json"] is None alongside the existing path and URL assertions, ensuring
tensor path responses leave both transport fields unset.
---
Outside diff comments:
In `@tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py`:
- Around line 1707-1752: Add parameterized tests alongside
test_sync_tensor_file_returns_file_with_correct_suffix and
test_sync_tensor_path_returns_readable_output_path that submit response_format
values "url" and "b64_json" for a supported tensor format, then assert the
endpoint returns HTTP 422. Ensure the tests explicitly enforce that only the
existing "file" and "path" response formats are accepted.
🪄 Autofix
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: 82eb42fb-4a54-4f92-b4f8-bf68b1c07baa
📒 Files selected for processing (3)
tensorrt_llm/serve/openai_protocol.pytensorrt_llm/serve/openai_video_routes.pytests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tensorrt_llm/serve/openai_protocol.py
- tensorrt_llm/serve/openai_video_routes.py
2ed0835 to
a113cc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py`:
- Around line 251-252: Add a finite ten-minute timeout to both synchronous video
POST requests in
tests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py:251-252 and
tests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py:372-373, covering the
text-to-video and image-to-video request flows respectively. Use the same
timeout value for both requests.
🪄 Autofix
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: a9127fe7-15d5-43e1-aab6-6c9a90388d7c
📒 Files selected for processing (2)
tests/unittest/_torch/visual_gen/test_trtllm_serve_e2e.pytests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py
|
PR_Github #68670 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #68732 [ run ] triggered by Bot. Commit: |
|
PR_Github #68732 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #68818 [ run ] triggered by Bot. Commit: |
|
PR_Github #68818 [ run ] completed with state |
Drop rationale clauses (measurement use-case, security reasoning, disclosure justification) from the async lifecycle, response_format=path, and release-note prose, keeping the state machine, endpoints, and flag behavior as plain description. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
…t post Move the two-stage `mark_post_start` from the end of stage 1 to the decode boundary, so `denoise` spans stage 1 plus the stage-2 upsample / LoRA bind / refinement loop and `post_denoise` is decode only, matching single-stage models. The Server-Timing `denoise` this PR emits was understated for LTX-2 two-stage (stage 1 only). Fine-grained `stage2_denoise_time()` / `decode_time()` logs read independent event pairs and are unaffected. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #68870 [ run ] triggered by Bot. Commit: |
|
PR_Github #68870 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #68974 [ run ] triggered by Bot. Commit: |
|
PR_Github #68974 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #69125 [ run ] triggered by Bot. Commit: |
|
PR_Github #69125 [ run ] completed with state |
The sync video route attaches Server-Timing to every tensor response. Its `path` sub-path is asserted by test_sync_tensor_path_returns_readable_output_path, but the `file` sub-path -- the FileResponse a measurement client actually downloads -- is only checked for its filename suffix and payload round-trip, so a regression that dropped the headers there would go unseen. Assert the headers on the `file` sub-path as a sibling test in TestVideoTensorResponse, reusing its _post_sync helper and the existing _assert_visual_gen_server_timing. This PR originally also changed openai_video_routes.py to attach those headers, which was the actual bug at the time it was opened. NVIDIA#17490 (BREAKING: response_format support for `path`) has since restructured that route and attaches them already, so only the missing coverage remains. Signed-off-by: Kanghwan Jang <861393+karljang@users.noreply.github.com>
Two conflicts, both in the serving layer against the deliberate response_format break on main (NVIDIA#17490: video API drops b64_json for "file"/"path" plus Server-Timing headers and a "postprocessing" job status): - openai_video_routes.py: the tensor-payload sync branch adopts the new conventions (timing headers, path envelope, FileResponse with headers); the dead b64 helper goes with the rest of the b64 paths. The background task keeps resolving through request_format (the route's tensor-only resolution) rather than raw request.format, and gains upstream's postprocessing status transition. - test_trtllm_serve_endpoints.py: keeps the thread-settling TestClient (pytest-threadleak guard) wired into upstream's reworked _create_server. Signed-off-by: Igor Shovkun <igshov@gmail.com>
`/v1/images/generations` and `/v1/images/edits` emitted only the engine's `generation` and `denoise` metrics, so a client could not tell how much server time was spent outside the engine — request parsing, image encoding, response serialization. Both video routes have reported `total` since NVIDIA#17490. Start the clock at the top of each handler, before request parsing, so the metric spans the same window the video routes measure. The existing `latency` variable is not reused because it starts after parsing and would under-report. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
The image edit route already writes each output image to media storage before returning a fetchable URL, but it could not return that path: `ImageEditRequest.response_format` only accepted `url` / `b64_json`, while `/v1/images/generations` and both video routes have supported `path` since NVIDIA#17490. Reuse the existing `_image_object` helper so the edit route shares one transport dispatch with the generation route, and gate `path` behind the same `TRTLLM_DISALLOW_LOCAL_MEDIA_PATH` check the other routes use. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
`/v1/images/generations` and `/v1/images/edits` emitted only the engine's `generation` and `denoise` metrics, so a client could not tell how much server time was spent outside the engine — request parsing, image encoding, response serialization. Both video routes have reported `total` since NVIDIA#17490. Start the clock at the top of each handler, before request parsing, so the metric spans the same window the video routes measure. The existing `latency` variable is not reused because it starts after parsing and would under-report. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
The image edit route already writes each output image to media storage before returning a fetchable URL, but it could not return that path: `ImageEditRequest.response_format` only accepted `url` / `b64_json`, while `/v1/images/generations` and both video routes have supported `path` since NVIDIA#17490. Reuse the existing `_image_object` helper so the edit route shares one transport dispatch with the generation route, and gate `path` behind the same `TRTLLM_DISALLOW_LOCAL_MEDIA_PATH` check the other routes use. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
`/v1/images/generations` and `/v1/images/edits` emitted only the engine's `generation` and `denoise` metrics, so a client could not tell how much server time was spent outside the engine — request parsing, image encoding, response serialization. Both video routes have reported `total` since NVIDIA#17490. Start the clock at the top of each handler, before request parsing, so the metric spans the same window the video routes measure. The existing `latency` variable is not reused because it starts after parsing and would under-report. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
The image edit route already writes each output image to media storage before returning a fetchable URL, but it could not return that path: `ImageEditRequest.response_format` only accepted `url` / `b64_json`, while `/v1/images/generations` and both video routes have supported `path` since NVIDIA#17490. Reuse the existing `_image_object` helper so the edit route shares one transport dispatch with the generation route, and gate `path` behind the same `TRTLLM_DISALLOW_LOCAL_MEDIA_PATH` check the other routes use. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
`/v1/images/generations` and `/v1/images/edits` emitted only the engine's `generation` and `denoise` metrics, so a client could not tell how much server time was spent outside the engine — request parsing, image encoding, response serialization. Both video routes have reported `total` since NVIDIA#17490. Start the clock at the top of each handler, before request parsing, so the metric spans the same window the video routes measure. The existing `latency` variable is not reused because it starts after parsing and would under-report. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
Description
Improves the VisualGen OpenAI-compatible serving API in two areas — output transports and async job observability.
Transports: add
response_format="path"to the image/video endpoints — return the server-side on-disk output path so a co-located client (shared filesystem) opens the file directly, with nourlsecond-fetch or base64 bloat. Precedent is self-hosted only (Veo-VertexgcsUri, ComfyUI filename, SGLang file-store); no SaaS API has it, since a filesystem path only means something when caller and engine share a filesystem.Observability: the async
/v1/videosjob exposes agenerating → postprocessing → completedlifecycle so a client can time generation before postprocessing, and both sync + async responses emit aServer-Timingheader with engine (generation/denoise) and end-to-end (total) metrics.Details below.
pathadded;url/b64_jsonunchanged.{url, b64_json}→{file, path}(dropb64_json;filereplaces the misnamedurlthat returned bytes). Aresponse_formatvalidator rejects the removedurl/b64_jsonvalues with an actionable 422 naming the replacement (not the genericInput should be 'file' or 'path'); callers on the old default are unaffected (oldurldefault and newfiledefault both return raw bytes). Plus a new canonical sync route/v1/videos/sync; the pre-existing/v1/videos/generationsis retained as a deprecated alias to the same sync handler (upstream already depends on it), so the route change itself is non-breaking./sync: OpenAI exposes agenerationsendpoint only for images (/v1/images/generations, synchronous); its video API/v1/videosis async by default and has no/v1/videos/generations. The old name borrowedgenerationsfrom the image endpoint for a route with no OpenAI counterpart;/v1/videos/syncnames the synchronous behavior explicitly, paired with the async/v1/videos./v1/videos/generationsstays registered as a deprecated alias so existing callers keep working.GET /v1/videos/{id}(andlist) now returns status only — the internaloutput_path/output_pathsare no longer leaked on the wire (they stay on the job for/contentresolution +delete). The path is delivered via/contentwhenresponse_format="path". Closes a pre-existing path leak./v1/videosjob now advancesqueued → generating → postprocessing → completed(the previously-unusedin_progressis split intogenerating= model inference andpostprocessing= encode the media and/or write the output file). Thegenerating → postprocessingtransition marks the end of inference, so a co-located benchmarking client pollingGET /v1/videos/{id}can time generation on its side (independent of postprocessing); the file stays downloadable via/contentoncecompleted(/contentreturns not ready / 400 whilegenerating/postprocessing). To keeppostprocessingobservable, the async encoder save is offloaded to a thread (env-gatedTRTLLM_VIDEO_ASYNC_ENCODE, matching the sync route) so the event loop stays responsive during the encode. The sync route is unchanged.GET /v1/videos/{id}/contentnow return aServer-Timingheader withgeneration,denoise, and a newtotal(ms), so a co-located dev client reads engine + end-to-end timings straight off the response instead of polling.totalis the full server time (sync: request arrival → response; async: POST arrival → job completed), stamped withperf_counterat handler entry. For async the timings ride on the job as internalexclude=Truefields bridging POST → background →/content(status endpoint stays status-only).generation/denoiseare the engine metrics (also on the in-processoutput.metrics);totalis a serve-side measurement, so it is not added toVisualGenMetrics. (Image generation keepsgeneration/denoise;totalis video-only for now.)VideoJob.output_pathsstays on the job (internal only) — excluded from every wire response, consumed bydeletefor batch cleanup and kept as the manifest the n>1 multi-output follow-up builds on. The single-video wire isoutput_path;output_pathsnever appears on the wire. (Video n>1 is unreachable over HTTP today — nonfield — so it stays length-1 for now; multi-output is thedata[]-of-objects follow-up below, not a flat paths list.)response_format="path"opt-out gate —pathreturns absolute server-side file paths (always under the media-storage directoryTRTLLM_MEDIA_STORAGE_PATH, so the disclosure is bounded) andtrtllm-servehas no auth, so it is enabled by default but can be disabled server-side withTRTLLM_DISALLOW_LOCAL_MEDIA_PATH=1(→ 400 on the image and video endpoints). Path (co-located) clients also receive the sameServer-Timingmetrics asfileclients.Usage (old → new)
Image ·
POST /v1/images/generations(endpoint unchanged,pathadded)Video sync · new canonical route
/v1/videos/sync(/v1/videos/generationskept as deprecated alias)Video async ·
POST /v1/videos(newgenerating/postprocessingstatus; newpath)Tests
161 unit tests (mock-based, no GPU):
tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py(imagepathincl. n>1data[]fan-out; videofile/pathacross sync-tensor, sync-encoder, async/content; async job status lifecyclegenerating → postprocessing → completedincl. an httpx-AsyncClienttest that observespostprocessingwhile the offloaded encode runs, plus/contentnot-ready gating;Server-Timinggeneration/denoise/totalon sync + async/content; status/list endpoints status-only — no path leak) andtest_visual_gen_utils.py. The async-completion tests run over an httpxAsyncClienton a live loop because starlette'sTestClientdoes not progress detached background tasks between requests.Follow-up (separate PR)
media_storageTTL GC to bound disk forpath/urlartifacts, which persist today with no per-request cleanup.url): keepresponse_format ∈ {file, path}; the POST returns{id, data: [VideoObject{path}]}(a path manifest, nourl/b64). The client fetches each item itself viaGET /v1/videos/{id}/content?i=<i>— extend/contentto accept?i=(default 0; bare/contentstays "first item"). Unlike image, video does not return a readyurlfield; the client builds/content?i=from the id.VideoJob.output_pathsis the manifest?i=indexes into.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)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.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.