Skip to content

Suppress OVRTX API deprecation warnings - #6950

Open
mataylor-nvidia wants to merge 2 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/ovrtx-suppress-deprecation-logs
Open

Suppress OVRTX API deprecation warnings#6950
mataylor-nvidia wants to merge 2 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/ovrtx-suppress-deprecation-logs

Conversation

@mataylor-nvidia

@mataylor-nvidia mataylor-nvidia commented Aug 6, 2026

Copy link
Copy Markdown

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

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 added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Too many files changed for review (2800 files, 100 file limit).

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isaac Lab Review Bot

The PR conditionally suppresses noisy OVRTX initialization and teardown logs and adds the required isaaclab_ov changelog fragment. However, legacy multi-environment initialization re-enters the same single-use force_log_level context manager, which can abort initialization.

  • Design and architecture: Containing third-party log suppression around specific OVRTX calls is appropriately scoped, but the initialization path must not retain and reuse a single-use generator context manager across separate operation groups.
  • API: No public symbols or signatures change. The observable change is limited to OVRTX logging verbosity during legacy initialization and teardown, and the changelog fragment correctly documents it under Fixed.
  • Implementation: The teardown path creates a fresh suppression context for reset_stage, but _initialize_from_spec_legacy creates _level_ctx once and enters it around both stage loading and multi-environment cloning. When force_log_level is selected, the second entry is invalid. Construct a fresh context for each block or place both groups under one context.

Significant concerns. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

spec: Tiled camera description (resolution, paths, data types).
"""
_suppress = logging.getLogger().getEffectiveLevel() > logging.DEBUG
_level_ctx = force_log_level(logging.ERROR) if _suppress else contextlib.nullcontext()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Critical · Implementation — Reused suppression context entered twice

_level_ctx is created once and entered by two separate with statements (lines 531 and 537). A force_log_level context manager built via contextlib.contextmanager is single-use, so the second entry raises RuntimeError: generator didn't yield on the common path (default WARNING root level with num_envs > 1), aborting legacy initialization. Call force_log_level(...)/nullcontext() freshly per block, as _close_legacy already does, or wrap both operation groups in one context.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant