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

chore: PPO system cleanup #1124

Merged
merged 25 commits into from
Dec 3, 2024
Merged

chore: PPO system cleanup #1124

merged 25 commits into from
Dec 3, 2024

Conversation

sash-a
Copy link
Contributor

@sash-a sash-a commented Nov 1, 2024

What?

Did a couple small clean ups to the PPO systems that are long overdue:

  • Removed all the capital comments
  • Removed opt states from the losses
  • Removed tree map around the done reshape during acting (it's just an array)
  • Removed metrics from the traj batch because they aren't really part of the batch
  • Cleaner unpacking of the loss metrics

@RuanJohn should we add this for MAT?

Copy link
Collaborator

@RuanJohn RuanJohn left a comment

Choose a reason for hiding this comment

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

Thank you @sash-a! I think these are some very needed changes. I just have a few suggestions and questions. Any comment applies to all systems.

mava/systems/mat/anakin/mat.py Show resolved Hide resolved
mava/systems/mat/anakin/mat.py Outdated Show resolved Hide resolved
mava/systems/mat/anakin/mat.py Show resolved Hide resolved
mava/systems/ppo/anakin/ff_ippo.py Outdated Show resolved Hide resolved
mava/systems/ppo/anakin/ff_ippo.py Outdated Show resolved Hide resolved
mava/systems/ppo/anakin/ff_mappo.py Show resolved Hide resolved
mava/systems/ppo/anakin/ff_mappo.py Show resolved Hide resolved
@sash-a
Copy link
Contributor Author

sash-a commented Nov 7, 2024

todo: same fixes for sable and switch sable to using the PPOTransition

README.md Show resolved Hide resolved
@OmaymaMahjoub OmaymaMahjoub added the benchmark required Docker images get pushed if PR has this label. label Nov 13, 2024
OmaymaMahjoub
OmaymaMahjoub previously approved these changes Nov 22, 2024
Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

LGTM! Let's just wait for the benchmark before we merge this 🙏

@OmaymaMahjoub OmaymaMahjoub added benchmark complete and removed benchmark required Docker images get pushed if PR has this label. labels Nov 22, 2024
RuanJohn
RuanJohn previously approved these changes Nov 26, 2024
Copy link
Collaborator

@RuanJohn RuanJohn left a comment

Choose a reason for hiding this comment

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

🏆

Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

Thanks @sash-a 🙌

Copy link
Collaborator

@RuanJohn RuanJohn left a comment

Choose a reason for hiding this comment

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

🌟

@RuanJohn RuanJohn merged commit 80554ef into develop Dec 3, 2024
4 checks passed
@RuanJohn RuanJohn deleted the chore/ppo-system-cleanup branch December 3, 2024 09:39
sash-a pushed a commit that referenced this pull request Dec 13, 2024
sash-a added a commit that referenced this pull request Jan 10, 2025
- chore: remove jax.tree_map
- Merge branch develop into chore/ppo-system-cleanup
- fix: update ret_output shape (#1147)
- Merge branch develop into chore/ppo-system-cleanup
- chore: minor comment changes
- fix: integration test workflow (#1145)
- Merge branch develop into chore/ppo-system-cleanup
- Merge branch develop into chore/ppo-system-cleanup
- chore: renaming loss_X -> X_loss
- fix: updated sebulba
- Merge branch develop into chore/ppo-system-cleanup
- chore: remove expired link to anakin notebook
- chore: add back advanced usage in examples
- Merge branch develop into chore/ppo-system-cleanup
- feat: sable cleanup
- chore: merge dev
- chore: unify value/critic loss naming
- refactor: loss_actor -> actor_loss
- Merge branch develop into chore/ppo-system-cleanup
- chore: pre-commit
- feat: mat system clean up
- chore: remove advanced usage
- Merge branch develop into chore/ppo-system-cleanup
- feat: ff mappo system clean up
- feat: ff ippo system clean up
- feat: rec mappo system clean up
- feat: rec ippo system clean up

Co-authored-by: Ruan de Kock <[email protected]>
Co-authored-by: Louay-Ben-nessir <[email protected]>
Co-authored-by: Arnol Fokam <[email protected]>
sash-a added a commit that referenced this pull request Jan 10, 2025
- chore: remove jax.tree_map
- Merge branch develop into chore/ppo-system-cleanup
- fix: update ret_output shape (#1147)
- Merge branch develop into chore/ppo-system-cleanup
- chore: minor comment changes
- fix: integration test workflow (#1145)
- Merge branch develop into chore/ppo-system-cleanup
- Merge branch develop into chore/ppo-system-cleanup
- chore: renaming loss_X -> X_loss
- fix: updated sebulba
- Merge branch develop into chore/ppo-system-cleanup
- chore: remove expired link to anakin notebook
- chore: add back advanced usage in examples
- Merge branch develop into chore/ppo-system-cleanup
- feat: sable cleanup
- chore: merge dev
- chore: unify value/critic loss naming
- refactor: loss_actor -> actor_loss
- Merge branch develop into chore/ppo-system-cleanup
- chore: pre-commit
- feat: mat system clean up
- chore: remove advanced usage
- Merge branch develop into chore/ppo-system-cleanup
- feat: ff mappo system clean up
- feat: ff ippo system clean up
- feat: rec mappo system clean up
- feat: rec ippo system clean up

Co-authored-by: Louay-Ben-nessir <[email protected]>
Co-authored-by: Arnol Fokam <[email protected]>
Co-authored-by: Ruan de Kock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants