Skip to content

Conversation

@AntoineRichard
Copy link
Collaborator

Description

Warn only once. @pascal-roth I'm wondering if this should not be handled at the logger level.

Fixes # (issue)

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

@github-actions github-actions bot added asset New asset feature or request isaac-mimic Related to Isaac Mimic team labels Feb 3, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 3, 2026

Greptile Overview

Greptile Summary

This PR configures Python's warnings module to display DeprecationWarning and FutureWarning only once per session across all IsaacLab modules. The change prevents repetitive warning messages when deprecated properties are accessed multiple times from different locations.

Key changes:

  • Added warnings.filterwarnings("once", ...) configuration to 7 module __init__.py files
  • Filters applied to DeprecationWarning and FutureWarning categories
  • Uses regex patterns to match each module namespace (isaaclab.*, isaaclab_assets.*, etc.)

Observations:

  • The implementation correctly uses the "once" action which shows each unique warning only once per session, regardless of call site
  • This approach is simpler than handling at the logger level, though the PR author questions whether logger integration would be better
  • The codebase has a RateLimitFilter in isaaclab/utils/logger.py for rate-limiting log messages, but Python's warnings module operates separately from the logging module unless explicitly bridged with logging.captureWarnings(True)
  • Import statements are placed after other module-level code rather than at the top per PEP8 conventions

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - it only configures warning display behavior without changing functional logic
  • The changes are straightforward and limited to warning filter configuration. The implementation correctly uses the warnings module API, though import placement violates PEP8 style conventions. No functional logic is altered, only warning display behavior.
  • All files have the same minor style issue with import placement - consider moving imports to the top of files per PEP8

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_assets/isaaclab_assets/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_contrib/isaaclab_contrib/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_mimic/isaaclab_mimic/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_physx/isaaclab_physx/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_rl/isaaclab_rl/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved
source/isaaclab_tasks/isaaclab_tasks/init.py Adds warning filter configuration to show deprecation/future warnings once per session - import placement could be improved

Sequence Diagram

sequenceDiagram
    participant User
    participant Module as Module __init__.py
    participant WarningsModule as Python warnings module
    participant DeprecatedCode as Deprecated Code

    User->>Module: Import isaaclab module
    Module->>WarningsModule: warnings.filterwarnings("once", DeprecationWarning, module="isaaclab.*")
    Module->>WarningsModule: warnings.filterwarnings("once", FutureWarning, module="isaaclab.*")
    Note over WarningsModule: Configure filters to show warnings only once per session
    
    User->>DeprecatedCode: Access deprecated property (1st time)
    DeprecatedCode->>WarningsModule: warnings.warn(..., DeprecationWarning)
    WarningsModule->>User: Display warning message
    Note over WarningsModule: Record warning shown
    
    User->>DeprecatedCode: Access same deprecated property (2nd time)
    DeprecatedCode->>WarningsModule: warnings.warn(..., DeprecationWarning)
    WarningsModule-->>User: Suppress warning (already shown)
    Note over WarningsModule: Warning filtered out (once filter active)
Loading

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.

7 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile


# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move import warnings to the top of the file with other imports (line 8-9) per PEP8 conventions

Suggested change
import warnings
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move import warnings to the top of the file with other imports (line 7-8) per PEP8 conventions

Suggested change
import warnings
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

__version__ = ISAACLAB_CONTRIB_METADATA["package"]["version"]

# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move import warnings to the top of the file with other imports (line 6-10) per PEP8 conventions

Suggested change
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

__version__ = "1.0.0"

# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add import warnings at the top of the file per PEP8 conventions

Suggested change
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

__version__ = ISAACLAB_PHYSX_METADATA["package"]["version"]

# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move import warnings to the top of the file with other imports (line 6-9) per PEP8 conventions

Suggested change
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add import warnings at the top of the file per PEP8 conventions

Suggested change
import warnings
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

__version__ = ISAACLAB_TASKS_METADATA["package"]["version"]

# Configure deprecation warnings to show only once per session (regardless of call site)
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move import warnings to the top of the file with other imports (line 6-10) per PEP8 conventions

Suggested change
# This prevents repeated warnings when deprecated properties are accessed from multiple locations
import warnings

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Labels

asset New asset feature or request isaac-mimic Related to Isaac Mimic team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant