Repository navigation
Add dVRK needle-pass environment to contrib - #6570
chrisvoncsefalvay wants to merge 8 commits into
Conversation
Move runtime imports behind lazy configuration strings and preserve inherited IK behaviour without CUDA synchronisation. Validate physical inputs and make invalid pose and jaw commands fail safe. Cache phase constants and cover the new action semantics and pre-application configuration loading.
Make the pinned PR 769 integration job blocking and let the source-build helper select supported active Python environments safely. Accept the unreleased 1.4 wheel without allowing a future major version. Execute the public retargeting graph with controller samples and require a non-empty, decodable qualification video.
github-actions
Bot
added
documentation
|
@chrisvoncsefalvay develop moved a lot; I have a rebased version at rwiltz/dvrk-needle-pass-develop. Want me to force-push it here, or would you rather rebase yourself? |
|
Also the XR camera does not seem to start at a good location, it's pretty far from the distal end of the EE. I can help fix that as well, or you can. |
|
Hi @rwiltz, 100% happy for you to force push! And of course, very happy to take your fix for the positioning! Thank you! |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (1), Scope and over-engineering (3), API (1), Compatibility and deprecation (1), Implementation (1), Style consistency (5), Test quality (4).
- PR description: The body misstates main as develop; correct it, remove the aside, list validation selectors, and add reproducible 1/32-environment step-time measurements for the 240 Hz contact path with revisions, command, GPU, backend, and warmup.
- Design and architecture: The donor-release action reads phase-machine privates and bypasses the cached contact sample, creating two owners for receiver-grasp state; expose and consume one public cached query.
- Scope and over-engineering: Minimal implementation: keep dvrk.py/dvrk_asset.py, preflight, assets, package/config initializers, ik_abs_env_cfg.py, needle_pass_env_cfg.py, spawners.py, and MDP init/stub, actions/config, events, observations, rewards, and terminations; delete the PR769 installer and grasp_solver.py, inlining retention load and moving provenance to documentation. Suggested PR breakdown: (1) dVRK asset; (2) needle-pass environment and CUDA tests; (3) released IsaacTeleop integration, docs, and focused CI.
- API: The exported reset function accepts None despite a non-optional env_ids annotation, breaking documented static usage; change it to torch.Tensor | None.
- Compatibility and deprecation: Breaking changes: parse_env_cfg now reserves any truthy requires_cuda attribute without a declared base contract or deprecation; enforce CUDA support inside the task runtime instead.
- Implementation: The 240 Hz contact path allocates and repeats four device normal tensors per sample; construct one device buffer during setup and expand or index it each step.
- Style consistency: HandoffPhaseCfg violates the required config/implementation split by living in terminations.py; move it to a dedicated configuration module and update lazy exports.
- Test quality: The suite duplicates stronger repository and runtime owner-boundary tests with literal mirrors, mocks, and an env-var-only probe; remove those cases while retaining distinct CUDA transfer coverage.
Significant concerns. Posted 16 inline findings.
Automated review; human maintainers own approval decisions.
| if [ ! -d \"\${TEST_WHEELHOUSE_PATH:-}\" ]; then | ||
| echo \"Wheelhouse path is missing: \${TEST_WHEELHOUSE_PATH:-}\" | ||
| exit 1 | ||
| if [ \"\${TEST_INSTALL_ISAACTELEOP_PR769:-}\" = \"true\" ]; then |
There was a problem hiding this comment.
🟡 Warning · Scope Complexity — Task-specific root build in shared CI action
The shared test action now root-builds an unmerged upstream PR (apt-get, git fetch, setpriv) for every package test job. Remove the flag, helper script, and dedicated job; land the task after isaacteleop releases the dVRK…
| dVRK needle-pass demonstration | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| The bimanual dVRK needle-pass task uses paired motion controllers through the unified Isaac |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Duplicated teleoperation instructions
This section repeats the preflight, launch command, and task invariants already added to cloudxr_teleoperation.rst, creating two maintenance owners. Link to that procedure and keep only imitation-specific steps.
| from runpy import run_path | ||
|
|
||
| REPOSITORY_ROOT = Path(__file__).resolve().parents[2] | ||
| needle_assets = run_path(REPOSITORY_ROOT / "source/isaaclab_tasks/isaaclab_tasks/contrib/needle_pass/assets.py") |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — runpy bypasses normal package imports
Both modules import only stdlib, so direct imports work; run_path re-executes them outside the package and hides renames from linters and type checkers. Import the symbols normally.
| # conversion above. The donor was selected only after a fixed native candidate | ||
| # was physically closed, settled with gravity enabled, and retained through a | ||
| # fixed tool disturbance. | ||
| ISAAC_GRASP_GENERATOR_EXTENSION = "isaacsim.replicator.grasping" |
There was a problem hiding this comment.
🟡 Warning · Scope Complexity — Provenance constants exported but unused
The ISAAC_GRASP_* hashes, candidate indices, contact points, and native seed poses are read by no environment code; tests only re-assert them against literals. Move this provenance into the module docstring and shrink __all__.
| def apply_actions(self) -> None: | ||
| # Import locally because this module is loaded before ``terminations`` | ||
| # by the public MDP namespace. | ||
| from .terminations import HandoffPhase, get_handoff_phase_machine, jaw_needle_contact_measurements |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Unjustified function-level import
terminations imports nothing from actions, so the cited load-order cycle does not exist and the guide's in-function import exemption does not apply. Move this import to module scope.
|
|
||
| # Some contact-rich tasks are qualified only against CUDA PhysX. Reject an | ||
| # unsupported override instead of silently changing their physical contract. | ||
| if getattr(cfg, "requires_cuda", False) and not device.startswith("cuda"): |
There was a problem hiding this comment.
🟡 Warning · Compatibility — Undeclared requires_cuda gate in shared parser
requires_cuda is duck-typed via getattr on no declared base field, so it is undiscoverable, can reject existing configs that happen to expose the name, and is bypassed by direct gym.make(cfg=...). Enforce it in the task's…
| TASK_ID = "IsaacContrib-NeedlePass-dVRK-IK-Abs" | ||
|
|
||
|
|
||
| def test_config_load_is_lazy_before_simulation_app_startup(): |
There was a problem hiding this comment.
🟡 Warning · Test Quality — Duplicates existing forbidden-import owner
source/isaaclab_tasks/test/core/test_env_cfg_no_forbidden_imports.py already owns the config-constructs-without-pxr/omni/carb/isaacsim contract repo-wide for registered tasks. Drop this per-task subprocess replay.
| return transform | ||
|
|
||
|
|
||
| def test_native_isaac_grasp_generator_provenance_and_held_reset(): |
There was a problem hiding this comment.
🟡 Warning · Test Quality — Constant mirrors assert no behavior
This case, test_asset_pins_and_explicit_needle_physics, and test_configured_device_homes_are_finite_and_distinct restate config literals and production formulas copied from the modules under test. Keep only the config-wiring assertions.
| assert machine._receiver_bounds(measurements).tolist() == [False] | ||
|
|
||
|
|
||
| def test_reset_write_order_and_single_needle_state_write(): |
There was a problem hiding this comment.
🟡 Warning · Test Quality — Mocked reset replay is weaker duplicate
_DirectStateWriteAudit already enforces the exact reset write sequence against the real environment, while these mocks supply every asserted call. Delete the mocked unit replay.
|
|
||
|
|
||
| @pytest.mark.isaacsim_ci | ||
| def test_native_receiver_candidates_complete_a_cuda_guarded_transfer(): |
There was a problem hiding this comment.
🟡 Warning · Test Quality — Opt-in candidate probe is not a test
This case skips unless two env vars supply an out-of-repo CSV, prints a report, recomputes the phase machine's retained-lift logic, and passes if any row qualifies. It never runs in CI and cannot fail for…
 [Medium impact] The PR adds one surgical-manipulation task and changes shared task-loading and CI paths. Fix the success check before merging so recordings cannot pass after the receiver loses the needle. Findings
SummaryAdds
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
XR[Paired XR controllers] --> RT[Pose and jaw retargeters]
RT --> A[18-value action]
A --> IK[World-frame IK]
A --> J[Paired jaw commands]
C[Measured needle contacts] --> G[Donor release guard]
G --> J
IK --> P[PhysX simulation]
J --> P
P --> C
C --> H[Hand-off phase machine]
P --> H
H --> O[Observations and rewards]
H --> S[Success check]
S --> D[Demonstration recorder]
Reviews (1) · Last reviewed commit: "Merge branch 'develop' into feat/dvrk-ne..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| lifted = measurements.needle_pose_w[:, 2] - self.reset_needle_z_w >= self.cfg.required_lift_delta_z_m | ||
| retained_lift_condition = receiver_only_condition & lifted | ||
| self._count_consecutive(self._retained_lift_counter, retained_lift_condition, receiver_phase) | ||
| lift_complete = receiver_phase & (self._retained_lift_counter >= self._retained_lift_required_steps) | ||
| self.phase[lift_complete] = int(HandoffPhase.RETAINED_LIFT) | ||
| return self.phase |
There was a problem hiding this comment.
RETAINED_LIFT never clears when the receiver loses the needle. In the documented recording flow, record_demos.py removes the success termination and waits for 10 consecutive successful steps. Once this phase is reached, success() keeps returning true even if receiver contact disappears or the needle falls below the required lift height. The separate drop termination requires a much larger fall, so the recorder can save a failed grasp as successful. Replay can also pass based on an earlier lift.
Recheck the retained-lift conditions after success and clear the phase and dwell counter when they fail. Add a test that loses receiver contact during the recorder's confirmation window.
Description
This draft PR replaces #6527, which was closed rather than rewriting its
main-based history. It is opened againstdevelopin response to @kellyguo11's maintainer feedback, which asked that the contribution move todevelopand that the task use the newisaaclab_tasks/contribarea.Part of the dVRK Extended Cinematic Universe: best read alongside NVIDIA/IsaacTeleop#769.
This adds a contact-driven dVRK PSM needle-pass environment, its pinned PSM asset configuration, and paired-controller XR teleoperation. The needle is initialised once during reset; runtime transfer is qualified from measured contacts and load without attaching or directly repositioning the needle.
The task now lives under
isaaclab_tasks.contrib.needle_passand is registered asIsaacContrib-NeedlePass-dVRK-IK-Abs. Thedevelopport also adds lazy runtime configuration imports, finite-input and controller-contract validation, fail-safe jaw and pose hold behaviour, and public teleoperation-pipeline coverage.Main changes
IsaacContrib-NeedlePass-dVRK-IK-Abs, with differential IK, contact observations, load-backed hand-off phases, rewards, termination criteria, and CUDA physics validation.isaaclab_assetsandisaaclab_tasks.Dependency status
NVIDIA/IsaacTeleop#769 is still open, and its dVRK retargeter API is not yet part of a release. The normal package dependency accepts
isaacteleop>=1.3.0,<2.0.0. Until a release in that range contains the dVRK API, the focused CI job and documented local-validation helper build the exact PR revisionca175df7afc8198cbba0592cd1b447b11a4f3165. The source-build helper can be removed after that API is released.Validation
./isaaclab.sh -f: all 22 pre-commit hooks passed../isaaclab.sh -p tools/changelog/cli.py check develop: both modified packages have valid fragments.Type of change
Screenshots
Checklist
./isaaclab.sh -f.source/<pkg>/changelog.d/for every touched package.CONTRIBUTORS.md.Big thanks to @kellyguo11 for the encouragement and advice on repo structure!