Require skrl 2.1 for Warp 1.13#5838
Conversation
Raise the isaaclab-rl skrl optional dependency floor to the first version compatible with Warp 1.13's public API. Keep the wheel-builder metadata aligned so generated wheels install the same compatible floor, and add metadata tests to prevent regressions.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR raises the skrl optional dependency floor from >=1.4.3/>=2.0.0 to >=2.1.0 to ensure compatibility with warp-lang 1.13, which removed the wp.context API that older skrl versions relied upon. The change is well-structured, updates both source package metadata and wheel-builder metadata, and includes comprehensive regression tests.
Design Assessment
✅ Architecture: Clean separation of concerns—dependency metadata updates in pyproject.toml and python_packages.toml, with corresponding test coverage in test_wheel_builder_metadata.py.
✅ Consistency: The version floor is updated consistently across:
source/isaaclab_rl/pyproject.toml(bothskrlandallextras)tools/wheel_builder/res/python_packages.toml(bothskrlandallextras)
✅ Test Design: The new tests use packaging library for robust requirement parsing rather than string matching, which is the correct approach for version specifier handling.
Findings
No Issues Found 🎉
The implementation is thorough:
test_isaaclab_rl_skrl_requirement_supports_warp_113()validates the source package requires skrl ≥2.1.0test_wheel_builder_skrl_requirement_matches_source_package()ensures wheel-builder stays aligned with source- Helper functions (
_minimum_version_for,_requirement_for) are well-designed and reusable
Minor Observation (Not blocking):
- The wheel-builder previously had
skrl>=1.4.3while pyproject.toml hadskrl>=2.0.0—this PR corrects that inconsistency by aligning both to>=2.1.0.
Test Coverage
✅ Adequate. The PR adds 56 new lines of test code that:
- Verify the source package skrl requirement supports Warp 1.13
- Verify wheel-builder metadata matches source package
- Use proper version comparison logic via
packaging.version.Version
The tests are regression-proof: any future accidental downgrade of the skrl floor will be caught.
CI Status
- ❌
Check changelog fragments— FAILED (may need investigation) - ✅
Build Wheel— passed - ✅
Check for Broken Links— passed - ⏳
pre-commit,Installation Tests,license-check— pending
The changelog fragment failure should be investigated. The fragment at source/isaaclab_rl/changelog.d/skrl-warp-113.rst appears properly formatted—the failure may be unrelated or a CI timing issue.
Verdict
No issues found — This is a clean, well-tested dependency update. The approach of bundling tests that enforce source/wheel metadata alignment is excellent for preventing future drift. Ready to merge once CI stabilizes.
Update (97b0cab): The new commit simplifies the test file by removing the skrl-specific validation tests. The changes:
- Removed imports:
packaging.requirements.Requirement,packaging.version.Version, and the_SKRL_WARP_113_MIN_VERSIONconstant - Removed functions:
_source_rl_optional_dependencies(),_wheel_builder_rl_optional_dependencies(),_requirement_for(),_minimum_version_for() - Removed tests:
test_isaaclab_rl_skrl_requirement_supports_warp_113()andtest_wheel_builder_skrl_requirement_matches_source_package() - Simplified remaining test:
test_wheel_builder_rsl_rl_pin_matches_source_package()now directly reads the wheel-builder dependencies inline instead of using the removed helper
This is a significant scope reduction—the PR now only updates the skrl version floor in metadata files without the corresponding regression tests. The test file is cleaner but loses the automated validation that the skrl version requirement stays compatible with Warp 1.13.
Assessment: Still LGTM. The core change (bumping skrl to ≥2.1.0) remains valid. Removing the tests is a reasonable choice if the team prefers not to maintain explicit version-floor assertions in CI.
Update (d01541d): ✅ This commit addresses Kelly's feedback about script alignment.
Changes:
- Updated
SKRL_VERSIONfrom "2.0.0" to "2.1.0" in all four skrl scripts:scripts/reinforcement_learning/skrl/play.pyscripts/reinforcement_learning/skrl/play_skrl.pyscripts/reinforcement_learning/skrl/train.pyscripts/reinforcement_learning/skrl/train_skrl.py
- Added changelog skip fragment
source/isaaclab_tasks/changelog.d/skrl-warp-113.skip - Added new regression test
test_skrl_scripts_min_version_matches_source_package()that validates all script-levelSKRL_VERSIONconstants match the minimum version inpyproject.toml
Assessment: Excellent update. The new test is well-designed—it parses pyproject.toml to extract the actual skrl lower bound and validates all scripts against it. This prevents future drift between package metadata and runtime version guards. Ready to merge.
Greptile SummaryThis PR raises the
Confidence Score: 4/5Safe to merge; the dependency bump is correct and the new tests will prevent future regressions. The metadata changes are minimal and clearly correct. The only notable gap is in the _minimum_version_for test helper, which doesn't account for the > strict-greater-than specifier operator — a future requirement written that way would produce a confusing None result instead of a meaningful version comparison failure. This doesn't affect the correctness of the current fix. source/isaaclab/test/cli/test_wheel_builder_metadata.py — the _minimum_version_for helper has a minor gap in operator coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User installs isaaclab-rl skrl extra] --> B{skrl version resolver}
B -->|Before PR: skrl>=2.0.0 / >=1.4.3| C[Could install skrl 2.0.x]
B -->|After PR: skrl>=2.1.0| D[Installs skrl 2.1.0+]
C --> E[Uses wp.context API]
E --> F[💥 Crash: wp.context removed in warp-lang 1.13]
D --> G[No wp.context usage]
G --> H[✅ Compatible with warp-lang 1.13]
subgraph Regression Tests
T1[test_isaaclab_rl_skrl_requirement_supports_warp_113]
T2[test_wheel_builder_skrl_requirement_matches_source_package]
T1 -->|verifies floor ≥2.1.0| D
T2 -->|verifies pyproject.toml == wheel_builder| D
end
Reviews (1): Last reviewed commit: "Require skrl 2.1 for Warp 1.13" | Re-trigger Greptile |
| versions = [ | ||
| Version(spec.version) | ||
| for spec in Requirement(requirement).specifier | ||
| if spec.operator in {"==", "===", ">=", "~="} | ||
| ] |
There was a problem hiding this comment.
_minimum_version_for helper recognises >=, ~=, ==, and === as lower-bound indicators but silently omits > (strict greater-than). A requirement written as skrl>2.0.0 would produce an empty versions list, causing minimum_version to be None, and the test would then fail on the generic assert minimum_version is not None rather than on the intended version comparison. Adding ">" to the set makes the helper robust against that spelling and keeps error messages meaningful.
| versions = [ | |
| Version(spec.version) | |
| for spec in Requirement(requirement).specifier | |
| if spec.operator in {"==", "===", ">=", "~="} | |
| ] | |
| versions = [ | |
| Version(spec.version) | |
| for spec in Requirement(requirement).specifier | |
| if spec.operator in {"==", "===", ">", ">=", "~="} | |
| ] |
kellyguo11
left a comment
There was a problem hiding this comment.
we should probably also update https://github.com/isaac-sim/IsaacLab/blob/develop/scripts/reinforcement_learning/skrl/train_skrl.py#L36 to align?
|
Oh yeah sorry about that... I just patched it. |
Description
Raises the
skrloptional dependency floor to>=2.1.0in both theisaaclab-rlpackage metadata and wheel-builder metadata. This avoids installing olderskrlreleases that reference the removedwp.contextAPI when used withwarp-lang1.13.No runtime code changes are included.
Fixes: N/A
Type of change
Screenshots
N/A - dependency metadata change.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there