Skip to content

Fixes redundant string to callable#4765

Merged
ooctipus merged 10 commits intoisaac-sim:developfrom
ooctipus:fix/redundant_string_to_callable
Feb 28, 2026
Merged

Fixes redundant string to callable#4765
ooctipus merged 10 commits intoisaac-sim:developfrom
ooctipus:fix/redundant_string_to_callable

Conversation

@ooctipus
Copy link
Collaborator

Description

With resolvable string, the string to callable is now redundant, and many assumption built with string_to_callable at update class from cfg doesn't make sense anymore (Magically change string to type during update was an logic bug).

Fixes # (issue)

Type of change

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

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 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 bug Something isn't working isaac-lab Related to Isaac Lab team labels Feb 27, 2026
@ooctipus ooctipus marked this pull request as ready for review February 27, 2026 12:08
@ooctipus ooctipus requested a review from Mayankm96 as a code owner February 27, 2026 12:08
@ooctipus ooctipus force-pushed the fix/redundant_string_to_callable branch from d69497d to 437bbe3 Compare February 27, 2026 12:08
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 27, 2026

Greptile Summary

This PR cleans up redundant string-to-callable conversion logic in the config system. With the introduction of ResolvableString, callable strings are now kept lazy and only resolved on first use, eliminating the need for eager conversion during config updates.

  • Removes redundant type check for ResolvableString in update_class_from_dict since it's already callable
  • Adds optimization to _wrap_resolvable_strings to preserve container identity when no items need wrapping
  • Updates tests to properly test lazy string resolution behavior
  • Updates imports after string_to_callable was moved to string utils module

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-contained refactoring that removes redundant code and adds performance optimizations. The logic is sound, tests are properly updated, and the changes align with the lazy resolution pattern introduced by ResolvableString
  • No files require special attention

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/utils/configclass.py Adds optimization to _wrap_resolvable_strings to preserve container identity when no wrapping occurs
source/isaaclab/isaaclab/utils/dict.py Removes redundant ResolvableString type check since ResolvableString is callable
source/isaaclab/test/utils/test_configclass.py Updates test to use callable string format instead of direct function reference

Last reviewed commit: 437bbe3

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.

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

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

We need that in ASAP :)

@AntoineRichard AntoineRichard force-pushed the fix/redundant_string_to_callable branch from 801726c to ac529bc Compare February 27, 2026 14:19
@kellyguo11 kellyguo11 changed the title fix redundant string to callable Fixes redundant string to callable Feb 27, 2026
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
@github-actions github-actions bot added the isaac-mimic Related to Isaac Mimic team label Feb 27, 2026
@ooctipus ooctipus merged commit c43a6f5 into isaac-sim:develop Feb 28, 2026
7 of 10 checks passed
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 isaac-mimic Related to Isaac Mimic team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants