[https://nvbugs/6720250][fix] Use adjusted clock for VisualGen timing - #18686
[https://nvbugs/6720250][fix] Use adjusted clock for VisualGen timing#18686chienchunhung wants to merge 2 commits into
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast --stage-list "CPU-Generic-x86-1,CPU-Generic-arm-1" --detailed-log |
|
PR_Github #71359 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. WalkthroughThe 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. ChangesVisual generation timing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to 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: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation 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)
Comment |
|
PR_Github #71359 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #71379 [ run ] triggered by Bot. Commit: |
|
PR_Github #71379 [ run ] completed with state
|
|
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.
That test builds its server with I built main HEAD on a B200 node and ran both before and after applying this PR. Your diff takes 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 Separately, #18692 merged after this PR was opened and added: Those four |
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
AdjustedSteadyClock.NVBUG 6720250waiver forunittest/_torch/visual_gen/test_trtllm_serve_endpoints.py.QA Engineer Review
totaldurations.tests/integration/test_lists/waives.txtonly through the removed waiver entry. No new test-list entry was added.