Modernize packaging and version management across sub-packages#5640
Modernize packaging and version management across sub-packages#5640diegoferigo-rai wants to merge 4 commits into
Conversation
Find the main package and all subpackages, without requiring __init__.py. This is the generic version of find_packages which requires __init__.py files.
There was a problem hiding this comment.
Code Review Summary
This PR modernizes packaging and version management across 14 sub-packages by replacing custom TOML-based version parsing with importlib.metadata.version(). The changes are well-structured and follow Python packaging best practices.
✅ Strengths
- Simplified version retrieval: Using
importlib.metadatais the recommended approach for Python 3.10+ and eliminates runtime dependencies ontoml/tomllib - Consistent fallback behavior: All packages now consistently fall back to
"0.0.0"when not installed - Cleaner
__init__.pyfiles: Removing boilerplate reduces maintenance burden - Testing dependencies separated: Moving test deps to
[testing]extra keeps the default install lighter find_namespace_packages: Proper use for flat-layout namespace packages
📝 Suggestions for Improvement
1. Dead code in isaaclab_newton/setup.py
The custom build_py class that bundles config/extension.toml into the wheel is now dead code:
class build_py(setuptools.command.build_py.build_py):
"""Custom build command that bundles config/extension.toml into the package."""
def run(self):
super().run()
src = os.path.join(EXTENSION_PATH, "config", "extension.toml")
dst_dir = os.path.join(self.build_lib, "isaaclab_newton", "config")
os.makedirs(dst_dir, exist_ok=True)
shutil.copy(src, os.path.join(dst_dir, "extension.toml"))Since __init__.py no longer reads from extension.toml at runtime (it uses importlib.metadata instead), this bundling is unnecessary. The setup.py still reads the TOML at build time for metadata, but that happens before wheel creation and doesn't require bundling.
Recommendation: Remove the custom build_py class and the cmdclass={"build_py": build_py} line, along with the shutil import.
2. Consider documenting the [testing] extra
The new isaaclab[testing] extra is a nice addition. Users running tests will need to know to install it:
pip install "isaaclab[testing]"Consider adding this to the developer documentation or CONTRIBUTING guide.
3. Minor: Unused IntEnum import removal
The PR correctly removes an unused from enum import IntEnum import in isaaclab_experimental/__init__.py. This was dead code that was never exposed or used.
🔍 Verification Checklist
- All 14 packages use consistent
importlib.metadata.version()pattern - Fallback to
"0.0.0"is consistent across all packages - No remaining runtime
tomlimports in__init__.pyfiles -
find_namespace_packagesis used correctly with explicit include patterns - Testing dependencies moved without loss
- No breaking changes to public API (removed variables like
ISAACLAB_*_EXT_DIRwere not exported or documented)
⚠️ Potential Considerations
-
Version during editable install: With
importlib.metadata, the version is read from installed package metadata. For editable installs (pip install -e), this should work correctly since pip registers metadata. However, if someone imports the package without installing (e.g., just adding toPYTHONPATH), they'll get"0.0.0". This is acceptable behavior and matches the PR description. -
CI pipelines: Verify that CI workflows install
isaaclab[testing]when running tests. Based on current workflows, test dependencies may have been implicitly available; this change makes them explicit.
Overall, this is a clean modernization that reduces complexity and aligns with current Python packaging standards. The main actionable item is removing the now-dead build_py code in isaaclab_newton/setup.py.
Update (a530236): Reviewed new commits. Changes include:
- Dockerfile additions for Miniconda/conda CI testing
- Extensive new test coverage for install workflows and command parsing
- Test refactors (using Warp types for camera dtypes,
.torchproperty access) - Import cleanups and removal of unnecessary AppLauncher dependencies in unit tests
The new commits are sound and don't introduce any issues. Previous suggestion about dead build_py code in isaaclab_newton/setup.py remains applicable but is non-blocking.
Greptile SummaryThis PR modernizes packaging across all 14 IsaacLab sub-packages by replacing
Confidence Score: 3/5Safe to merge for most packages, but the removal of ISAACLAB_ASSETS_DATA_DIR breaks the documented public API for isaaclab_assets users. The isaaclab_assets init.py removes ISAACLAB_ASSETS_DATA_DIR, a variable that the package's own README.md documents as the canonical way to locate local asset files. Users following that guide will get an ImportError immediately on import. The PR checklist confirms documentation has not been updated. All other changes are clean and consistent across the 14 sub-packages. source/isaaclab_assets/isaaclab_assets/init.py and source/isaaclab_assets/docs/README.md — the removed ISAACLAB_ASSETS_DATA_DIR export conflicts with the still-published usage example in the README. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Package __init__.py imported"] --> B{"importlib.metadata\n.version(pkg_name)"}
B -- "Package installed" --> C["__version__ = real version"]
B -- "PackageNotFoundError" --> D["__version__ = '0.0.0'"]
subgraph "Old approach"
E["import toml"] --> F["Load config/extension.toml"]
F --> G["__version__ from toml"]
E -- "ImportError" --> H["fallback to '0.0.0'"]
end
subgraph "setup.py"
J["find_namespace_packages"] --> K["Auto-discovers sub-packages"]
N["EXTRAS_REQUIRE['testing']"] --> O["pytest, coverage, etc."]
end
|
|
For reviewers: the agents correctly identified that
|
64d5b47 to
a530236
Compare
Description
This pull request modernizes packaging and version management across IsaacLab sub-packages to align with current Python packaging practices and reduce maintenance overhead.
The main change is replacing custom version parsing in package
__init__.pyfiles withimportlib.metadata.version, with a fallback to0.0.0when a package is not installed. In addition, setup scripts were standardized to usesetuptoolsandfind_namespace_packagesfor more consistent package discovery.This PR also improves package metadata handling by loading version and related metadata from
config/extension.tomlwhere appropriate, reducing duplication across setup files. Finally, testing-related dependencies that were previously included in the main install path are now exposed through a dedicated[testing]extra, keeping the default installation lighter and making optional dependencies explicit.These changes simplify initialization logic, remove redundant code, and make the packaging setup more robust and easier to maintain.
Fixes partially:
Type of change
Screenshots
N/A
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there