Skip to content

[testing, do not merge] add pin package to dep for CI testing#5234

Closed
peterd-NV wants to merge 2 commits intoisaac-sim:developfrom
peterd-NV:peterd/fix_pinocchio
Closed

[testing, do not merge] add pin package to dep for CI testing#5234
peterd-NV wants to merge 2 commits intoisaac-sim:developfrom
peterd-NV:peterd/fix_pinocchio

Conversation

@peterd-NV
Copy link
Copy Markdown
Contributor

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Apr 10, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Thanks for the PR! Adding pin (Pinocchio) as a dependency for testing makes sense — the codebase has ~19 files that import pinocchio. A few issues to address:

1. Missing version pin

The existing pin-pink==3.1.0 dependency already requires pin>=2.6.3, so Pinocchio is already pulled in on Linux. Adding "pin" without a version constraint means any future breaking release could silently break things. At minimum, pin it to a compatible range:

"pin>=2.6.3",

or match whatever version the test suite is validated against.

2. Missing platform markers

The pin PyPI package has no Windows wheels (only Linux and macOS). Adding it as an unconditional dependency will break pip install isaaclab on Windows. It should have a platform marker, similar to how pin-pink is gated:

f"pin>=2.6.3 ; platform_system != 'Windows'",

or at minimum:

f"pin>=2.6.3 ; platform_system == 'Linux' and ({SUPPORTED_ARCHS_ARM})",

to match the existing pin-pink scope.

3. Placement in the dependency list

The line is added in the general INSTALL_REQUIRES list, but this is described as being "for testing." If it's only needed for tests, consider whether it belongs in an extras_require test group instead. If it's needed at runtime (not just tests), the PR title/description should be updated to reflect that.

4. PR description is empty

The description is still the default template with no actual content filled in. Please describe:

  • What test/scenario was failing without this dependency?
  • Why pin-pink's transitive dependency on pin isn't sufficient (is this for scenarios that need Pinocchio without Pink?).

None of the checklist items are checked either.

Comment thread source/isaaclab/setup.py Outdated
# Required by pydantic-core/imgui_bundle on Python 3.12 (Sentinel symbol).
"typing_extensions>=4.14.0",
"lazy_loader>=0.4",
"pin",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Three issues with this line:

  1. No version constraint — should at minimum be "pin>=2.6.3" to match what pin-pink already requires.
  2. No platform markerpin has no Windows wheels on PyPI. This will break installation on Windows. Should be gated with at least platform_system != 'Windows'.
  3. Possibly redundant on Linuxpin-pink==3.1.0 (line 68) already depends on pin>=2.6.3, so on Linux+supported arches Pinocchio is already installed. Is this intended to cover macOS or non-pin-pink test scenarios?

Suggested fix:

f"pin>=2.6.3 ; platform_system != 'Windows'",

@peterd-NV peterd-NV changed the title add pin package to dep for testing [testing, do not merge] add pin package to dep for testing Apr 10, 2026
@peterd-NV peterd-NV changed the title [testing, do not merge] add pin package to dep for testing [testing, do not merge] add pin package to dep for CI testing Apr 10, 2026
@peterd-NV peterd-NV closed this Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant