Fixes uv run path and aligns closer to ./isaaclab.sh -i approach#5663
Conversation
There was a problem hiding this comment.
📋 Code Review Summary
This PR fixes the uv run workflow broken by #5650 and adds comprehensive test coverage. The changes are well-structured and follow good practices.
✅ Strengths
1. pyproject.toml Changes
- Restoring modular extras (
newton,ov,rl,rtx, etc.) aligns with the documented./isaaclab.sh -imodular installation approach - Adding
python-preference = \"only-managed\"correctly prevents the uv venv from inheriting a conda Python, avoiding C++ runtime conflicts - Base dependencies now include the extras needed for the documented bare
uv run trainworkflow (isaaclab-newton[all],isaaclab-physx[newton],isaaclab-rl[rsl-rl])
2. GitHub Actions Security
- Pinning actions to SHA commits (e.g.,
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) is a security best practice that prevents supply chain attacks from tag mutations
3. Test Coverage
test_uv_run_pyproject.pyprovides excellent regression tests that verify:- Extra names match documentation
- Modular structure without Isaac Sim bundling
- Base dependencies cover training workflows
- Python preference setting
test_run_install_ci.pytests Docker result copying behavior thoroughly
4. CI Workflow Enhancement
- Adding wheel extras resolution dry-run test catches dependency resolution issues early
- Using
--dry-runavoids downloading packages while still validating resolution
📝 Minor Observations
-
Timeout increase (5 → 20 min): The build-wheel job timeout was increased significantly. This is reasonable given the added install test step, but worth monitoring if this becomes a bottleneck.
-
isaaclab-teleopremoval: Theallextra no longer includesisaaclab-teleop(previously present). If intentional, this is fine; if not, it may need to be re-added. -
isaacsimextra removed entirely: The test explicitly validates thatisaacsimis not in optional-dependencies. This is a deliberate design choice to keep Isaac Sim opt-in for wheel users.
🔍 Verdict
LGTM - This is a clean bug fix with excellent test coverage. The changes correctly restore the documented uv run workflow and add safeguards to prevent similar regressions.
Automated review by Isaac Lab Review Bot
Update (9ca8ead): New commits add RSL-RL version alignment fix:
- ✅ RSL-RL pin updated:
rsl-rl-libversion in wheel builder changed from3.1.2→5.0.1to matchisaaclab_rl/setup.py - ✅ New test added:
test_wheel_builder_metadata.pyensures the wheel builder RSL-RL pin stays in sync with the source package — good safeguard against future drift - ✅ Changelog added: Documents the fix properly
wheel.yml changed from SHA pins (@043fb46d...) back to version tags (@v7). This is less secure but more maintainable. Acceptable trade-off for non-critical CI actions.
Still LGTM ✅
Update (cb966ea): Added license exception for decorator package (BSD-2 with UNKNOWN marker) in .github/workflows/license-exceptions.json. This is a routine CI fix for license checker false positives. ✅ No concerns.
Greptile SummaryThis PR fixes the broken
Confidence Score: 4/5Safe to merge; the pyproject restructuring and CI hardening are straightforward with one design ambiguity worth resolving. The core fixes (restoring documented extras, forcing uv-managed Python, pinning action SHAs) are correct and well-tested. The main concern is that the contrib optional-dependency exactly duplicates an entry already in base dependencies, making --extra contrib a silent no-op. The version-comment mismatch between upload-artifact and download-artifact is a minor maintenance hazard. pyproject.toml — the contrib extra and the base dependencies list both declare isaaclab-contrib, which should be resolved before merging to avoid user confusion. Important Files Changed
Reviews (1): Last reviewed commit: "Fixes uv run path and aligns closer to ...." | Re-trigger Greptile |
| contrib = [ | ||
| "isaaclab-contrib", | ||
| ] |
There was a problem hiding this comment.
isaaclab-contrib is already present in the base dependencies list (line 14), so the contrib optional-dependency section adds nothing — uv run --extra contrib installs exactly the same set of packages as uv run alone. This makes the option silently useless and will confuse users who expect it to pull in something extra. Either remove contrib from the base dependencies (leaving it opt-in via the extra) or remove the contrib extra entirely.
| contrib = [ | |
| "isaaclab-contrib", | |
| ] | |
| contrib = [ | |
| # NOTE: isaaclab-contrib is already in base dependencies; | |
| # this extra is a no-op. Remove either this section or the | |
| # base dependency entry so the intent is unambiguous. | |
| "isaaclab-contrib", | |
| ] |
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 | ||
| with: | ||
| name: ${{ steps.meta.outputs.artifact_name }} | ||
| path: tools/wheel_builder/build/dist/isaaclab-*.whl | ||
| if-no-files-found: error | ||
| retention-days: 30 | ||
|
|
||
| - name: Download wheel artifact for install test | ||
| if: steps.changes.outputs.run_build == 'true' | ||
| uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 |
There was a problem hiding this comment.
upload-artifact / download-artifact version comment mismatch
actions/upload-artifact is pinned with a # v7 comment and actions/download-artifact with # v8. GitHub releases these two actions in lockstep, so matching version numbers are expected. The mismatch suggests either the download hash resolves to v7 (and the comment is wrong) or to v8 (and the upload comment is wrong). Since the pinned hash is what executes, the functional impact depends on which hash is correct, but misleading comments can cause future maintainers to chase the wrong release notes when investigating behaviour changes.
…ac-sim#5663) # Description Fixes the uv run path that got broken by isaac-sim#5650. Updates the available options to align closer with the new logic for ./isaaclab.sh -i modular installation. Also adds more unit tests for uv run and pip package. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] 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 <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
Description
Fixes the uv run path that got broken by #5650. Updates the available options to align closer with the new logic for ./isaaclab.sh -i modular installation.
Also adds more unit tests for uv run and pip package.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there