Conversation
Greptile SummaryThis PR bisects CI failures by reverting two previously merged commits — the Cassie Newton joint-drive fix (#5251) and the Python 3.12 version pin (#5213) — and applies a Docker CI fix to Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["develop @ caab0dec\n(Cassie Newton fix + Python 3.12 pin merged)"] --> B["8910efe0\nRevert Cassie Newton fix\n(schemas.py, cassie.py, extension.toml, CHANGELOG)"]
B --> C["5ab304b8\nRevert Python 3.12 pin\n(all setup.py files, license-check.yaml,\nlicense-exceptions.json, haply docs)"]
C --> D["15da8d51 HEAD\nApply CI fix\n(Dockerfile.base, Dockerfile.curobo only)"]
D --> E["Net result on develop:\n- python_requires ≥3.10\n- Cassie has no Newton drive fix\n- Docker CI workarounds applied"]
|
| @@ -1,18 +1,6 @@ | |||
| Changelog | |||
There was a problem hiding this comment.
Missing changelog entries for source-directory reverts
AGENTS.md requires a changelog entry for every change targeting the source/ directory. This PR reverts schemas.py, schemas_cfg.py, and cassie.py (all under source/) and removes the changelog entries that documented those additions, but adds no new entry to record the removal. If these reverts land in a release, the changelog will silently drop the feature with no user-visible notice or migration guidance.
A bump to e.g. 4.6.1 (and a corresponding entry in source/isaaclab_assets/docs/CHANGELOG.rst at e.g. 0.3.2) with a Removed section explaining why the Newton-drive fix for Cassie was reverted would keep the history accurate. The same applies to the Python-version-pin revert across all setup.py files.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR reverts two previously merged changes — PR #5251 (Cassie Newton joint drive fix with ensure_drives_exist) and PR #5213 (Python 3.12 pinning) — to bisect CI failures. It also includes a CI fix commit that downgrades the license-check workflow to Isaac Sim 5.1.0, updates Docker images to install nvidia-srl-usd-to-urdf explicitly (previously bundled), and adjusts license exception entries. The revert logic is clean, but there are a few inconsistencies worth addressing.
Design Assessment
Design is sound. Reverting suspected CI-breaking commits to bisect failures is the right approach. The additional CI fix (Docker + license-check changes) is a reasonable forward-fix for the older Isaac Sim version. The ensure_drives_exist feature and Python 3.12 pinning can be re-landed once CI stability is confirmed.
Findings
🟡 Warning: isaaclab_contrib classifiers diverge from all other packages — source/isaaclab_contrib/setup.py:59-62
All other packages in this PR use the classifier pattern Python :: 3.11, Python :: 3.12 with Isaac Sim :: 5.0.0, 5.1.0, 6.0.0. However, isaaclab_contrib uses Python :: 3.10, Python :: 3.11 with Isaac Sim :: 4.5.0, 5.0.0 — dropping both 3.12 and 6.0.0 support entirely. If isaaclab_contrib genuinely only supports older Isaac Sim versions, this is fine but should be documented. If this was a copy-paste divergence in the revert, it should be aligned with the other packages.
🟡 Warning: Docker symlink assumes nvidia-srl-usd-to-urdf installs into Kit python's nvidia/srl — docker/Dockerfile.base:116-118
The pip install --no-deps nvidia-srl-usd-to-urdf installs into the Kit python at ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages/, but the symlink source still points to ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages/nvidia/srl. This works because isaaclab.sh -p -m pip uses the Kit python, but the comment says the package "was previously bundled in the Isaac Sim image but has been removed" — if the package layout of nvidia-srl-usd-to-urdf doesn't install to nvidia/srl, the symlink will silently fail to resolve. Consider adding a build-time check (e.g., ls ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages/nvidia/srl between the pip install and the symlink) to fail fast during the Docker build if the path doesn't exist.
🔵 Suggestion: Hardcoded Python 3.12 paths in Docker may conflict with Python version downgrade — docker/Dockerfile.base:109-110, 117
While this PR downgrades python_requires across all packages to >=3.10 (or >=3.11), the Dockerfiles still hardcode python3.12 in PYTHONPATH, LD_LIBRARY_PATH, and the symlink paths. This is fine as long as the Docker base image ships Python 3.12, but creates a subtle inconsistency — the setup.py files claim 3.10/3.11 support while the Docker build only works with 3.12. Worth a comment noting that Docker images remain 3.12-only even though the packages advertise broader compatibility.
🔵 Suggestion: License-check CI comment could be more specific — .github/workflows/license-check.yaml:55
The comment # Adjust as needed on the Python version line is vague. Since this is a deliberate downgrade from 3.12 to 3.11 for CI stability, consider something like # Python 3.11 for Isaac Sim 5.1.0 compatibility to document the intent.
Test Coverage
- Bug fix PR: No regression tests needed — this is a revert to bisect CI failures, not a new fix. The reverted features had their own tests.
- Coverage assessment: Adequate. The changes are to CI infrastructure, Docker, setup.py classifiers, and configuration reverts. No runtime behavioral changes require new tests.
CI Status
- ✅ pre-commit, labeler, link checker: passing
- ⏳ license-check, Docker builds, docs builds: pending
- The license-check and Docker builds are the ones most affected by this PR — their results will validate whether the CI fix is correct.
Verdict
Minor fixes needed
The revert approach is correct and the CI fix changes are well-targeted. The isaaclab_contrib classifier divergence is the main item to verify — confirm whether it's intentional or a revert artifact. The Docker symlink concern is low-risk but worth a defensive check. Overall a clean bisection PR that should unblock CI.
| "Programming Language :: Python :: 3.10", | ||
| "Programming Language :: Python :: 3.11", | ||
| "Isaac Sim :: 4.5.0", | ||
| "Isaac Sim :: 5.0.0", |
There was a problem hiding this comment.
🟡 Warning: The classifiers here diverge from every other package in this PR. All others use Python :: 3.11/3.12 + Isaac Sim :: 5.0.0/5.1.0/6.0.0, but contrib uses Python :: 3.10/3.11 + Isaac Sim :: 4.5.0/5.0.0 — dropping 3.12 and 6.0.0 entirely.
Is this intentional (contrib only supports older Isaac Sim versions) or a revert artifact? If intentional, a brief comment here would help future readers understand why contrib has different compatibility.
| # been removed. Install with --no-deps to avoid lxml<5 conflict with dex-retargeting. | ||
| # Then symlink nvidia.srl into the ml_archive nvidia namespace so Python can find it | ||
| # (the deprecated ml_archive prebundle shadows site-packages for the nvidia namespace). | ||
| RUN ${ISAACLAB_PATH}/isaaclab.sh -p -m pip install --no-deps nvidia-srl-usd-to-urdf \ |
There was a problem hiding this comment.
🔵 Suggestion: Since nvidia-srl-usd-to-urdf was previously bundled and is now explicitly installed, the symlink on line 118 assumes the pip package installs its content at nvidia/srl within site-packages. Consider adding a quick sanity check between the install and symlink (e.g., && test -d ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages/nvidia/srl) to fail the Docker build early if the package layout changes in a future version.
…int drives (isaac-sim#5251)" This reverts commit caab0de.
…)" This reverts commit 447b4a4.
5176195 to
89dd05f
Compare
)" This reverts commit 89dd05f.
…oint drives (isaac-sim#5251)" This reverts commit ebad4e7.
Description
bisect ci failures
Fixes # (issue)
bisect ci failures
Type of change
Screenshots
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there