Skip to content

[https://nvbugs/6720250][fix] Use adjusted clock for VisualGen timing - #18686

Open
chienchunhung wants to merge 2 commits into
NVIDIA:mainfrom
chienchunhung:codex/fix-visualgen-adjusted-clock
Open

[https://nvbugs/6720250][fix] Use adjusted clock for VisualGen timing#18686
chienchunhung wants to merge 2 commits into
NVIDIA:mainfrom
chienchunhung:codex/fix-visualgen-adjusted-clock

Conversation

@chienchunhung

@chienchunhung chienchunhung commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Description

PR #17491 moved request-arrival timestamps to the server's AdjustedSteadyClock, while the VisualGen completion paths added by PR #18444 still read the raw clock. This leaves the image endpoints calling an undefined symbol and the video endpoints mixing clock domains.

Use the server's adjusted clock for all image and video completion timestamps, add deterministic same-clock regression coverage for all five paths, and remove the temporary waiver from PR #18685.

Dev Engineer Review

  • Updated VisualGen image and video completion timing to use the server’s AdjustedSteadyClock.
  • Applied the same clock to image generation, image editing, synchronous video, tensor video, and asynchronous video paths.
  • Removed the unused steady-clock import.
  • Changes preserve the existing APIs and limit scope to timestamp calculation.
  • Removed the temporary NVBUG 6720250 waiver for unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py.
  • Added required license and SPDX headers.
  • No configuration changes were identified.

QA Engineer Review

  • Modified timing coverage for image generation, image editing, synchronous encoded video, synchronous tensor video, and asynchronous video.
  • Tests now use a deterministic shared adjusted clock and verify exact 5-second total durations.
  • The modified endpoint test remains referenced by tests/integration/test_lists/waives.txt only through the removed waiver entry. No new test-list entry was added.
  • The waiver removal has valid formatting and the intended bug reference.
  • Reported validation includes the full pre-commit suite and Python compilation of modified files.
  • Verdict: sufficient.

Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast --stage-list "CPU-Generic-x86-1,CPU-Generic-arm-1" --detailed-log

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71359 [ run ] triggered by Bot. Commit: 614207c Link to invocation

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5110ab16-5e89-4fd7-86b9-f3dd9fe0e319

📥 Commits

Reviewing files that changed from the base of the PR and between 3503e3f and 614207c.

📒 Files selected for processing (4)
  • tensorrt_llm/serve/openai_server.py
  • tensorrt_llm/serve/openai_video_routes.py
  • tests/integration/test_lists/waives.txt
  • tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.


Walkthrough

The server now uses its adjusted steady clock for image and video timing. Visual generation tests use fixed clock values and assert exact 5000 ms totals. The related test waiver was removed.

Changes

Visual generation timing

Layer / File(s) Summary
Server clock timing wiring
tensorrt_llm/serve/openai_server.py, tensorrt_llm/serve/openai_video_routes.py
Image, image-edit, synchronous video, tensor-response, encoder-response, and background completion timing now use _adjusted_steady_clock.
Deterministic timing validation
tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py, tests/integration/test_lists/waives.txt
Tests use fixed arrival and completion timestamps and assert exact 5000 ms totals across image and video paths. The test skip waiver was removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 3c675

Visual-generation timing now uses a consistent server clock for request arrival and completion, with deterministic coverage across the affected image and video paths. No current merge-blocking risk remains.

Suggested reviewers: bowenfu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.19% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the NVBugs issue, fix type, and primary change: using the adjusted clock for VisualGen timing.
Description check ✅ Passed The description clearly explains the clock-domain issue and the implemented solution. It does not include the template's separate Test Coverage or PR Checklist sections, but the core issue and solutio…
Full details: Description check

Explanation

The description clearly explains the clock-domain issue and the implemented solution. It does not include the template's separate Test Coverage or PR Checklist sections, but the core issue and solution are sufficiently documented, and the stated objectives provide the relevant test coverage.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@chienchunhung
chienchunhung enabled auto-merge (squash) September 4, 2026 00:14
Comment thread tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71359 [ run ] completed with state SUCCESS. Commit: 614207c
/LLM/main/L0_MergeRequest_PR pipeline #58479 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71379 [ run ] triggered by Bot. Commit: 3c67502 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #71379 [ run ] completed with state FAILURE. Commit: 3c67502
/LLM/main/L0_MergeRequest_PR pipeline #58499 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@luyiyun1021

Copy link
Copy Markdown
Collaborator

Heads-up on a second breakage from the same #17491 clock refactor this PR is cleaning up. It is outside the current diff, and right now it is neither fixed nor waived.

unittest/llmapi/test_llm.py::test_openai_completion_list_prompt_stream_reuses_stream_metadata fails on A100X-PyTorch-Post-Merge-1:

AttributeError: 'OpenAIServer' object has no attribute '_adjusted_steady_clock'
tensorrt_llm/serve/openai_server.py:2506: AttributeError

That test builds its server with object.__new__(OpenAIServer), bypassing __init__, and hand-sets only the attributes the code path needed when it was written. #17491 changed the openai_completion streaming path (generator_wrapper) from the module-level get_steady_clock_now_in_seconds() to self._adjusted_steady_clock.now(), so the stub is now missing an attribute it previously never had to set.

I built main HEAD on a B200 node and ran both before and after applying this PR. Your diff takes test_trtllm_serve_endpoints.py from 29 failed, 124 passed to 153 passed, but the llmapi test stays red — same AttributeError, only shifted to line 2509 by the SPDX header you add. These two lines make it pass, verified on that same tree with this PR applied:

 from tensorrt_llm import LLM
+from tensorrt_llm._utils import AdjustedSteadyClock
 from tensorrt_llm.bindings import executor as tllm
@@ def test_openai_completion_list_prompt_stream_reuses_stream_metadata @@
         server._input_proc_executor = None
+        server._adjusted_steady_clock = AdjustedSteadyClock()

Would you be willing to fold that in here? Same root cause as the rest of the diff, and this exact test has gone stale the same way three times now — #16841 (NVBug 6507081) added reasoning_parser = None, #17401 (NVBug 6555875) swapped perf_metrics for _collect_perf_metrics, and both refreshed the fake rather than waiving it. No NVBug has been filed for this occurrence.

Separately, #18692 merged after this PR was opened and added:

full:DGX_B200/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py::TestFlux1TextToImage SKIP (https://nvbugs/6720944)
full:DGX_B200/unittest/_torch/visual_gen/test_trtllm_serve_e2e.py::TestFlux2TextToImage SKIP (https://nvbugs/6720944)

Those four test_t2i_sync_* cases fail on the same get_steady_clock_now_in_seconds NameError, so this PR fixes them too and that waiver should come out alongside the test_trtllm_serve_endpoints.py one you already remove. Your waives.txt hunk still applies cleanly to current main cf45d3ce8b, so this is an extra deletion rather than a rebase conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants