Update the central pyproject, remove subpackage specific dependencies#6009
Update the central pyproject, remove subpackage specific dependencies#6009StafaH wants to merge 2 commits into
Conversation
StafaH
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai and
kellyguo11
as code owners
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR centralizes all Isaac Lab third-party dependency declarations into the root pyproject.toml, removing per-subpackage dependency specifications. The approach establishes a single source of truth that the wheel builder, ./isaaclab.sh -i CLI, and uv workspace all consume. The architectural direction is sound and will significantly reduce maintenance burden for version bumps across the monorepo.
🔴 Critical: Wheel Extras Resolution Failure (CI Blocked)
The Build Wheel CI check fails because isaaclab[isaacsim,all] cannot co-resolve:
isaacsim-core==6.0.0.0 depends on imgui-bundle==1.92.4
isaaclab[all] depends on imgui-bundle>=1.92.5
isaacsim-kernel==6.0.0.1 depends on typing-extensions==4.12.2
isaaclab[all] depends on typing-extensions>=4.15.0
The newton extra (which feeds into all) raises imgui-bundle>=1.92.5 and typing-extensions>=4.15.0, but Isaac Sim pins these lower. In the old structure this was avoided because the newton viewer deps were only installed via isaaclab_visualizers[newton] which was not part of the wheel's all extra when isaacsim was also active.
Suggested fix: Either (a) move imgui-bundle and typing-extensions out of the all aggregate (like viser is excluded in the old python_packages.toml), or (b) relax the lower bounds to accommodate Isaac Sim's pins (e.g., imgui-bundle>=1.92.4), or (c) declare an explicit conflicts for [isaacsim] vs [newton] in the wheel metadata.
🟡 Medium: OPTIONAL_SUBMODULE_ROOT_EXTRAS Declares mimic → ("mimic", "teleop") Despite Conflict
In install.py, the mapping:
OPTIONAL_SUBMODULE_ROOT_EXTRAS: dict[str, tuple[str, ...]] = {
"mimic": ("mimic", "teleop"),
"teleop": ("teleop",),
}But the root pyproject.toml declares:
conflicts = [
[{ extra = "teleop" }, { extra = "mimic" }],
...
]These two extras cannot co-resolve (lxml version conflict). The _install_centralized_dependencies path will attempt to pip install both the mimic and teleop extras' requirements sequentially — pip doesn't enforce uv-style conflict declarations, so this may silently install incompatible lxml versions. Consider either:
- Removing
"teleop"from mimic's root extras mapping, or - Adding a runtime warning/skip when both are requested, matching the conflict declaration's intent.
🟡 Medium: packaging Version Pin Relaxed
The isaaclab_rl package previously pinned packaging<24 (required by certain moviepy versions). The root pyproject.toml now lists "packaging" without an upper bound. If downstream users have moviepy versions that rely on packaging<24 behavior, this could cause subtle runtime issues with video recording utilities.
🟢 Minor: all Extra Duplicates Individual Extras Inline
The all extra lists every dependency inline rather than referencing the other extras. This means version bumps must be synchronized in two places (the individual extra AND all). Consider whether the wheel builder or uv supports extra self-references (e.g., {include-group = "sb3"}) to keep them DRY, or add a test that verifies all is a superset of the union of the named extras.
🟢 Minor: Documentation Hardcodes Dependency Lists
The docs (e.g., bleeding-edge.rst, rlinf_vla_posttraining.rst) now inline the full pip install command with specific versions:
pip install "ray[default]>=2.47.0" "av>=12.3.0" ...These will drift from the root pyproject.toml over time. Consider referencing the extra instead (e.g., pip install isaaclab[rlinf] for wheel users, or uv run --extra rlinf for source users) to maintain the single-source-of-truth principle this PR establishes.
🟢 Positive Observations
- The
[tool.isaaclab.wheel-extras]pattern forisaacsimis a clean solution for keeping conflicting extras out of workspace resolution while still shipping them in the wheel. - The
constraint-dependenciesfor the torch stack (pinning exact versions for dev while keeping wheel specs loose) is well-designed. - The conflict declarations for teleop/mimic/all are appropriate given the lxml incompatibility.
- Test coverage updates are thorough — the new
test_wheel_builder_metadata.pyintegration test that actually runsgen_pyproject.pyis a strong improvement over the old static TOML checks. - Changelog entries and
.skipmarkers are correctly structured.
Update (d9f9172): New commits add docs infrastructure improvements (switch to uv in docs CI, parallel sphinx builds with -j auto, and sphinx-paramlinks extension). No concerns — these are clean, self-contained changes that don't affect the dependency centralization logic. Previous review points remain unchanged.
There was a problem hiding this comment.
🔍 Code Review: Dependency Centralization
Thanks for this significant refactoring effort to consolidate dependencies into the root pyproject.toml. This is a valuable architectural improvement that will make dependency management much easier to maintain. Here's my analysis:
✅ Strengths
-
Single Source of Truth: Moving all dependencies to the root
pyproject.tomleliminates the scattered dependency declarations across ~15 sub-packages, making it much easier to audit and update versions. -
Well-Documented Changes: The changelog entries clearly explain the behavioral change (Newton is now a core dependency) and the architectural shift.
-
Comprehensive Test Updates: The tests have been properly updated to reflect the new structure, particularly in
test_uv_run_pyproject.pyandtest_wheel_builder_metadata.py. -
Conflict Resolution: Good use of
[tool.uv]conflictsto handle the teleop/mimic lxml version conflict gracefully. -
Wheel Builder Simplification: Removing
res/python_packages.tomland havinggen_pyproject.pyread directly from the root pyproject reduces duplication and potential for drift.
⚠️ Concerns & Suggestions
1. Pinning Strategy Inconsistency
The root pyproject mixes loose (>=) and exact (==) version specifiers without clear rationale:
"torch>=2.10", # loose
"pillow==12.1.1", # exact
"transformers==4.57.6", # exact
"matplotlib>=3.10.3", # looseSuggestion: Consider documenting why certain packages are pinned exactly (Isaac Sim compatibility?) vs. loosely. This helps future maintainers understand when they can update vs. when caution is needed.
2. [tool.uv.constraint-dependencies] Duplication Risk
[tool.uv]
constraint-dependencies = [
"torch==2.10.0",
"torchvision==0.25.0",
"torchaudio==2.10.0",
]While the comment explains the reasoning, there's now a disconnect: [project.dependencies] has torch>=2.10 but constraints pin ==2.10.0. If someone reads just the dependencies section, they might not realize the workspace resolution will differ from the published wheel resolution.
Suggestion: Add a comment in the [project.dependencies] section pointing to the constraint-dependencies section.
3. all Extra Expansion
The all extra now explicitly lists every dependency rather than referencing other extras:
all = [
"stable-baselines3>=2.6",
"tqdm",
"rich",
# ... many more
]This creates maintenance burden – if you update sb3 extra, you must also update all. The previous approach of referencing extras was DRYer.
Suggestion: Consider whether all could reference other extras, or add a test that validates all is a superset of all other extras.
4. Install CLI Changes Need Integration Testing
The changes to install.py are substantial:
- New
_root_core_dependencies()and_root_extra_dependencies()functions _install_centralized_dependencies()replacing per-package dependency resolution- Mapping changes (
ov[ovrtx]→rtxextra, etc.)
Question: Have these paths been tested with the actual ./isaaclab.sh -i command? Unit tests mock out the pip calls, so integration behavior could differ.
5. Documentation Updates May Be Incomplete
The docs changes show users should now run:
pip install "ray[default]>=2.47.0" "av>=12.3.0" ...instead of the simpler:
uv pip install -e "source/isaaclab_contrib[rlinf]"This is a UX regression for contrib users – they now need to know individual packages.
Question: Should there be a simpler command like ./isaaclab.sh -i rlinf to install these dependencies?
6. Missing h5py in Root Core Dependencies
isaaclab_mimic previously depended on h5py>=3.15.0 for data collection. I see h5py is now only in the RL utilities section, but is it still available for mimic workflows?
🔬 Minor Nits
-
Line 88 in
pyproject.toml: Comment says "RL utilities" butpackagingis listed there – ispackagingan RL utility? -
_TORCH_DISTRIBUTIONSset in install.py: Consider making this a frozenset for immutability. -
Changelog file naming:
centralize-deps-root-pyproject.rstis quite long – consider shorter names for future changelogs.
📋 Checklist Observations
Based on the PR checklist:
- Pre-commit checks: Not visible in the diff, please confirm
./isaaclab.sh --formatpasses - Tests: Unit tests updated ✅, but consider integration test coverage
- Changelog: Present ✅
-
extension.tomlversion: Not applicable (this is pyproject.toml changes)
Summary
This is a well-executed refactoring with clear benefits for maintainability. The main suggestions are around documenting pinning rationale, ensuring the all extra stays in sync, and verifying integration test coverage for the install CLI changes.
Would appreciate clarification on the integration testing and the contrib/rlinf UX change before merging.
Greptile SummaryThis PR consolidates all Isaac Lab third-party dependencies into the root
Confidence Score: 4/5The change is a large but mechanical reorganization: sub-packages lose their dep declarations and the root pyproject becomes the single authority. The logic is straightforward and well-tested; no runtime behavior changes outside the install CLI and wheel builder. The rework is sound and the new helpers mirror the old editable-install approach closely. The main open question is the silent removal of the packaging<24 upper bound that isaaclab_rl previously enforced — if any code path relies on pre-v24 packaging APIs, this would fail at runtime rather than install time. The pip-vs-uv-pip inconsistency in the docs is minor but could trip up users following the rlinf setup guide. The root pyproject.toml (specifically the packaging entry and the new all extra composition) and docs/source/experimental-features/bleeding-edge.rst (inconsistent pip vs uv pip install command) deserve a second look before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["root pyproject.toml\n(single source of truth)"]
subgraph DevWorkspace ["uv Dev Workspace"]
A -->|"core deps + constraint-dependencies\n(torch pinned)"| B["uv lock / uv sync"]
A -->|"optional-dependencies extras"| B
A -->|"[tool.uv.sources] editable refs"| C["isaaclab* sub-packages\n(empty dependencies=[])"]
end
subgraph CLI ["./isaaclab.sh -i"]
D["command_install()"] -->|"_install_centralized_dependencies()"| E["_root_core_dependencies()\nreads root pyproject, strips torch + isaaclab*"]
D -->|"optional submodule extras"| F["_install_root_extra(extra)\nreads optional-dependencies[extra]"]
D -->|"_install_extra_feature()"| F
end
subgraph WheelBuilder ["tools/wheel_builder"]
G["build.sh"] -->|"gen_pyproject.py root/pyproject.toml"| H["gen_pyproject.py\nstrips isaaclab* workspace members\nmerges tool.isaaclab.wheel-extras"]
H --> I["generated wheel pyproject.toml\n(isaacsim extra from wheel-extras)"]
end
A -->|"_load_root_pyproject()"| CLI
A -->|"tomllib.load()"| WheelBuilder
Reviews (1): Last reviewed commit: "Move pyproject dependencies to root" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # The base contrib package is installed with the core Isaac Lab packages. | ||
| # Install its optional dependencies (e.g., for RLinf VLA post-training) with pip: | ||
| pip install "ray[default]>=2.47.0" "av>=12.3.0" "numpydantic>=1.7.0" \ | ||
| "albumentations>=1.4.18" decord2 "dm_tree>=0.1.8" "diffusers>=0.35.0" \ | ||
| "timm>=1.0.14" "peft>=0.17.0" pandas |
There was a problem hiding this comment.
pip install while the companion guide rlinf_vla_posttraining.rst uses uv pip install for the identical rlinf dependency list. In an Isaac Lab checkout where uv manages the environment, bare pip may resolve to the system Python rather than the active virtual environment, silently installing into the wrong interpreter. Both files should use the same tool.
| # The base contrib package is installed with the core Isaac Lab packages. | |
| # Install its optional dependencies (e.g., for RLinf VLA post-training) with pip: | |
| pip install "ray[default]>=2.47.0" "av>=12.3.0" "numpydantic>=1.7.0" \ | |
| "albumentations>=1.4.18" decord2 "dm_tree>=0.1.8" "diffusers>=0.35.0" \ | |
| "timm>=1.0.14" "peft>=0.17.0" pandas | |
| # The base contrib package is installed with the core Isaac Lab packages. | |
| # Install its optional dependencies (e.g., for RLinf VLA post-training) with uv pip: | |
| uv pip install "ray[default]>=2.47.0" "av>=12.3.0" "numpydantic>=1.7.0" \ | |
| "albumentations>=1.4.18" decord2 "dm_tree>=0.1.8" "diffusers>=0.35.0" \ | |
| "timm>=1.0.14" "peft>=0.17.0" pandas |
| "rl-games @ git+https://github.com/isaac-sim/rl_games.git@python3.11", | ||
| "gym", | ||
| "standard-distutils", | ||
| ] |
There was a problem hiding this comment.
packaging<24 upper-bound
source/isaaclab_rl/pyproject.toml previously pinned packaging<24 — likely because packaging v24 dropped several deprecated APIs (e.g. LegacyVersion, parse_version returning a LegacyVersion for non-PEP-440 strings). The new root entry is simply "packaging" with no upper bound. If anything in isaaclab_rl or downstream code exercises those removed APIs, the install will succeed silently but fail at runtime. It is worth confirming no code path touches packaging.version.LegacyVersion or similar symbols before dropping this constraint.
|
Yes let's do this :D |
2 participants
Description
Consolidates all dependencies into the central pyproject. This should make it easier to maintain a ground truth.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there