Skip to content

Add rnd_ppo.py documentation and refactor#151

Merged
vwxyzjn merged 40 commits intomasterfrom
rnd-doc
Aug 25, 2022
Merged

Add rnd_ppo.py documentation and refactor#151
vwxyzjn merged 40 commits intomasterfrom
rnd-doc

Conversation

@yooceii
Copy link
Copy Markdown
Collaborator

@yooceii yooceii commented Apr 3, 2022

Description

Closes #127

Types of changes

  • Bug fix
  • New feature
  • New algorithm
  • Documentation

Checklist:

  • I've read the CONTRIBUTION guide (required).
  • I have ensured pre-commit run --all-files passes (required).
  • I have updated the documentation and previewed the changes via mkdocs serve.
  • I have updated the tests accordingly (if applicable).

If you are adding new algorithms or your change could result in performance difference, you may need to (re-)run tracked experiments. See #137 as an example PR.

  • I have contacted @vwxyzjn to obtain access to the openrlbenchmark W&B team (required).
  • I have tracked applicable experiments in openrlbenchmark/cleanrl with --capture-video flag toggled on (required).
  • I have added additional documentation and previewed the changes via mkdocs serve.
    • I have explained note-worthy implementation details.
    • I have explained the logged metrics.
    • I have added links to the original paper and related papers (if applicable).
    • I have added links to the PR related to the algorithm.
    • I have created a table comparing my results against those from reputable sources (i.e., the original paper or other reference implementation).
    • I have added the learning curves (in PNG format with width=500 and height=300).
    • I have added links to the tracked experiments.
  • I have updated the tests accordingly (if applicable).

@yooceii yooceii linked an issue Apr 3, 2022 that may be closed by this pull request
28 tasks
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 3, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vwxyzjn/cleanrl/FVfp6xKi7pTtnaXPFTa7dhPRKnqL
✅ Preview: https://cleanrl-git-rnd-doc-vwxyzjn.vercel.app

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Apr 3, 2022

@vercel
Copy link
Copy Markdown

vercel bot commented Jun 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cleanrl ✅ Ready (Inspect) Visit Preview Aug 25, 2022 at 3:56AM (UTC)

@vwxyzjn
Copy link
Copy Markdown
Owner

vwxyzjn commented Jun 26, 2022

Hey, @yooceii would you mind reverting the formatting change? They make it harder to review and identify the code specifically relating to RND.

image

Formatting change should be done in a larger PR in #167 together...

…cessary CPU-GPU synchronization."

This reverts commit 98e0161.
@vwxyzjn
Copy link
Copy Markdown
Owner

vwxyzjn commented Aug 24, 2022

Refactor Check (compatible with the performance in the tracked experiment)

I compared the SPS performance of the latest refactor against the old script used in the tracked experiment. I named the old one old_ppo_rnd_envpool.py and ran it and the latest script without initializing observation normalization parameter.

The following screenshot confirms the refactor does not result in a performance difference (and that we did the refactor correctly — making it faster without impacting sample efficiency)

image

Copy link
Copy Markdown
Owner

@vwxyzjn vwxyzjn 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 @yooceii! There is one last thing. Could you also document the meaning of losses/fwd_loss in the docs? Otherwise, everything LGTM. The comment above also confirms the refactor was only beneficial (reducing GPU memory, making the script about 50%-100% faster)

Copy link
Copy Markdown
Owner

@vwxyzjn vwxyzjn left a comment

Choose a reason for hiding this comment

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

Thanks for the great work @yooceii!

@vwxyzjn vwxyzjn merged commit 0284d74 into master Aug 25, 2022
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.

Add rnd_ppo.py documentation and refactor

2 participants