Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes configclass dict conversion for torch tensors #1530

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

lgulich
Copy link
Contributor

@lgulich lgulich commented Dec 12, 2024

Description

Fix configclass dict conversion for torch tensors

Up to v1.2.0 if a configclass would contain a list/tuple of torch
tensors it would be left as is.

#1227 changed the behavior of converting lists/tuples in a dict, which
means that currently torch tensors are converted to an empty dict,
effectively losing all contained data.

The underlying issue is that torch.tensor.__dict__ returns an empty
dict, which was (luckily) ignored previously because we did not convert
the contents of lists.

This MR fixes this by treating torch tensors specially. I don't like
having a special case for a non-builtin class but given that
IsaacLab is heavily married with torch tensors I think it's ok in this
case.

Since currently the behavior is different between 1.2 and 1.3: can we cherry pick this change to the 1.3 branch?

Type of change

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

Checklist

  • 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

Up to v1.2.0 if a configclass would contain a list/tuple of torch
tensors it would be left as is.

\#1227 changed the behavior of converting lists/tuples in a dict, which
means that currently torch tensors are converted to an empty dict,
effectively losing all contained data.

The underlying issue is that `torch.tensor.__dict__` returns an empty
dict, which was (luckily) ignored previously because we did not convert
the contents of lists.

This MR fixes this by treating torch tensors specially. I don't like
having a special case for a non-builtin class but given that
IsaacLab is heavily married with torch tensors I think it's ok in this
case.
@kellyguo11 kellyguo11 changed the title Fix configclass dict conversion for torch tensors Fixes configclass dict conversion for torch tensors Dec 13, 2024
@kellyguo11 kellyguo11 merged commit c9f6ac5 into main Dec 13, 2024
4 of 5 checks passed
@kellyguo11 kellyguo11 deleted the lgulich/fix-configclass-dict-serialization branch December 13, 2024 04:47
@mschweig
Copy link

mschweig commented Dec 21, 2024

Hi @lgulich, @kellyguo11

this PR causes issues with rsl_rl/train.py and Boston Dynamics Spot joint_parameter_lookupwhich is torch.tensor

https://github.com/isaac-sim/IsaacLab/blob/main/source/extensions/omni.isaac.lab_assets/omni/isaac/lab_assets/spot.py

In this case torch.tensor.__dict__ does not return an empty dict and this works in 1.3.0.

This PR changes the behavior and returns the tensor object instead of converting obj.__dict__ to a dict.
But this implementation raises a omegaconf exception with rsl_rl train:

File "/home/manuel/miniconda3/envs/isaac/lib/python3.10/site-packages/omegaconf/nodes.py", line 154, in _validate_and_convert_impl raise UnsupportedValueType( omegaconf.errors.UnsupportedValueType: Value 'Tensor' is not a supported primitive type full_key: env.scene.robot.actuators.spot_knee.joint_parameter_lookup object_type=dict

I am quite new to Isaac Lab, however would be happy to help and fix this one.

@lgulich
Copy link
Contributor Author

lgulich commented Dec 25, 2024

Hi @mschweig

I'm happy to help. Any chance you could add a unit test in this file?

Then I can take a look to make sure that it's fixed.

@mschweig
Copy link

Hi @lgulich

sure, gonna add a dummy ActuatorCfg and a unittest that checks if the __dict__ conversion does not return an empty dict.

@mschweig
Copy link

Hi @lgulich

added a unittest and created a PR:

#1608

The test fails as I expect according to my understanding. Should I open a issue to track it?

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.

3 participants