Skip to content

Fixes deprecated teleop device factory to resolve class_type strings#4770

Closed
rwiltz wants to merge 2 commits intoisaac-sim:developfrom
rwiltz:rwiltz/fix_failing_unit_test
Closed

Fixes deprecated teleop device factory to resolve class_type strings#4770
rwiltz wants to merge 2 commits intoisaac-sim:developfrom
rwiltz:rwiltz/fix_failing_unit_test

Conversation

@rwiltz
Copy link
Contributor

@rwiltz rwiltz commented Feb 27, 2026

Description

Fix TypeError: issubclass() arg 1 must be a class in the deprecated create_teleop_device factory. Device configs (e.g. Se3KeyboardCfg) use {DIR} string placeholders in class_type that are only auto-resolved by @configclass, not plain @DataClass. The factory now resolves these strings before calling issubclass().
Add a new test (test_create_teleop_device_from_devices_cfg) that exercises the factory through the DevicesCfg pattern used by env configs.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@rwiltz rwiltz requested a review from kellyguo11 February 27, 2026 19:59
@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Feb 27, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 27, 2026

Greptile Summary

This PR fixes a TypeError: issubclass() arg 1 must be a class in the deprecated teleop device factory when device configs use @dataclass instead of @configclass. The issue occurred because {DIR} placeholders in class_type fields are only auto-resolved by @configclass, not by @dataclass.

Key changes:

  • Added _resolve_class_ref() helper that mirrors the @configclass MRO-walking logic to manually resolve {DIR} placeholders
  • Applied the resolution to both device constructors and retargeter constructors before calling issubclass()
  • Added comprehensive test coverage through test_create_teleop_device_from_devices_cfg() that exercises the DevicesCfg pattern

The implementation correctly handles inheritance by walking the MRO to find the declaring class, ensuring {DIR} resolves relative to the original declaration. This is a solid, defensive fix for the deprecated factory function.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - it's a well-isolated bug fix in a deprecated module with proper test coverage
  • The implementation correctly mirrors existing @configclass logic for resolving {DIR} placeholders, is applied consistently to both device and retargeter constructors, includes targeted test coverage, and only affects a deprecated factory function. No breaking changes or side effects.
  • No files require special attention

Important Files Changed

Filename Overview
source/isaaclab_teleop/isaaclab_teleop/deprecated/teleop_device_factory.py Added _resolve_class_ref() helper to resolve {DIR} placeholders in class_type strings before calling issubclass(), fixing TypeError for configs using @dataclass
source/isaaclab_teleop/test/test_openxr_device_constructors.py Added test test_create_teleop_device_from_devices_cfg() to verify the factory works with DevicesCfg pattern that uses {DIR} placeholders

Last reviewed commit: e18d95d

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.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@ooctipus
Copy link
Collaborator

ooctipus commented Feb 27, 2026

#4765

@rwiltz Thanks for the PR, I think this would fix it as well?

@kellyguo11
Copy link
Contributor

should be addressed in #4765, will close this one.

@kellyguo11 kellyguo11 closed this Feb 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants