examples(deviceio_live_view): package under isaacteleop_examples - #1034
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:
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 (7)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughAdds a packaged DeviceIO Live View example. The example combines human-tracking outputs and renders hands, controllers, full-body joints, and head pose through viser. It handles absent, invalid, and untracked inputs. It adds module-based launching, Hatchling packaging, a README, and updated reference documentation. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR reorganizes the example into a namespaced package and adds a module entry point without changing its behavior; the documented checks cover the packaging and import paths, so no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant DeviceIO
participant OutputCombiner
participant HumanDeviceIOViz
participant Viser
DeviceIO->>OutputCombiner: provide tracking outputs
OutputCombiner->>HumanDeviceIOViz: deliver combined data
HumanDeviceIOViz->>Viser: update geometry and controller HUD
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
437d7bd to
02aa0b7
Compare
02aa0b7 to
529e458
Compare
623bc27 to
e9b6e9b
Compare
ivany-nv
left a comment
ivany-nv
left a comment
There was a problem hiding this comment.
Built and installed the full CMake install tree on x86_64 Linux, default prefix — the case the description lists as not covered. Re-verified on e9b6e9b9. Layout, generated pyproject, and uv run python -m isaacteleop_examples.deviceio_live_view --help from the install tree all check out: no doubled python/python/, no stray isaacteleop_examples/__init__.py, find-links resolves to <prefix>/wheels with the built wheel, isaacteleop pinned and resolved from there rather than an index, legacy examples install unchanged, clang_format_check and pre-commit clean.
One fix needed, in the macro: the example-root README.md is not installed, since install_python_example() only installs python/. No-op for deviceio_live_view, but three downstream examples ship python/README.md into the install tree today and lose it on conversion:
- #1044 —
rename from examples/haptic_feedback/python/README.md to examples/haptic_feedback/README.md - #1036 —
examples/teleop_session_manager/python/README.mddeleted - #1046 —
examples/oxr/python/README.mddeleted
install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/README.md"
DESTINATION ${_IPE_DESTINATION} OPTIONAL)Here or in #1036, your call.
a9205a6 to
f8239a8
Compare
ivany-nv
left a comment
ivany-nv
left a comment
There was a problem hiding this comment.
Re-approving for f8239a89. Re-ran the install-tree verification on the new head: layout, generated pyproject, uv run python -m isaacteleop_examples.deviceio_live_view --help from the install tree, clang_format_check and pre-commit all still clean. The macro is byte-identical, and the delta since e9b6e9b9 is runtime-only (GroundGrid), so nothing in the packaging conclusion changes.
The two open items from the previous review still stand: the example-root README.md is still not installed, and the --host default flip is still there.
a916fca to
46e2a82
Compare
examples/README.md tells example authors to put pyproject.toml at the example root with sources under python/, and to install with `install_python_example(DESTINATION examples/<name>)`. The macro only reads python/pyproject.toml, so that call installs nothing usable. Pick the layout by where pyproject.toml sits. Two things follow from it: the `find-links` depth, which is relative to the installed pyproject, and whether python/ is installed as a directory or flattened into the destination -- a converted example must keep it so the namespace root matches the source tree. No example uses the converted layout yet, so this is a no-op today. The legacy branch goes away with the last conversion (#985). Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Every viser viewer in examples/ runs where the headset is -- a workstation or a Jetson under the play space -- and gets opened from a laptop somewhere else on the network. Binding 127.0.0.1 means the one machine that cannot practically open the browser is the only one allowed to. Flip the default on all eight, and make the startup line report the address it actually bound instead of always printing localhost, which was already wrong whenever --host was passed. Pass --host 127.0.0.1 for the old behaviour. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
live_deviceio.py imported its sibling as a top-level module, which resolved
only because the invoked script's own directory landed on sys.path. That breaks
under `python -m`, breaks when the files are copied into another project, and
squats the bare name `deviceio_viser`. Adopt the layout examples/README.md
describes: a distribution owning isaacteleop_examples/<name>, relative imports,
a `-m` entry point, and pyproject.toml at the example root.
uv pip install -e ./examples/deviceio_live_view
python -m isaacteleop_examples.deviceio_live_view
Fix the grid while here. It lay in viser's default XY plane, which stands up as
a wall once the up direction is +y; it is now an xz ground plane that follows
the lowest tracked joint, so it lands on the floor in a head-relative space too,
with a camera framed against it.
First of the per-example conversions for #985.
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
46e2a82 to
e956e4d
Compare
common.py's GroundGrid (mcap_record_replay) and deviceio_viser.py's (deviceio_live_view, landed on main via #1034) are byte-identical. Not deduping: each example package is self-contained by design (see examples/README.md), and a shared helper would make one example depend on another. Leave a pointer in each copy instead of letting the duplication look accidental. Flagged by ivany-nv's verification pass on #1038. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
3 participants
Description
Part of #985; #1035 has landed. Split into three commits per review — the two preparation pieces stand on their own and can be read (or landed) without the conversion.
1.
cmake: let install_python_example() take either example layout—examples/README.mdalready tells authors to putpyproject.tomlat the example root and callinstall_python_example(DESTINATION examples/<name>), but the macro only readpython/pyproject.toml, so that call installed nothing usable. It now picks the layout by wherepyproject.tomlsits, which decides thefind-linksdepth and whetherpython/is installed as a directory or flattened. No example uses the converted layout at this commit, so it is a no-op; the legacy branch goes away with the last conversion.2.
examples: bind viser viewers to every interface by default— @ivany-nv was right that a one-example flip was wrong, so this goes the other way and moves all eight. These viewers run where the headset is and get opened from a laptop elsewhere on the network;127.0.0.1means the one machine that cannot practically open the browser is the only one allowed to. The startup line now reports the address it actually bound, which was already wrong whenever--hostwas passed. The# noqa: S104is gone — ruff selects noSrules here.--host 127.0.0.1restores the old behaviour.3.
examples(deviceio_live_view): package under isaacteleop_examples— the conversion itself.live_deviceio.pyimported its sibling as a top-level module, which resolved only because the invoked script's directory landed onsys.path; that breaks underpython -m, breaks when the files are copied into another project — what the Physical AI Developer team hit — and squats the bare namedeviceio_viser. Plus the grid fix: it lay in viser's default XY plane, which stands up as a wall underset_up_direction("+y").Still open from review:
install_python_example()does not install the example-rootREADME.md, which #1044 / #1036 / #1046 need. Happy to add theinstall(FILES ... OPTIONAL)to commit 1 — say the word.Type of change
Testing
x86_64/ Ubuntu, Python 3.12.uv build --wheel ./examples/deviceio_live_viewshipsisaacteleop_examples/deviceio_live_view/{__init__,__main__,deviceio_viser,live_deviceio}.pyand nothing else — no namespace__init__.py, no bare top-level name.SKIP=check-copyright-year pre-commit runclean on every touched file; all nine example scripts byte-compile.The macro body is byte-identical to the previously approved
a916fca56(that diff is comments only), so @ivany-nv's install-tree verification still applies. Not re-run: the full CMake install tree, and a live headset run.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCONo new tests: this is a move plus a default flip, and neither example has automated coverage today (
TODO(#880)). Verification is the wheel-layout check above plus a human run on a headset.