Bring in Sharpa retargeter from V2D#444
Conversation
📝 WalkthroughWalkthroughThis pull request introduces infrastructure for optional bundling of V2D's Sequence DiagramsequenceDiagram
actor GHA as GitHub Actions<br/>(build-ubuntu.yml)
participant ACT as setup-v2d-src<br/>Action
participant REPO as jiwenc-nv/v2d<br/>Repository
participant CACHE as GitHub<br/>Cache
participant CMAKE as CMake<br/>Build System
participant WHEEL as Wheel<br/>Builder
participant STRIP as strip_robotic<br/>_grounding.py
GHA->>ACT: Trigger (non-release branch)
ACT->>ACT: Read deps/v2d/version.txt
alt Cache Hit
ACT->>CACHE: Restore by SHA key
CACHE-->>ACT: robotic_grounding/src
else Cache Miss
ACT->>REPO: Clone retargeter branch
REPO-->>ACT: V2D repository
ACT->>ACT: Checkout pinned SHA
ACT->>ACT: Copy robotic_grounding/source<br/>into deps/v2d/src/
ACT->>CACHE: Save by SHA key
end
ACT->>GHA: Output bundled=true/false
GHA->>CMAKE: Configure with<br/>BUNDLE_ROBOTIC_GROUNDING=TRUE
CMAKE->>CMAKE: Verify __init__.py exists
CMAKE->>WHEEL: Build wheel with<br/>robotic_grounding staged
alt Release Build
WHEEL-->>GHA: Release wheel artifact
GHA->>STRIP: Post-build strip phase
STRIP->>STRIP: Remove robotic_grounding/<br/>entries, rewrite RECORD
STRIP-->>GHA: Cleaned wheel
else Non-Release Build
WHEEL-->>GHA: Wheel (with grounding)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Critical observationsWheel integrity & RECORD handling — The
CMake conditional logic — When
Version pinning & CI gating — The setup action only fetches when
Warm-start state lifecycle —
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
|
/preview-docs |
|
❌ No successful |
|
/preview-docs |
|
✅ Preview deployed: https://NVIDIA.github.io/IsaacTeleop/preview/pr-444/ |
jiwenc-nv
force-pushed
the
sharpa_with_v2d
branch
2 times, most recently
from
eb8baa3 to
a4d9272
Compare
jiwenc-nv
changed the title
Note: V2D is still pre-release and gated from public access. The fetch is gated on V2D_RETARGETER_TOKEN and skipped on release branches.
Wraps robotic_grounding.retarget.SharpaHandKinematics with Teleop's BaseRetargeter contract -- OpenXR -> MANO joint mapping, warm-started qpos, output indexing. Lazy-imports cleanly: installs without the [grounding] extra raise a directed ModuleNotFoundError instead of breaking module load. Co-Authored-By: rwiltz <165190220+rwiltz@users.noreply.github.com>
Promotes references/retargeting.rst into a directory and adds sharpa.rst covering [grounding] build, Python usage, the demo, and ctest validation.
![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 the current code and only fix it if needed.
Inline comments:
In `@scripts/strip_robotic_grounding_from_wheel.py`:
- Around line 91-93: The script currently logs and skips when a provided wheel
path fails the path.is_file() check (the block using "if not path.is_file(): ...
continue") which allows the script to later return success; change this so
invalid input paths cause a non-zero exit instead of continuing: either call
sys.exit(1) (or raise SystemExit(1)) immediately in that if-block, or set an
error flag when encountering any invalid path and after the loop call
sys.exit(1) if the flag is set, ensuring the script does not return success at
the final return on successful completion.
In `@src/core/retargeting_engine_tests/python/pyproject.toml`:
- Around line 24-35: The grounding extra lists "loop-rate-limiters" and "daqp"
without version constraints which can pull incompatible older releases; update
the grounding array in pyproject.toml to add minimum version specifiers that
match src/core/python/requirements-grounding.txt (or the tested minimums), e.g.
add "loop-rate-limiters>=<min_version>" and "daqp>=<min_version>" so that the
grounding list (the grounding = [...] block) enforces the same minimum versions
as the requirements file.
In `@src/retargeters/sharpa_hand_retargeter.py`:
- Around line 284-290: Before mapping, detect any overlapping names between
left_joint_names and right_joint_names (e.g. overlap = set(left_joint_names) &
set(right_joint_names)); if overlap is non-empty, raise an error (ValueError or
custom) listing the offending joint names so the ambiguous names are rejected
instead of silently favoring left side. Place this check immediately before the
loop that populates _output_indices_left/_output_indices_right and
_left_indices/_right (i.e., prior to the for i, jname in
enumerate(target_joint_names) loop) so mapping only proceeds when left/right
sets are disjoint.
🪄 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: 8f79a831-aa0d-4fd5-a8e5-11722990e50f
📒 Files selected for processing (19)
.github/actions/setup-v2d-src/action.yml.github/workflows/build-ubuntu.yml.gitignoredeps/v2d/version.txtdocs/source/index.rstdocs/source/references/retargeting/index.rstdocs/source/references/retargeting/sharpa.rstexamples/retargeting/python/sharpa_hand_retargeter_demo.pyscripts/setup_v2d_src.shscripts/strip_robotic_grounding_from_wheel.pysrc/core/python/CMakeLists.txtsrc/core/python/pyproject.toml.insrc/core/python/requirements-grounding.txtsrc/core/python/requirements-retargeters.txtsrc/core/retargeting_engine_tests/python/CMakeLists.txtsrc/core/retargeting_engine_tests/python/pyproject.tomlsrc/core/retargeting_engine_tests/python/test_sharpa_hand_retargeter.pysrc/retargeters/__init__.pysrc/retargeters/sharpa_hand_retargeter.py
| if not path.is_file(): | ||
| print(f" {path}: not a file, skipping", file=sys.stderr) | ||
| continue |
There was a problem hiding this comment.
Fail closed when an input wheel path is invalid.
Line 91-Line 93 currently logs and skips invalid paths, then Line 101 still returns success. For a release-safety stripping step, this can silently bypass stripping when path resolution is wrong.
Proposed fix
def main(argv: list[str]) -> int:
@@
- any_modified = False
+ any_modified = False
+ had_error = False
for w in args.wheels:
path = Path(w)
if not path.is_file():
print(f" {path}: not a file, skipping", file=sys.stderr)
+ had_error = True
continue
if strip_wheel(path):
any_modified = True
@@
- return 0
+ return 1 if had_error else 0Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/strip_robotic_grounding_from_wheel.py` around lines 91 - 93, The
script currently logs and skips when a provided wheel path fails the
path.is_file() check (the block using "if not path.is_file(): ... continue")
which allows the script to later return success; change this so invalid input
paths cause a non-zero exit instead of continuing: either call sys.exit(1) (or
raise SystemExit(1)) immediately in that if-block, or set an error flag when
encountering any invalid path and after the loop call sys.exit(1) if the flag is
set, ensuring the script does not return success at the final return on
successful completion.
| # Mirrors src/core/python/requirements-grounding.txt. The Sharpa unit test | ||
| # imports robotic_grounding (staged into PYTHONPATH from the wheel build); | ||
| # its runtime deps come from this extra. | ||
| grounding = [ | ||
| "numpy", | ||
| "scipy", | ||
| "torch>=2.7.0", | ||
| "pin>=2.7", | ||
| "pin-pink>=4.0", | ||
| "loop-rate-limiters", | ||
| "daqp", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Canonical grounding requirements =="
cat src/core/python/requirements-grounding.txt | sed -n '1,40p'
echo
echo "== Test pyproject grounding extra =="
cat src/core/retargeting_engine_tests/python/pyproject.toml | sed -n '20,45p'
echo
echo "== Quick diff on relevant packages =="
python - <<'PY'
from pathlib import Path
req = Path("src/core/python/requirements-grounding.txt").read_text()
py = Path("src/core/retargeting_engine_tests/python/pyproject.toml").read_text()
targets = ["pin", "pin-pink", "loop-rate-limiters", "daqp"]
print("requirements-grounding.txt:")
for t in targets:
for line in req.splitlines():
if line.strip().startswith(t):
print(" ", line.strip())
print("\npyproject grounding extra:")
inside=False
for line in py.splitlines():
s=line.strip()
if s.startswith("grounding = ["):
inside=True
continue
if inside and s=="]":
break
if inside:
for t in targets:
if t in s:
print(" ", s.strip().strip('",'))
PYRepository: NVIDIA/IsaacTeleop
Length of output: 1999
Missing version constraints for loop-rate-limiters and daqp in grounding extra
The grounding extra claims to mirror src/core/python/requirements-grounding.txt, but loop-rate-limiters and daqp are listed without minimum version constraints. This allows installation of older incompatible versions, risking test failures and nondeterministic environment setup.
Fix
grounding = [
"numpy",
"scipy",
"torch>=2.7.0",
"pin>=2.7",
"pin-pink>=4.0",
- "loop-rate-limiters",
- "daqp",
+ "loop-rate-limiters>=1.0.0",
+ "daqp>=0.5.0",
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/retargeting_engine_tests/python/pyproject.toml` around lines 24 -
35, The grounding extra lists "loop-rate-limiters" and "daqp" without version
constraints which can pull incompatible older releases; update the grounding
array in pyproject.toml to add minimum version specifiers that match
src/core/python/requirements-grounding.txt (or the tested minimums), e.g. add
"loop-rate-limiters>=<min_version>" and "daqp>=<min_version>" so that the
grounding list (the grounding = [...] block) enforces the same minimum versions
as the requirements file.
| for i, jname in enumerate(target_joint_names): | ||
| if jname in left_joint_names: | ||
| self._output_indices_left.append(i) | ||
| self._left_indices.append(left_joint_names.index(jname)) | ||
| elif jname in right_joint_names: | ||
| self._output_indices_right.append(i) | ||
| self._right_indices.append(right_joint_names.index(jname)) |
There was a problem hiding this comment.
Reject overlapping left/right joint names before mapping.
Line 285-Line 290 silently prioritizes left_joint_names when a name exists in both sides, so right-hand values can be dropped without error.
Proposed fix
self._output_indices_left: list[int] = []
self._output_indices_right: list[int] = []
+ overlap = set(left_joint_names) & set(right_joint_names)
+ if overlap:
+ raise ValueError(
+ f"left_joint_names and right_joint_names overlap: {sorted(overlap)}"
+ )
+
for i, jname in enumerate(target_joint_names):
if jname in left_joint_names:
self._output_indices_left.append(i)
self._left_indices.append(left_joint_names.index(jname))
elif jname in right_joint_names:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/retargeters/sharpa_hand_retargeter.py` around lines 284 - 290, Before
mapping, detect any overlapping names between left_joint_names and
right_joint_names (e.g. overlap = set(left_joint_names) &
set(right_joint_names)); if overlap is non-empty, raise an error (ValueError or
custom) listing the offending joint names so the ambiguous names are rejected
instead of silently favoring left side. Place this check immediately before the
loop that populates _output_indices_left/_output_indices_right and
_left_indices/_right (i.e., prior to the for i, jname in
enumerate(target_joint_names) loop) so mapping only proceeds when left/right
sets are disjoint.
1 participant
Note: V2D is still pre-release and is gated from public access.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores