chore: update ruff target-version to py312#5681
Conversation
There was a problem hiding this comment.
Code Review Summary
Verdict: ✅ Approve
This PR correctly aligns the ruff linter target-version with the project's requires-python = ">=3.12,<3.13" constraint. The change is well-motivated and the auto-fixes are appropriate.
Analysis
✅ Core Change - Correct
The target-version = "py312" update in pyproject.toml is the right fix. The previous py310 setting caused:
- False positives for Python 3.11+ builtins (
ExceptionGroup,tomllib) - Incorrect stdlib import ordering
✅ Import Reorderings - Valid
All import changes follow PEP 8/isort conventions with py312 stdlib awareness:
tomllibis now recognized as stdlib (moved before third-party imports)distutils.util.strtoboolreordering is valid (third-party via setuptools shim)
✅ Exception Change - Safe
# se3_haply.py line 347
-except asyncio.TimeoutError:
+except TimeoutError:Since Python 3.11, asyncio.TimeoutError is an alias to the built-in TimeoutError. This change is correct and idiomatic for Python 3.12+.
✅ Pre-commit Fix (new in 8a9e499)
Added UP040 and UP047 to the ignore list:
"UP040", # TypeAlias annotation → `type` keyword (PEP 695, migrate later)
"UP047", # Generic function → type parameters (PEP 695, migrate later)This properly defers the PEP 695 migration while unblocking the CI. The ignore comments indicate these are intentional deferrals, not permanent suppressions.
📝 Minor Note - Scope
The .gitignore addition (*.swp for vim swap files) is unrelated to the ruff configuration but is a reasonable quality-of-life improvement. Consider splitting this into a separate commit for cleaner git history.
LGTM - Ready to merge.
Greptile SummaryThis PR corrects ruff's
Confidence Score: 4/5Safe to merge with a follow-up to replace the The core change — updating scripts/reinforcement_learning/rl_games/train.py and scripts/reinforcement_learning/rl_games/train_rl_games.py — both still import Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ruff target-version py310 → py312] --> B{Import classification changes}
B --> C[tomllib: third-party → stdlib\nCorrect: added in Python 3.11]
B --> D[distutils: stdlib → third-party\nWarning: removed in Python 3.12]
B --> E[asyncio.TimeoutError → TimeoutError\nCorrect: UP041 fix for Python 3.12]
D --> F{setuptools installed?}
F -->|Yes| G[Import succeeds at runtime\nbut fragile / not guaranteed]
F -->|No| H[ModuleNotFoundError at startup\ntrain.py & train_rl_games.py]
Reviews (1): Last reviewed commit: "chore: update ruff target-version to py3..." | Re-trigger Greptile |
| from distutils.util import strtobool | ||
|
|
||
| import gymnasium as gym | ||
| from distutils.util import strtobool |
There was a problem hiding this comment.
distutils removed in Python 3.12 — import will fail at runtime
distutils was entirely removed from the Python standard library in 3.12 (PEP 632). On a clean Python 3.12 environment without setuptools, this import raises ModuleNotFoundError at startup. Ruff correctly reclassified it as a third-party import (acknowledging it is no longer stdlib), but didn't remove the broken call. The same issue exists in train_rl_games.py line 31. The recommended fix is to inline a small replacement — e.g. bool(val.lower() in {"y", "yes", "t", "true", "on", "1"}) — or depend on a maintained PyPI alternative such as str2bool.
ba0c920 to
8a8ef77
Compare
There was a problem hiding this comment.
Code Review Summary
Verdict: ✅ Approve
This PR correctly aligns the ruff linter target-version with the project's requires-python = ">=3.12,<3.13" constraint.
Analysis
✅ Core Configuration Change
The target-version = "py312" update in pyproject.toml addresses:
- False positives for Python 3.11+ builtins (
ExceptionGroup,tomllib) - Incorrect stdlib import ordering for
tomllib(now correctly treated as stdlib)
✅ PEP 695 Rules Suppressed
"UP040", # TypeAlias annotation → `type` keyword (PEP 695, migrate later)
"UP047", # Generic function → type parameters (PEP 695, migrate later)Good decision to defer PEP 695 type syntax migration — this prevents a large refactor while enabling the py312 target.
✅ Auto-fix Changes
All changes are correct ruff auto-fixes:
- Import reordering:
tomllibmoved beforepathlibin several files (now recognized as stdlib) distutils.util.strtoboolreplacement: Migrated toisaaclab.utils.string.strtoboolutility function- Exception modernization:
asyncio.TimeoutError→TimeoutErrorinse3_haply.py(correct since Python 3.11)
✅ Changelog Skip File
Empty .skip file added to changelog.d/ — appropriate for internal tooling changes.
LGTM — Clean fix with proper PEP 695 deferrals. CI pre-commit check passes.
Update (1eb9cfd): Reviewed incremental changes. The strtobool function was properly added to isaaclab.utils.string module instead of using local helpers — this is a better approach as it centralizes the deprecated distutils.util.strtobool replacement in a reusable utility. Implementation is correct and follows the same semantics. No issues found.
🔄 Incremental Review UpdateNew commit: Changes Since Last ReviewThe Before: def _strtobool(val: str) -> bool:
return val.lower() in ("y", "yes", "t", "true", "on", "1")After: def _strtobool(val: str) -> bool:
"""Convert a string representation of truth to True/False.
Raises ValueError for unrecognised values.
"""
v = val.lower()
if v in ("y", "yes", "t", "true", "on", "1"):
return True
elif v in ("n", "no", "f", "false", "off", "0"):
return False
else:
raise ValueError(f"invalid truth value {val!r}")Assessment ✅This is a good improvement — the new implementation:
Verdict remains: ✅ Approve |
The project requires Python >=3.12 but ruff was configured with target-version = py310. This caused false positives for 3.11+ builtins and prevented correct stdlib import sorting for tomllib. PEP 695 rules (UP040, UP047) are suppressed for now to avoid a large migration; tracked separately for incremental adoption.
8a8ef77 to
1eb9cfd
Compare
The project requires
python >= 3.12but ruff was configured withtarget-version = "py310". This causes false positives (e.g.ExceptionGroupflagged as undefined) and prevents correct stdlib import sorting for modules liketomllib.This PR updates
target-versionto"py312"and applies the resulting auto-fixes.