feat: Adds an opt-in asynchronous render path (ovrtx only) - #7075
feat: Adds an opt-in asynchronous render path (ovrtx only)#7075pv-nvidia wants to merge 15 commits into
Conversation
8362c6f to
ebd7ffd
Compare
Greptile SummaryThe PR adds opt-in, one-frame-deferred OVRTX rendering with double-buffered transform staging, renderer configuration and environment-variable controls, backend fallbacks, and rendering tests. The teardown fix still drains queued frames only after camera-owned destinations have been cleared.
Confidence Score: 4/5The PR is not yet safe to merge because camera teardown still clears queued render destinations before the asynchronous queue is drained. A queued frame retains its camera render data, but camera cleanup clears that data before RenderContext invokes renderer close; the later drain consequently cannot deliver the retained output. Files Needing Attention: source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py Important Files Changed
Reviews (3): Last reviewed commit: "Leave the ovstage write path unchanged: ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The renderer-agnostic asynchronous-rendering option and OVRTX strategy split are additive, but the ovstage implementation cannot honor configured queue depths greater than one frame because every subsequent scene write drains all pending renders.
- Design and architecture: The strategy abstraction cleanly separates synchronous and pipelined execution. However, all per-frame ovstage mutations pass through
_write_attribute_ovstage(), whose unconditionalsettle_before_scene_write()drains the complete render queue. This limits ovstage to one pending render regardless of the requested latency and should be documented or surfaced through clamping and a warning. - API: The new configuration field, environment override, helper functions, exports, documentation, and changelog entries preserve synchronous behavior by default. The documented contract that larger integer values keep more renders in flight is not met on the advertised ovstage path for values greater than one.
- Implementation: Ordinal forwarding, first-frame priming, deferred delivery to the original render buffers, and close-time draining are explicitly handled. The remaining actionable issue is the ovstage write barrier’s full-queue drain, which collapses all configured depths above one to the same effective pipeline depth.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
1eb6b6a to
fac3d5c
Compare
fac3d5c to
c996b69
Compare
…mera latency to trade for throughput. False/0 renders synchronously (default), True is one frame, larger integers keep more renders in flight. The ISAAC_LAB_ASYNC_RENDERING environment variable overrides it with the same values for any task. Only the OVRTX renderer implements the pipelined path, on both the legacy and ovstage scene-ownership paths; every ovstage scene write first drains in-flight renders, since OVRTX reads the stage's storage in place. Other renderers warn and render synchronously.
The synchronous OVRTX strategy now hands the device's current Warp stream to the attribute unmap, as the OVRTX API contract requires for GPU mappings: the commit must synchronize with the stream that filled the mapped buffer. In the current OVRTX implementation the commit kernel runs on the legacy default CUDA stream, which already serializes with Warp's blocking streams, so this codifies an ordering that previously held only as an implementation detail. The staging contract and call sites document why the Warp fill launches need no explicit stream of their own: they enqueue on the device's current stream, the same stream publication hands to OVRTX. The pixel-tolerance table keeps scalar entries for sync-only environments and uses a [sync, async] pair only for cartpole, with the lookup helper handling both shapes; the AST/regex guard test for the table was overkill and is removed.
The bool | int frame count promised a contract only one of the two
scene-ownership paths can honor. ovstage retains a single committed
snapshot ("latest committed snapshot only; an ordinal passed to
rendering is a publication gate, not a historical snapshot selector")
and the default BORROW attach makes renders read it in place
("rendering may observe that publication or a later one"), so each
frame's scene writes must drain the render still in flight and that
path can never sustain more than one frame of latency. Rather than
shipping an integer that means different things per path, the first
release supports exactly one frame everywhere; the strategy internals
keep the ring/queue shape so a follow-up can re-widen the config once
deeper legacy-path queues measure a benefit, or once ovstage retains
per-ordinal payload history (explicitly deferred in its future-work
notes as of ovstage 0.3).
resolve_async_rendering_frames / async_rendering_frames_from_env become
resolve_async_rendering_enabled / async_rendering_enabled_from_env.
Frame counts above one are rejected in the config (ValueError) and
ignored with a warning in the environment variable.
c996b69 to
1089f13
Compare
Re-widens RendererCfg.async_rendering to bool | int: an integer n > 0 keeps n renders in flight on the OVRTX legacy scene-ownership path, so camera outputs describe the simulation state from n steps earlier. The ovstage path still sustains at most one frame -- it retains a single committed snapshot that renders read in place, so each frame's scene writes drain the render still in flight -- and larger values are clamped to 1 there with a warning that says why. resolve_async_rendering_frames / async_rendering_frames_from_env return; the *_enabled variants shipped by the first PR remain as the boolean view, so no released API is removed. Draft for the follow-up PR to isaac-sim#7075: before opening, attach a benchmark showing a workload where renders outlast a full CPU frame, which is the regime where depth beyond one pays.
…cfg parse errors, test env isolation - Drain in-flight renders in _render_ovstage before material publishes and the write-floor advance, which mutate the stage outside the _write_attribute_ovstage funnel. - Make the slot reset in _AsyncRenderStrategy.cleanup() best-effort so a failed binding write cannot abort the renderer's teardown. - Raise the documented ValueError (not AttributeError) when RendererCfg.async_rendering holds a non-bool/int/str value such as a Hydra null. - Clear an ambient ISAAC_LAB_ASYNC_RENDERING in the isaaclab_tasks test suite so it cannot flip the synchronous rendering lanes or their tolerance selection; the async lane re-sets it in its test body.
…gers The staging slot lifecycle was implicit and order-dependent: whichever staging call ran first opened the frame's slot, and stage_object_transforms rotated unconditionally on the assumption it ran first. The real frame order is camera-first, so every frame consumed both slots and each _begin_slot host-blocked on a write submitted after the render still in flight, defeating the double-buffering. Slots now rotate in exactly one place - _advance_slot, called when the frame's render is enqueued - and both stagers share the frame's slot via _staging_slot in whichever order they run. Advancing after the enqueue's drain also makes the incoming slot's wait_for_writes complete immediately in steady state.
- OVRTXVisualMaterialWriter.close() settles the render strategy before releasing its bindings: RenderContext.close() closes writers before renderers, so the final queued render could still be reading them. - _reset_slots delivers queued renders before clearing the ring, so a re-initialize cannot discard a frame still executing (each op is its buffer's only keepalive). - Document that first-frame priming is per scene, not per camera.
The priming drain guaranteed a rendered first frame only for the first render() after (re)initialization, so a second camera sharing the renderer read its zero-initialized buffers on frame 0. Primed targets are now tracked per render data in a WeakSet: each camera's first frame drains synchronously (and duplicates on its second read, like every pipelined frame), a released camera drops out on its own, and _reset_slots clearing the set preserves the re-initialize contract.
This reverts commit 12fc113.
The scene (render products, camera bindings, resolution) is built from the first camera's spec and later specs are silently ignored, so a second camera deduped onto the renderer by RenderContext.get_renderer's config equality receives the first camera's images (verified empirically with duo_camera + ovrtx on Isaac-Lift-KukaAllegro-Camera). Warn at create_render_data so the unsupported sharing is visible at env construction instead of producing silently duplicated observations.
Benchmarks show async rendering on the ovstage path gains nothing over synchronous rendering: every frame's scene writes drain the render still in flight, so the pipeline window is too small to pay off. Requesting async with ovstage now warns and renders synchronously; the feature is postponed to a follow-up pull request. The scene-write barriers stay in place, so the follow-up only changes strategy selection and the ovstage snapshot handling. The ovstage async CI lane is removed - it would only duplicate the synchronous ovstage coverage.
With asynchronous rendering gated off on the ovstage path, the _write_attribute_ovstage funnel and its drains defended a combination that can no longer be constructed. Restore the direct write_attribute().wait() calls so this pull request does not touch the ovstage write path; the follow-up pull request that enables ovstage async must reintroduce the barriers together with the strategy selection change.
| # Drain in-flight renders before either backend releases the renderer that owns them; each | ||
| # queued frame delivers into the buffers it was submitted with. | ||
| self._strategy.cleanup() |
There was a problem hiding this comment.
Queued outputs lose destinations
When a camera is invalidated or destroyed with an asynchronous render queued, its cleanup clears warp_buffers before RenderContext closes the renderer. The later strategy drain delivers into that cleared map, so extraction is skipped and the queued camera frame is dropped.
Description
Adds an opt-in asynchronous render path for the OVRTX renderer. Rendering then overlaps the next step's simulation and Python work, and camera outputs describe the simulation state from one step earlier.
RendererCfg.async_rendering: boolenables it (defaultFalse: synchronous, unchanged). TheISAAC_LAB_ASYNC_RENDERINGenvironment variable overrides the setting for any task.env.step()throughput +37-82%. When async rendering is disabled, throughput is unchanged within noise.Internally, frame execution goes through a render strategy selected at construction: synchronous inline, or one in-flight
step_asyncwith double-buffered transform staging. The first frame of a scene is drained immediately, so the first camera read is never blank.Testing: kitless unit tests (config resolution, strategy selection, ordinals, staging, teardown); async golden-image variants of the Cartpole rendering tests in post-merge CI, graded against the same goldens with
[sync, async]tolerance pairs; benchmarks-guide section and changelog fragments.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there