[CI probe, do not merge] Run the Kit visualizer pause checks on CI hardware (NVBUG 6570125) - #7055
Closed
fatimaanes wants to merge 7 commits into
Closed
[CI probe, do not merge] Run the Kit visualizer pause checks on CI hardware (NVBUG 6570125)#7055fatimaanes wants to merge 7 commits into
fatimaanes wants to merge 7 commits into
Conversation
RTX ResponsiveDenoising keeps refining a paused frame, so two captures taken while simulation is frozen differ by a small number of high-amplitude pixels. Amplitude separates that residue from real motion and the pixel count does not: at the shared threshold of 50 the paused noise reaches 1.55x the weakest real motion signal, so any count-based gate loose enough to stay quiet is also blind to a genuine regression. _assert_frames_remain_stable now takes a channel_diff_threshold, raised to 80 at the Kit viewport pause and 150 at the Kit tiled camera pause. The tiled override is gated on KitVisualizer because all four tiled cases share _attempt_pause and the Newton-visualizer cells sit at zero noise. Newton ViewerGL rasterises without DLSS and keeps the strict default. The pixel gate stays at 100. Both paused tiled captures render fresh again, reverting #6658. Comparing the sensor's cached frame against itself cannot detect a renderer that keeps changing the image after physics stops, which left that assertion unable to fail. Measured on L40 / driver 595.58.03 / Isaac Sim 6.1.0-alpha.47. Work-around for NVBUG 6570125; remove once OVRTX 0.5 ships the fix.
These checks have been dead weight for a while. RTX responsive denoising keeps refining the image after physics stops, so two frames captured during a pause differ by ~900 pixels and the test went red constantly. #6658 quietened it by comparing the camera's cached frame against itself, which can't fail whatever the renderer does. The denoiser residue is a few very bright pixels; real motion is spread across many. So count on amplitude instead: only pixels differing by 160 or more (80 in the viewport). The shimmer drops out, real movement doesn't. Measured over 24 runs on an L40, and confirmed the check still fails when fed actual motion. Worth being clear about the scope: no physics can step inside this pause window, so this is a renderer-stability check, not a pause-correctness one. It catches a gross instability, not a subtle one. Work-around/Temp fix for NVBUG 6570125. Need to remove once the real fix is in.
Shortened the comment regarding NVBUG 6570125 for clarity. Signed-off-by: fanes <74020209+fatimaanes@users.noreply.github.com>
Collaborator
Author
|
CI probe complete. All four visualizer integration tests passed on L40S (g6e.2xlarge), two independent runners, first pass, no retries. The L40→L40S transfer question is answered. #7049 is the PR of record. |
The pause pixel-count log never reaches CI: every pause call site runs inside caplog.at_level(logging.WARNING), which hard-sets the root logger, so the module logger's effective level is WARNING and info() short-circuits. Raising it to WARNING would instead self-trip _assert_no_visualizer_log_issues once ASSERT_VISUALIZER_WARNINGS is enabled. The assert message already carries the count, threshold and gate on the run where it matters. Give the tiled PhysX cell its own constant rather than borrowing the viewport one. It measures 103 differing pixels at the default threshold against a gate of 100, so lowering the viewport value later would silently break it.
Collaborator
Author
|
Closing — this served its purpose. This PR was never intended to merge. Review continues on #7049. |
kellyguo11
pushed a commit
that referenced
this pull request
Aug 14, 2026
Temporary workaround for NVBUG 6570125 while we wait for the upstream renderer fix. ## Problem RTX responsiveDenoising continues refining the image after physics stops, so fresh renders captured during a pause are no longer stable. The affected Kit visualizer checks are currently green for the wrong reasons: Tiled camera: #6658 changed the paused captures to reuse the cached sensor frame, so the assertion effectively compares the same frame against itself and can't detect renderer instability. Viewport: still performs a real comparison, but the known denoiser noise can push it over the existing threshold and the failure is absorbed by retries. ## What this changes This PR restores fresh renders for the tiled pause check and adjusts the per-channel difference threshold only for the affected Kit RTX pause paths. The existing 100-pixel gate stays unchanged. We're changing how large a per-channel difference must be before a pixel is counted, rather than simply allowing more differing pixels. | site | @50 | @80 | @100 | @120 | @150 | @160 | outcome | |---|---|---|---|---|---|---|---| | Kit viewport (Newton) | 67 | **12** | 8 | 7 | 3 | — | 80 | | Kit viewport (PhysX) | 14 | 4 | 3 | 1 | 0 | — | 80 | | Kit tiled (Newton) | 999 | 604 | 226 | 120 | 59 | **49** | 160 | | Kit tiled (PhysX) | 103 | 4 | 4 | 2 | 0 | — | 80 | | Newton ViewerGL | 0 | 0 | 0 | 0 | 0 | — | 50 | Values are the number of differing pixels at each per-channel threshold. For the margin-critical case (Kit tiled camera, Newton backend), threshold 160 leaves at most 49 px of paused denoiser noise (worst of n=36) against at least 160 px with real motion (weakest of n=72), both measured with the denoiser enabled. With the existing 100-pixel gate the known renderer noise passes at 2.0× margin while real motion is still detected at 1.6×. ## Changes `_assert_frames_remain_stable` now accepts a per-call `channel_diff_threshold`; the default remains 50. Kit RTX pause checks use 80 for the viewport, 160 for the tiled camera on Newton, and 80 for the tiled camera on PhysX — three separate constants, since the tiled PhysX cell measures 103 px at the default threshold and must not silently track a later change to the viewport value. Newton ViewerGL remains at the strict default since it doesn't exercise the affected DLSS path. `max_differing_pixels` remains 100 everywhere. The tiled pause check is restored to fresh renders instead of cached frames, so the assertion is actually exercising renderer stability again. ## Validation With retries disabled, the workaround passed 40/40 runs across all four affected configurations. As a control, restoring the original threshold of 50 reproduced the failure 6/6 times at 766–949 differing pixels. Those six failures are all the **tiled** assertion: the control ran under `pytest -x` and the viewport test is defined first in the same file, so the run aborted at the tiled test and the viewport site passed at 50 in those reps. The control therefore demonstrates that the tiled workaround filters real denoiser noise rather than simply making the test pass; it is not evidence for the viewport site. The viewport raise from 50 to 80 rests instead on production CI, where the assertion has really been failing (`107 pixels differed, expected at most 100`, with a historical band reaching 161 px), plus n=5 bench samples showing 67 px at threshold 50 falling to 12 px at 80. The same change was validated on three independent L40S CI runners through #7055, with all four visualizer integration tests passing first-pass, no retries. Restoring fresh renders also had no meaningful runtime impact (~27s median). This is a temporary workaround for NVBUG 6570125 and should be removed or re-measured once the upstream fix is available. That revert is load-bearing rather than cosmetic: with the denoiser disabled — the world after the renderer fix — the weakest real motion in the governing cell measures 80 px at threshold 160, which is *below* the 100-pixel gate. Leaving 160 in place after the fix ships would make the tiled check blind to the weakest motion it exists to catch, so the removal comment on the constants is the thing guarding that. --------- Signed-off-by: fanes <74020209+fatimaanes@users.noreply.github.com>
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
1 participant
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.
CI probe — not for merge. The PR of record is #7049.
This branch carries the exact same commits as #7049. It exists only to get the test suite to run.
Why this duplicate exists
#7049 comes from a fork, and fork PRs receive no repository secrets. Without
NGC_API_KEYtheBuild Base Docker Imagejob cannot log in tonvcr.ioand fails at the Isaac Sim base image pull, and because every test job carriesneeds: [build, config], all ~26 of them report SKIPPED — includingtest-isaaclab-visualizers, the job that runs the very test #7049 re-enables. That PR's central claim is therefore unverifiable by its own pipeline.Opening the same change from an in-repo branch restores secrets, so the test matrix actually runs.
What this run is meant to measure
The paused-frame thresholds in #7049 (viewport 80, Kit tiled camera 160) were calibrated on an L40 (driver 595.58.03, Isaac Sim 6.1.0-alpha.47). CI runs on
g6e.2xlarge, i.e. an L40S — same Ada AD102 family, different SKU.There is one cell where the two can be compared directly, and they disagree. On the Kit viewport pause site at a channel threshold of 50, CI has been observed at 107 px (job 93935796248,
expected at most 100), with a historical failing band of 105–161 px. The same cell measured on the L40 bench peaked at 67 px over 5 samples. CI runs roughly 1.6× hotter there.The Kit tiled cell — the one that governs the 160 constant — has never produced a real number on CI hardware at all, because #6658 set both paused captures to
force_recompute=Falseand left the assertion comparing a cached frame against itself. Its measured headroom (49 px against a gate of 100) is entirely L40-derived.This run produces that missing number. The assertion prints its pixel count on failure, so a red result is as useful as a green one.
Disposition
Closing once the run completes. Any threshold change it motivates lands on #7049.