Skip to content

Conversation

@pascal-roth
Copy link
Collaborator

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/

source/isaaclab/test/terrains/test_terrain_generator.py:20:8: TID251 `isaacsim.core.utils` is banned: Do not import from isaacsim.core.utils; use internal framework utilities instead.
   |
18 | import torch
19 | 
20 | import isaacsim.core.utils.torch as torch_utils
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID251
21 | import pytest

Idea from @Mayankm96

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@pascal-roth pascal-roth self-assigned this Nov 3, 2025
@github-actions github-actions bot added enhancement New feature or request infrastructure labels Nov 3, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 --fix and --exit-non-zero-on-fix flags to .pre-commit-config.yaml
  • Configured [tool.ruff.lint] section in pyproject.toml to 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-fix flag ensures commits are blocked when violations are found
  • Error messages provide clear migration paths (e.g., use isaaclab.sim.utils.prims instead of isaacsim.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
Loading

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@pascal-roth pascal-roth moved this to In review in Isaac Lab Nov 3, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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-commit from 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
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@ooctipus ooctipus left a 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!

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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.yaml with --fix and --exit-non-zero-on-fix flags
  • Configured three banned-api patterns in pyproject.toml with 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
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request infrastructure

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

2 participants