Add Quick Start client and Isaac Lab application validation#773
Add Quick Start client and Isaac Lab application validation#773arussell-nvidia wants to merge 27 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add bundled IWER installation and headless OOB auto-connect support to the WebXR client. CloudXR gains hub-only launch mode, environment-selected backend ports, and runtime reuse detection. New browser probes and Bash runners validate standard Quick Start, hosted-client, and Isaac Lab workflows. Ubuntu GPU workflow jobs execute these validations, upload artifacts, and gate wheel publication. Documentation describes the available CloudXR launch modes. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant index as index.tsx
participant loader as LoadIWER
participant App
participant CloudXR2DUI
participant OOBChannel
index->>loader: preload bundled IWER
loader-->>App: report XR readiness
App->>CloudXR2DUI: requestConnect()
OOBChannel->>App: deliver StreamConfig
App->>CloudXR2DUI: applyOobStreamConfig(config)
sequenceDiagram
participant Workflow
participant Runner
participant CloudXR
participant BrowserProbe
Workflow->>Runner: execute Quick Start E2E
Runner->>CloudXR: start and await readiness
Runner->>BrowserProbe: validate hosted client and OOB state
BrowserProbe-->>Runner: return validation result
Runner->>CloudXR: run teleoperation workflow
Runner-->>Workflow: publish logs and JSON artifacts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3d55958 to
8de2d5b
Compare
8de2d5b to
4c4ff35
Compare
# Conflicts: # src/core/cloudxr/python/launcher.py # src/core/cloudxr_tests/python/test_launcher.py
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com> # Conflicts: # src/core/cloudxr/python/launcher.py # src/core/cloudxr_tests/python/test_launcher.py
Signed-off-by: Andrew Russell <arussell@nvidia.com> # Conflicts: # deps/cloudxr/webxr_client/webpack.chunkNames.js # docs/source/getting_started/quick_start.rst
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
Signed-off-by: Andrew Russell <arussell@nvidia.com>
4c4ff35 to
ef34e22
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Andrew Russell <arussell@nvidia.com>
|
Updated this draft to target This branch now consolidates the Quick Start runtime validation baseline from #706 with the hosted desktop/client validation probe, and adds a skipped-by-default Isaac Lab validation hook behind The refreshed run has |
arussell-nvidia
changed the title
|
Ready for review. This is the public GitHub implementation for Quick Start client validation and the opt-in Isaac Lab application path described at the bottom of the Quick Start guide. @qingsi-at-nv @aristarkhovNV @yanziz-nvidia, please review the implementation and the stated boundary: the code path is ready for review, while a provisioned Isaac Lab run is still required before that environment can be claimed as passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deps/cloudxr/webxr_client/src/App.tsx`:
- Around line 306-332: Update the auto-connect flow in the capability-check
effect and CloudXR2DUI initialization flow so it targets the latest CloudXR2DUI
instance after iwerLoaded causes store recreation. Do not let
capabilitiesCheckedRef permanently suppress the trigger: re-arm
autoConnectTriggeredRef or otherwise rerun the auto-connect retry when the
CloudXR2DUI identity changes, while preserving the existing
URL/headless/auto-connect guards and retry behavior.
In `@deps/cloudxr/webxr_client/src/index.tsx`:
- Around line 24-35: Update shouldPreloadIWERForHeadlessOob to reuse App.tsx’s
canonical readUrlParam and URL_PARAMS definitions for oobEnable, autoConnect,
and headless instead of creating URLSearchParams and using isEnabled. Preserve
the same enabled-value behavior while ensuring aliases and validators remain
consistent with isOobHeadlessAutoConnect.
In `@src/core/cloudxr/python/launcher.py`:
- Around line 391-404: Update CloudXRLauncher._has_sourced_cloudxr_runtime_env
to require both the existing environment markers and a live runtime/proxy signal
before reporting the external runtime as reusable. In launcher.py lines 407-448,
ensure nullcontext and related reuse paths occur only after this liveness check;
otherwise start the runtime and WSS normally. In test_launcher.py lines 475-506,
add stale-sourced-environment coverage and update reuse tests to model a live
runtime.
🪄 Autofix (Beta)
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: 9a39ed40-344f-4f8c-b09f-81d4e5797f7c
📒 Files selected for processing (18)
.github/workflows/build-ubuntu.ymldeps/cloudxr/webxr_client/helpers/LoadIWER.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdeps/cloudxr/webxr_client/src/config/params.tsdeps/cloudxr/webxr_client/src/index.tsxdeps/cloudxr/webxr_client/webpack.chunkNames.jsdocs/source/getting_started/quick_start.rstscripts/quick_start_client_probe.pyscripts/run_quick_start_e2e.shscripts/run_quick_start_isaac_lab_e2e.shsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/env_config.pysrc/core/cloudxr/python/launcher.pysrc/core/cloudxr/python/oob_teleop_env.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/test_launcher.pysrc/core/cloudxr_tests/python/test_oob_teleop_env.py
|
|
||
| const searchParams = new URLSearchParams(window.location.search); | ||
| if ( | ||
| isOobEnabled(searchParams) && | ||
| isOobAutoConnectEnabled(searchParams) && | ||
| cloudXR2DUI.getConfiguration().headless && | ||
| !autoConnectTriggeredRef.current | ||
| ) { | ||
| autoConnectTriggeredRef.current = true; | ||
| const requestConnect = (attemptsRemaining: number) => { | ||
| window.setTimeout(async () => { | ||
| try { | ||
| setErrorMessage(''); | ||
| await cloudXR2DUI.requestConnect(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (attemptsRemaining > 0 && message.includes('not connected to three.js')) { | ||
| requestConnect(attemptsRemaining - 1); | ||
| return; | ||
| } | ||
| autoConnectTriggeredRef.current = false; | ||
| setErrorMessage(`Failed to auto-start XR session: ${error}`); | ||
| } | ||
| }, 500); | ||
| }; | ||
| requestConnect(20); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Auto-connect can silently die when IWER installs after mount (store gets recreated out from under the pending retry).
store's deps include iwerLoaded (line 396), so when the post-mount IWER-load effect (214-237) flips iwerLoaded false→true, store is recreated and the CloudXR2DUI-init effect (406-462, deps [store]) tears down the old CloudXR2DUI (cleanup() nulls connectAction) and swaps in a new one. But the check-capabilities effect (247-336) is a one-shot gated by capabilitiesCheckedRef; it fires with the old cloudXR2DUI (captured via normal closure at that render) and schedules the requestConnect(20) retry loop against it. Once capabilitiesCheckedRef.current is set, the auto-connect logic never re-runs for the freshly created UI instance.
When the scheduled retry finally calls cloudXR2DUI.requestConnect() on the now-cleaned-up instance, connectAction is null, so it throws Error('Connect handler is not ready') — a message that doesn't match the only retryable string ('not connected to three.js', line 322), so the loop aborts on the first attempt instead of retrying.
This only manifests when iwerLoaded is still false on first render (i.e. index.tsx's preload hasn't already landed before mount) — exactly the fallback path this feature is designed to handle — so it's a realistic way for the new headless Quick Start validation to fail non-deterministically in CI.
Consider decoupling "which CloudXR2DUI to auto-connect" from the one-shot capability-check gate, e.g. keep a ref to the latest cloudXR2DUI and re-arm autoConnectTriggeredRef/re-run the auto-connect trigger when cloudXR2DUI identity changes after IWER becomes ready, rather than only firing it inside the guarded checkCapabilitiesOnce.
Also applies to: 355-397
🤖 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 `@deps/cloudxr/webxr_client/src/App.tsx` around lines 306 - 332, Update the
auto-connect flow in the capability-check effect and CloudXR2DUI initialization
flow so it targets the latest CloudXR2DUI instance after iwerLoaded causes store
recreation. Do not let capabilitiesCheckedRef permanently suppress the trigger:
re-arm autoConnectTriggeredRef or otherwise rerun the auto-connect retry when
the CloudXR2DUI identity changes, while preserving the existing
URL/headless/auto-connect guards and retry behavior.
| function isEnabled(value: string | null): boolean { | ||
| return value === '1' || value?.toLowerCase() === 'true'; | ||
| } | ||
|
|
||
| function shouldPreloadIWERForHeadlessOob(): boolean { | ||
| const params = new URLSearchParams(window.location.search); | ||
| return ( | ||
| isEnabled(params.get('oobEnable')) && | ||
| isEnabled(params.get('autoConnect')) && | ||
| isEnabled(params.get('headless')) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duplicate URL-param logic that can drift from the canonical parser.
shouldPreloadIWERForHeadlessOob re-implements oobEnable/autoConnect/headless detection via raw URLSearchParams.get(), while App.tsx uses readUrlParam/URL_PARAMS (which apply isValid/url aliasing). If either param ever gains an alias or validator, this preload gate silently diverges from App's isOobHeadlessAutoConnect, skipping the preload and pushing the IWER install into the post-mount effect — which is the trigger condition for the stale-cloudXR2DUI-closure race flagged in App.tsx.
♻️ Reuse the canonical param reader
+import { readUrlParam } from './config/resolve';
+
-function isEnabled(value: string | null): boolean {
- return value === '1' || value?.toLowerCase() === 'true';
-}
-
function shouldPreloadIWERForHeadlessOob(): boolean {
const params = new URLSearchParams(window.location.search);
- return (
- isEnabled(params.get('oobEnable')) &&
- isEnabled(params.get('autoConnect')) &&
- isEnabled(params.get('headless'))
- );
+ const isEnabled = (v: string | null) => v === '1' || v?.toLowerCase() === 'true';
+ return (
+ isEnabled(readUrlParam(params, 'oobEnable')) &&
+ isEnabled(readUrlParam(params, 'autoConnect')) &&
+ isEnabled(readUrlParam(params, 'headless'))
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isEnabled(value: string | null): boolean { | |
| return value === '1' || value?.toLowerCase() === 'true'; | |
| } | |
| function shouldPreloadIWERForHeadlessOob(): boolean { | |
| const params = new URLSearchParams(window.location.search); | |
| return ( | |
| isEnabled(params.get('oobEnable')) && | |
| isEnabled(params.get('autoConnect')) && | |
| isEnabled(params.get('headless')) | |
| ); | |
| } | |
| function shouldPreloadIWERForHeadlessOob(): boolean { | |
| const params = new URLSearchParams(window.location.search); | |
| const isEnabled = (v: string | null) => v === '1' || v?.toLowerCase() === 'true'; | |
| return ( | |
| isEnabled(readUrlParam(params, 'oobEnable')) && | |
| isEnabled(readUrlParam(params, 'autoConnect')) && | |
| isEnabled(readUrlParam(params, 'headless')) | |
| ); | |
| } |
🤖 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 `@deps/cloudxr/webxr_client/src/index.tsx` around lines 24 - 35, Update
shouldPreloadIWERForHeadlessOob to reuse App.tsx’s canonical readUrlParam and
URL_PARAMS definitions for oobEnable, autoConnect, and headless instead of
creating URLSearchParams and using isEnabled. Preserve the same enabled-value
behavior while ensuring aliases and validators remain consistent with
isOobHeadlessAutoConnect.
| @staticmethod | ||
| def _has_sourced_cloudxr_runtime_env() -> bool: | ||
| """Return true when the current shell looks sourced from ``cloudxr.env``.""" | ||
| return bool( | ||
| os.environ.get("XR_RUNTIME_JSON") and os.environ.get("NV_CXR_RUNTIME_DIR") | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _resolve_launch_cloudxr_runtime(args: argparse.Namespace) -> bool: | ||
| """Resolve auto-launch behavior for documented two-terminal workflows.""" | ||
| launch_cloudxr_runtime = getattr(args, "launch_cloudxr_runtime", None) | ||
| if launch_cloudxr_runtime is not None: | ||
| return bool(launch_cloudxr_runtime) | ||
| return not CloudXRLauncher._has_sourced_cloudxr_runtime_env() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Verify runtime liveness before auto-reusing it.
cloudxr.env remains sourceable after its runtime exits, so these variables alone can suppress both runtime and WSS startup. Check a live runtime/proxy before returning nullcontext; otherwise launch normally.
src/core/cloudxr/python/launcher.py#L391-L404: require a liveness signal, not only configuration variables.src/core/cloudxr/python/launcher.py#L407-L448: only reuse the external runtime after that liveness check succeeds.src/core/cloudxr_tests/python/test_launcher.py#L475-L506: add stale-sourced-environment coverage and make reuse tests model a live runtime.
📍 Affects 2 files
src/core/cloudxr/python/launcher.py#L391-L404(this comment)src/core/cloudxr/python/launcher.py#L407-L448src/core/cloudxr_tests/python/test_launcher.py#L475-L506
🤖 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 `@src/core/cloudxr/python/launcher.py` around lines 391 - 404, Update
CloudXRLauncher._has_sourced_cloudxr_runtime_env to require both the existing
environment markers and a live runtime/proxy signal before reporting the
external runtime as reusable. In launcher.py lines 407-448, ensure nullcontext
and related reuse paths occur only after this liveness check; otherwise start
the runtime and WSS normally. In test_launcher.py lines 475-506, add
stale-sourced-environment coverage and update reuse tests to model a live
runtime.
|
I rechecked the Isaac Lab scope against the public documentation:
Local Python compilation of the probe and focused test modules passes. The full public PR check set remains green; the provisioned Isaac Lab job is still expected to skip until its checkout is available. |
|
Addressed the three review findings in 23eef1d: auto-connect retries now stay bound to the current CloudXR UI instance after IWER store recreation, startup URL parameters use the canonical resolver, and sourced runtime reuse now requires live local runtime and WSS listeners. Python compile and focused stale/live environment tests pass locally; the full PR checks are running. |
|
Current head 069bb07 is terminal green: 46 checks succeeded, 3 expected checks were skipped, and 0 failed. The requested reviewers remain Andrei, Yanzi, and Qingsi. |
2 participants
Summary
mainIsaac Lab application scope
When
QUICK_START_ISAAC_LAB_ROOTorISAAC_LAB_ROOTpoints to a provisioned Isaac Lab checkout, the workflow:teleop_se3_agent.pywith theIsaacContrib-Stack-Cube-Franka-IK-Abstask and--xrThis is the public GitHub implementation for the Isaac Lab application validation requested from the Quick Start page. The optional job remains skipped on runners without a provisioned Isaac Lab checkout, so a provisioned-run result is still required before claiming that environment has passed.
Validation
mainReview status
Ready for implementation review. Review readiness does not claim that the provisioned Isaac Lab execution or final merge approval is complete.
Summary by CodeRabbit