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

[Proposal] Compositional Configuration via Dataclasses (Hydra, etc.) #76

Closed
1 task done
romesco opened this issue Apr 24, 2023 · 6 comments
Closed
1 task done
Assignees
Labels
enhancement New feature or request

Comments

@romesco
Copy link
Contributor

romesco commented Apr 24, 2023

Proposal

Orbit has already made thoughtful design decisions that support really nice UX/UI paradigms for configuration management. Showing a few use-cases of how this might be done would be beneficial for the community as it can be difficult to decide "how to start". We would like to have one or two best-practices examples that fully leverage configuration via dataclasses. Preferably, one which makes no commitment to configuration framework, and one which demonstrates a a few tweaks that can allow Hydra to be used at its most powerful capacity.

Pitch

We'll provide an example that shows how to set up an environment(robot / scene), RL algorithm, and training script and configure all of this via dataclasses.

We'll then show how to extend this configuration paradigm via some key features of Hydra.

Alternatives

We can also look into what benefits other configuration frameworks have (one such being tyro), as long as they use dataclasses as the principle data structure.

Additional context

#57 and #67 is a related issue/PR pair, and it is a good start! We will build on this and provide more use-cases that leverage Hydra and are clean / easy to understand. See some of the previous discussion in #67 .

Checklist

  • I have checked that there is no similar issue in the repo (required)
@romesco
Copy link
Contributor Author

romesco commented Apr 24, 2023

Following this proposal, I'll update with a working draft in PRs which we can look over and provide comments on.

@Mayankm96
Copy link
Contributor

Thanks a lot for this! @romesco

Looking forward to your proposal :)

@Mayankm96 Mayankm96 added the enhancement New feature or request label Apr 24, 2023
@Mayankm96
Copy link
Contributor

Adding reference to this issue IGE #127 on auto-completion when using Hydra. Seems pretty handy.

@Mayankm96 Mayankm96 linked a pull request Apr 26, 2023 that will close this issue
5 tasks
@Mayankm96
Copy link
Contributor

Hi @romesco, I am wondering if there any updates on the plan with Hydra integration :)

@romesco
Copy link
Contributor Author

romesco commented Jul 5, 2023

Thanks for the tag! I have something I'll push this weekend! Sorry about the tardiness, I had conference travel and then a deadline!

Mayankm96 added a commit that referenced this issue Jul 24, 2023
…sent (#76)

# Description

Previously, type annotation was always required to make the terms follow
the order in which they are defined in the configclass. If this was not
done, then the terms were getting sorted alphabetically which made it
different from the expected behavior (user-defined order).

On further inspection, turned out that in our wrappers for configclass,
we were using `dir(cls)` to parse the class members, which sorts all the
members of the class alphabetically. Changing it to `cls.__dict__` fixed
this issue since in Python 3.7 onwards, dictionaries follow the
user-defined ordering.

Since this behavior changes the way config terms are parsed, the old
configclass still exists inside the
`omni.isaac.orbit.compat.utils.configclass` module so that people can
still run policies trained with the old ordering.

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- Breaking change (fix or feature that would cause existing
functionality to not work as expected)

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
Mayankm96 added a commit that referenced this issue Dec 22, 2023
…sent (#76)

# Description

Previously, type annotation was always required to make the terms follow
the order in which they are defined in the configclass. If this was not
done, then the terms were getting sorted alphabetically which made it
different from the expected behavior (user-defined order).

On further inspection, turned out that in our wrappers for configclass,
we were using `dir(cls)` to parse the class members, which sorts all the
members of the class alphabetically. Changing it to `cls.__dict__` fixed
this issue since in Python 3.7 onwards, dictionaries follow the
user-defined ordering.

Since this behavior changes the way config terms are parsed, the old
configclass still exists inside the
`omni.isaac.orbit.compat.utils.configclass` module so that people can
still run policies trained with the old ordering.

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- Breaking change (fix or feature that would cause existing
functionality to not work as expected)

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
@Mayankm96
Copy link
Contributor

Hi, since the number of issues has been rising, we decided to go over the backlogs and remove the stale ones.

We did an initial Hydra integration, as you already saw in: #700

You had some great suggestions there for which it would be interesting to adapt the code. If you have the availability, please feel free to submit a new issue so that we can start working more actively. Thank you for the support! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants