Skip to content

[Warp] Read parity fixture poses through the warp accessor - #7060

Merged
kellyguo11 merged 2 commits into
isaac-sim:developfrom
hujc7:jichuanh/warp-mdp-review-followup
Sep 3, 2026
Merged

[Warp] Read parity fixture poses through the warp accessor#7060
kellyguo11 merged 2 commits into
isaac-sim:developfrom
hujc7:jichuanh/warp-mdp-review-followup

Conversation

@hujc7

@hujc7 hujc7 commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Addresses review comments on #6900.

  • The pose-command parity fixture reached numpy via .torch.cpu().numpy(), routing warp buffers through a torch view and a host copy to get data warp hands back directly. Now uses .warp.numpy().
  • UniformPoseCommand imports moved to the top of the file; they were local without a cycle to justify it.

ProxyArray.numpy() is not equivalent and was tried first: unknown attributes forward to the torch view, so it inherits torch's CPU-only restriction and raises on the cuda tensors these fixtures use.

Not included, raised on #6900 and better handled separately:

  • Dropping the _wp suffix. term_dones_wp belongs to a family of six (time_outs_wp, dones_wp, terminated_wp, _truncated_wp, _terminated_wp, _scratch_term_mask_wp); renaming one leaves the package less consistent than it is now. A rename of the family, with deprecations, is its own change.
  • wp.static for the resolved threshold flags, and 2D/tiled rewrites of the reward kernels. Both are optimizations that change how the kernels are defined and warrant their own benchmarking.

Release backport

  • Backport this pull request to the active release branch after it merges into develop

Test plan:

  • pytest source/isaaclab_experimental/test/ — 205 passed, 1 skipped
  • uv run isaaclab -f clean

The pose-command fixture reached numpy via .torch.cpu().numpy(), routing warp
buffers through a torch view and a host copy to get data warp can hand back
directly. .warp.numpy() is the accessor for that.

Note that ProxyArray.numpy() is not equivalent: unknown attributes forward to
the torch view, so it inherits torch's CPU-only restriction and raises on the
cuda tensors these fixtures use.

The UniformPoseCommand imports move to the top of the file; they were local
without a cycle to justify it.
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Aug 13, 2026
Reviewers have twice read the checked-in set as redundant with the sweep that
produces it. The sweep reports what adapts today; the set records what must keep
adapting, so a task losing support fails instead of quietly shrinking the
computed answer. The comment stated what the set is and how to update it, but
not that, and the rationale sat forty lines away in the module docstring.
@hujc7
hujc7 marked this pull request as ready for review September 3, 2026 00:28
@hujc7
hujc7 requested a review from a team September 3, 2026 00:28

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

This test-only follow-up changes the pose-command parity fixture to materialize NumPy arrays directly from Warp buffers, moves command imports to module scope, clarifies the purpose of the checked-in Warp task-support set, and adds the required skip changelog fragment.

  • Design and architecture: Using the Warp accessor avoids routing Warp-owned buffers through an intermediate Torch view while retaining the fixture’s NumPy reference calculations. The module-scope imports are consistent with ordinary dependency usage in this helper.
  • API: No public symbols, configuration fields, defaults, exports, or documented runtime behavior change. The helper continues producing a UniformPoseCommand with the same command-buffer shape and device behavior for existing test consumers.
  • Implementation: The four accessor substitutions preserve the array dimensions used by body indexing and the NumPy quaternion and position calculations; CUDA data is still materialized on the host as required by NumPy, but without the intermediate Torch conversion path. No production implementation path is modified.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

@greptile-apps

greptile-apps Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates pose-command parity fixtures to read CUDA-backed pose data directly through the Warp accessor and moves pose-command imports to module scope.

  • Replaces Torch-view CPU conversions with direct wp.array.numpy() reads.
  • Documents why the checked-in Warp-supported task set is retained as a regression pin.
  • Adds an empty changelog skip marker for the review follow-up.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable defects identified in the changed test infrastructure.

The direct Warp accessor returns the expected host-side pose shapes for the synchronized fixture buffers, and the import and documentation changes preserve existing test behavior.

Important Files Changed

Filename Overview
source/isaaclab_experimental/test/envs/mdp/parity_helpers.py Direct Warp-to-NumPy fixture reads preserve the expected pose shapes and synchronization behavior in the current callers; the import relocation revealed no concrete collection failure.
source/isaaclab_experimental/test/envs/test_frontend_cfg_conversion.py Adds explanatory comments clarifying that the supported-task set guards against unnoticed Warp-support regressions.

Reviews (1): Last reviewed commit: "Say why the warp-supported set is pinned..." | Re-trigger Greptile

@kellyguo11
kellyguo11 merged commit 318abfd into isaac-sim:develop Sep 3, 2026
49 of 50 checks passed
@isaaclab-bot

isaaclab-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Backported to release/3.0.0 as 1344cfb.

isaaclab-bot Bot pushed a commit that referenced this pull request Sep 3, 2026
Addresses review comments on #6900.

- The pose-command parity fixture reached numpy via
`.torch.cpu().numpy()`, routing warp buffers through a torch view and a
host copy to get data warp hands back directly. Now uses
`.warp.numpy()`.
- `UniformPoseCommand` imports moved to the top of the file; they were
local without a cycle to justify it.

`ProxyArray.numpy()` is not equivalent and was tried first: unknown
attributes forward to the torch view, so it inherits torch's CPU-only
restriction and raises on the cuda tensors these fixtures use.

Not included, raised on #6900 and better handled separately:

- Dropping the `_wp` suffix. `term_dones_wp` belongs to a family of six
(`time_outs_wp`, `dones_wp`, `terminated_wp`, `_truncated_wp`,
`_terminated_wp`, `_scratch_term_mask_wp`); renaming one leaves the
package less consistent than it is now. A rename of the family, with
deprecations, is its own change.
- `wp.static` for the resolved threshold flags, and 2D/tiled rewrites of
the reward kernels. Both are optimizations that change how the kernels
are defined and warrant their own benchmarking.

## Release backport

- [x] <!-- backport-active-release --> Backport this pull request to the
active release branch after it merges into `develop`

Test plan:

- [x] `pytest source/isaaclab_experimental/test/` — 205 passed, 1
skipped
- [x] `uv run isaaclab -f` clean

(cherry picked from commit 318abfd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants