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.

Sign up for GitHub

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

Jump to bottom

Fixes configclass dict conversion for torch tensors #1530

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Fixes configclass dict conversion for torch tensors #1530

merged 3 commits into from
Dec 13, 2024

Conversation

lgulich
Copy link
Contributor

@lgulich lgulich commented Dec 12, 2024 •
edited
Loading

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

@lgulich
Fix configclass dict conversion for torch tensors
fa2df64 
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
Contributor

mschweig commented Dec 21, 2024 •
edited
Loading

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
Contributor

Hi @lgulich

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

@mschweig mschweig mentioned this pull request Dec 28, 2024
Merged
6 tasks
@mschweig
Copy link
Contributor

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?

kellyguo11 pushed a commit that referenced this pull request Jan 13, 2025
@mschweig
Adds dict conversion test for ActuatorBase configs (#1608)
9ab52d4 
# Description

<!--
Thank you for your interest in sending a pull request. Please make sure
to check the contribution guidelines.

Link: https://isaac-sim.github.io/IsaacLab/source/refs/contributing.html
-->

A recent PR
(#1530 (comment))
causes issues with joint_parameter_lookup attributes of Articulation
Configs. The `class_to_dict` utility function needs to return a
primitive dict. Returning a torch.Tensor conflicts with OmegaConf.

This PR adds a unittest for the dict conversion as requested by @lgulich
so that the issue can be fixed. The test case fails on main branch as
expected according to my understanding. Passes on v1.3.0.

<!-- As a practice, it is recommended to open an issue to have
discussions on the proposed pull request.
This makes it easier for the community to keep track of what is being
developed or added, and if a given feature
is demanded by more than one party. -->

## Type of change

<!-- As you go through the list, delete the ones that are not
applicable. -->

- Breaking change (fix or feature that would cause existing
functionality to not work as expected)

<!--
Example:

| Before | After |
| ------ | ----- |
| _gif/png before_ | _gif/png after_ |

To upload images to a PR -- simply drag and drop an image while in edit
mode and it should upload the image directly. You can then paste that
source into the above before/after sections.
-->

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) 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
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

<!--
As you go through the checklist above, you can mark something as done by
putting an x character in it

For example,
- [x] I have done this task
- [ ] I have not done this task
-->
AntoineRichard pushed a commit to AntoineRichard/IsaacLab that referenced this pull request Jan 15, 2025
@mschweig @AntoineRichard
Adds dict conversion test for ActuatorBase configs (isaac-sim#1608)
113c574 
# Description

<!--
Thank you for your interest in sending a pull request. Please make sure
to check the contribution guidelines.

Link: https://isaac-sim.github.io/IsaacLab/source/refs/contributing.html
-->

A recent PR
(isaac-sim#1530 (comment))
causes issues with joint_parameter_lookup attributes of Articulation
Configs. The `class_to_dict` utility function needs to return a
primitive dict. Returning a torch.Tensor conflicts with OmegaConf.

This PR adds a unittest for the dict conversion as requested by @lgulich
so that the issue can be fixed. The test case fails on main branch as
expected according to my understanding. Passes on v1.3.0.

<!-- As a practice, it is recommended to open an issue to have
discussions on the proposed pull request.
This makes it easier for the community to keep track of what is being
developed or added, and if a given feature
is demanded by more than one party. -->

## Type of change

<!-- As you go through the list, delete the ones that are not
applicable. -->

- Breaking change (fix or feature that would cause existing
functionality to not work as expected)

<!--
Example:

| Before | After |
| ------ | ----- |
| _gif/png before_ | _gif/png after_ |

To upload images to a PR -- simply drag and drop an image while in edit
mode and it should upload the image directly. You can then paste that
source into the above before/after sections.
-->

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) 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
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

<!--
As you go through the checklist above, you can mark something as done by
putting an x character in it

For example,
- [x] I have done this task
- [ ] I have not done this task
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mayankm96 Mayankm96 Awaiting requested review from Mayankm96 Mayankm96 is a code owner

@jsmith-bdai jsmith-bdai Awaiting requested review from jsmith-bdai jsmith-bdai is a code owner

@Dhoeller19 Dhoeller19 Awaiting requested review from Dhoeller19

@kellyguo11 kellyguo11 Awaiting requested review from kellyguo11 kellyguo11 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lgulich @mschweig @kellyguo11