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 MARL workflows for recording videos during training/inferencing #1596

Merged
merged 7 commits into from
Jan 3, 2025
Merged

Fixes MARL workflows for recording videos during training/inferencing #1596

merged 7 commits into from
Jan 3, 2025

Conversation

Rishi-V
Copy link
Contributor

@Rishi-V Rishi-V commented Dec 24, 2024 •
edited by kellyguo11
Loading

Description

Fixing bug so that using training workflow on MARL workflow populates videos/train.
See #1595

Type of change

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

Screenshots

before_and_after
The first run was without the changes where we see videos/train empty. The second run is after the changes with videos/train successfully populated.

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • [N/A] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [Sort of] I have added tests that prove my fix is effective or that my feature works; I have verified that it works on train.py for skrl and rl_games. I have not verified rsl_rl or sb3 as well have not verified play.py on any of the four. However I have implemented the changes on all of them as they all seem to follow the exact same structure.
  • 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; Unsure if this fix is worth being labelled as a contributor, if so would be happy to be added to the contributors.md (full name is Rishi Veerapaneni).

@kellyguo11 kellyguo11 changed the title Fixing bug so that using training workflow on MARL workflow populates videos/train Fixes MARL workflows for recording videos during training/inferencing Dec 25, 2024
@Rishi-V
Copy link
Contributor Author

Rishi-V commented Dec 27, 2024 •
edited
Loading

Update: I just reran my previous commands (after my "bug fix" changes) and for some reason am getting different buggy/erroring outcomes. I will test this more and get back to this.

@Rishi-V
Copy link
Contributor Author

Rishi-V commented Dec 29, 2024

The current version should work (i.e. videos/train gets populated) with rl_games and skrl.

I also additionally added rsl_rl_ppo_cfg.py and sb3_ppo_cfg.yaml as I think that others could reasonably want to try cart_double_pendulum with them (they have the same configurations as cartpole). However when I tried to run them they failed with different issues that I very briefly tried but could not easily fix. I have kept them with "FIXME" comments though for others.

kellyguo11
kellyguo11 reviewed Jan 3, 2025
View reviewed changes
...aac.lab_tasks/omni/isaac/lab_tasks/direct/marl_envs/agents/skrl_double_anymal_mappo_cfg.yaml Outdated
@@ -0,0 +1,80 @@
seed: 42
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this cfg for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry; that was for a different branch I was working on and shouldn't be there. I'll double check my branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed + I reviewed the other changes and it should finally be clean. Sorry for the sloppy commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

np, thanks for the fix!

@kellyguo11 kellyguo11 merged commit 7ea72c4 into isaac-sim:main Jan 3, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kellyguo11 kellyguo11 kellyguo11 left review comments

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

@Dhoeller19 Dhoeller19 Awaiting requested review from Dhoeller19

@Mayankm96 Mayankm96 Awaiting requested review from Mayankm96 Mayankm96 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.
2 participants
@Rishi-V @kellyguo11