-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Adds pre-commit to declare isaacsim.core.utils as banned-api
#3926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR adds ruff pre-commit enforcement to prevent future usage of isaacsim.core.utils imports in the codebase. The changes configure the TID251 (banned-api) rule to block three import patterns: isaacsim.core.utils, isaacsim.core.utils.prims, and isaacsim.core.utils.stage, with helpful error messages directing developers to use internal IsaacLab utilities instead.
Key Changes:
- Added ruff pre-commit hook with
--fixand--exit-non-zero-on-fixflags to.pre-commit-config.yaml - Configured
[tool.ruff.lint]section inpyproject.tomlto enable TID251 rule - Defined banned-api patterns with custom error messages pointing to isaaclab alternatives
Implementation Notes:
- The configuration only prevents new imports of banned APIs; existing usages in the codebase will need separate cleanup
- The
--exit-non-zero-on-fixflag ensures commits are blocked when violations are found - Error messages provide clear migration paths (e.g., use
isaaclab.sim.utils.primsinstead ofisaacsim.core.utils.prims)
Confidence Score: 4/5
- This PR is safe to merge with low risk - it adds preventive tooling without affecting runtime behavior
- The configuration changes are straightforward and only add linting enforcement. However, the score is 4/5 rather than 5/5 because: (1) ruff is a new tool addition to the pre-commit pipeline that may have integration considerations, and (2) the codebase currently has many existing violations of these banned imports that will need to be addressed separately
- No files require special attention - both configuration files have simple, well-structured changes
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| .pre-commit-config.yaml | 5/5 | Added ruff pre-commit hook with --fix and --exit-non-zero-on-fix arguments to enforce banned-api checks |
| pyproject.toml | 4/5 | Configured ruff to ban isaacsim.core.utils imports with TID251 rule and provided helpful alternative import messages |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant PreCommit as Pre-commit Hook
participant Ruff as Ruff Linter
participant Config as pyproject.toml
Dev->>Dev: Makes code changes
Dev->>PreCommit: git commit
PreCommit->>Ruff: Run ruff --fix --exit-non-zero-on-fix
Ruff->>Config: Read [tool.ruff.lint] configuration
Config-->>Ruff: TID251 rule enabled
Ruff->>Config: Read banned-api list
Config-->>Ruff: isaacsim.core.utils banned
Ruff->>Ruff: Scan code for banned imports
alt Banned import found
Ruff-->>PreCommit: Exit non-zero with error message
PreCommit-->>Dev: Commit blocked with error
Dev->>Dev: Fix import to use isaaclab utilities
Dev->>PreCommit: git commit (retry)
else No banned imports
Ruff-->>PreCommit: Exit zero (success)
PreCommit-->>Dev: Commit allowed
end
2 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR updates the ruff pre-commit hook version from v0.7.2 to v0.14.3 and removes the "check for latest version" comment.
Key Changes:
- Updates
ruff-pre-commitfrom v0.7.2 to v0.14.3 (.pre-commit-config.yaml:88) - Removes version check comment (.pre-commit-config.yaml:88)
Context:
The PR title mentions "Adds pre-commit to declare isaacsim.core.utils as banned-api", but the actual banned-api configuration was added in a previous commit (5b40fa1). This PR only updates the ruff version.
Note:
The version jump from v0.7.2 to v0.14.3 is significant (7 minor versions). This could introduce:
- Changes to the TID251 (banned-api) rule behavior
- New lint rules that might affect the codebase
- Potential breaking changes in rule implementation
Confidence Score: 3/5
- This PR is moderately safe but requires testing due to significant version jump
- The version update from v0.7.2 to v0.14.3 is a large jump (7 minor versions). While updating to the latest version is generally good practice, such a large version change could introduce behavioral changes in the TID251 rule or other ruff functionality. The PR should be tested with pre-commit hooks to ensure no unexpected issues arise.
- No files require special attention, but the ruff version update should be tested by running pre-commit hooks
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| .pre-commit-config.yaml | 3/5 | Updates ruff version from v0.7.2 to v0.14.3 (significant jump) and removes version comment. Large version jump could introduce behavioral changes or new rules. |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Git as Git Commit
participant PreCommit as Pre-commit Hook
participant Ruff as Ruff v0.14.3
participant Code as Codebase
Dev->>Git: Commit changes
Git->>PreCommit: Trigger pre-commit hooks
PreCommit->>Ruff: Run ruff with --fix --exit-non-zero-on-fix
Ruff->>Code: Check for TID251 violations
alt Banned import found (e.g., isaacsim.core.utils)
Code-->>Ruff: Import detected
Ruff-->>PreCommit: Exit non-zero (violation found)
PreCommit-->>Git: Block commit
Git-->>Dev: Commit failed - fix banned imports
else No violations
Code-->>Ruff: No banned imports
Ruff-->>PreCommit: Exit zero (success)
PreCommit-->>Git: Allow commit
Git-->>Dev: Commit successful
end
1 file reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Adds ruff pre-commit hook with TID251 (banned-api) rule to prevent future imports from isaacsim.core.utils and its submodules. The configuration correctly uses the most-specific-match approach where isaacsim.core.utils.prims and isaacsim.core.utils.stage provide targeted error messages with migration paths, while the general isaacsim.core.utils pattern catches all other submodules.
Key changes:
- Added ruff v0.14.3 to
.pre-commit-config.yamlwith--fixand--exit-non-zero-on-fixflags - Configured three banned-api patterns in
pyproject.tomlwith specific guidance messages - The hook will automatically prevent
isaacsim.core.utils.*imports in future PRs
Confidence Score: 4/5
- This PR is safe to merge with minimal risk - it only adds pre-commit checks without modifying runtime code
- The changes are straightforward and well-structured: adds a ruff pre-commit hook with banned-api patterns. The configuration correctly uses most-specific-match logic for targeted error messages. However, score reduced to 4 because: (1) The PR doesn't actually fix the existing 113+ files that currently violate this rule, only prevents future violations, and (2) the banned-api patterns could be redundant (the general pattern catches all submodules)
- No files require special attention - both configuration files are standard and correctly formatted
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| pyproject.toml | 3/5 | Adds ruff banned-api configuration to block isaacsim.core.utils imports, but patterns don't use wildcards to catch submodule imports |
| .pre-commit-config.yaml | 5/5 | Adds ruff pre-commit hook with auto-fix enabled |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Git as Git Commit
participant PreCommit as Pre-commit Hook
participant Ruff as Ruff Linter
participant Config as pyproject.toml
Dev->>Git: git commit with code changes
Git->>PreCommit: Trigger pre-commit hooks
PreCommit->>Ruff: Run ruff with --fix
Ruff->>Config: Read banned-api patterns
Config-->>Ruff: Return TID251 rules
alt Import matches banned pattern
Ruff->>Ruff: Check if import matches isaacsim.core.utils*
Ruff->>Ruff: Select most specific pattern match
Ruff-->>PreCommit: TID251 violation found
PreCommit-->>Git: Exit non-zero (block commit)
Git-->>Dev: Commit blocked with error message
else No banned imports
Ruff-->>PreCommit: All checks pass
PreCommit-->>Git: Success
Git-->>Dev: Commit successful
end
2 files reviewed, no comments
Description
Prevent isaacsim.core.utils to be used in future PRs using the banned-api from ruff. https://docs.astral.sh/ruff/rules/banned-api/
Idea from @Mayankm96
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there