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

Friendlier CONTRIBUTING.md #117

Closed
Tracked by #115
vwxyzjn opened this issue Feb 21, 2022 · 4 comments
Closed
Tracked by #115

Friendlier CONTRIBUTING.md #117

vwxyzjn opened this issue Feb 21, 2022 · 4 comments
Assignees

Comments

@vwxyzjn
Copy link
Owner

vwxyzjn commented Feb 21, 2022

We should make contribution guidelines clearer. We got the feedback that "Although the current project seems welcoming to new developers, there is little concrete information on how to get involved. For example, what code style does the project follow? What are the criteria for including a new algorithm: what documentation does it need to have, test cases, etc? There is little in the way of API docs either, this admittedly is not critical as the files are themselves quite readable, but I'd suggest adding at least one-line docstrings to classes, methods, etc (and consider splitting up the scripts into more separate methods as well)"

This is great feedback. To address it, let's re-think the checklist for including a new algorithm. Such a checklist should include:

  1. pre-commit utilities: sort dependencies, remove unused variables and imports, format code using black, and check word spelling Introduce pre-commit pipelines #107.
  2. Empirical analysis and benchmark: we should adopt a similar guide from sb3-contrib with a bit of our spin. The implemented algorithm should come with tracked experiments that
    • match the reported performance in the paper (if applicable)
    • match the reported performance in a high-quality reference implementation (SB3, Tianshou, and others) (if applicable).
    • We should also add documentation on how exactly we want the tracked experiments to be done (i.e., what W&B project? should they capture video recording?)
  3. Documentation: the proposed algorithm should also come with documentation at https://docs.cleanrl.dev/rl-algorithms/ to
    • explain crucial implementation details
    • add links to the original paper and related papers (if applicable)
    • add links to the PR related to the algorithm
    • add links to the tracked experiments and benchmark results.
  4. Tests: the proposed algorithm should come with an e2e test that makes sure the algorithm does not crash.

I will try to make some examples next week.

@vwxyzjn vwxyzjn mentioned this issue Feb 21, 2022
19 tasks
@vwxyzjn
Copy link
Owner Author

vwxyzjn commented Feb 22, 2022

Maybe we should add a contribution template similar to SB3's template:

Checklist:

  • I've read the CONTRIBUTION guide (required).
  • I have ensured pre-commit run --all-files passes (required).
  • I have contacted @vwxyzjn to obtain access to the cleanrl W&B team (required).
  • I have tracked applicable experiments in cleanrl/benchmark with --capture-video flag toggled on (required).
  • I have updated the documentation accordingly.
    • I have explained note-worthy implementation details.
    • 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.
    • I have added links to the tracked experiments.
  • I have updated the tests accordingly (if applicable).

@vwxyzjn vwxyzjn mentioned this issue Feb 22, 2022
10 tasks
@vwxyzjn vwxyzjn changed the title More friendly CONTRIBUTING.md Friendlier CONTRIBUTING.md Feb 22, 2022
@yooceii
Copy link
Collaborator

yooceii commented Feb 28, 2022

Maybe also add I've run and corrected the file based on code style maybe google style by pylint and yapf
The 4th subcheck in 5th check should be a must.

@vwxyzjn
Copy link
Owner Author

vwxyzjn commented Feb 28, 2022

The 4th subcheck in 5th check should be a must.

👍

Maybe also add I've run and corrected the file based on code style maybe google style by pylint and yapf

Right now we have already adapted black as the code style, which conflicts with yapf.

Pylint seems doable. I have also noticed SB3 uses flake8 to do the lining. Maybe we can open a separate issue to evaluate these two options.

@vwxyzjn
Copy link
Owner Author

vwxyzjn commented Apr 10, 2022

Closed by #154

@vwxyzjn vwxyzjn closed this as completed Apr 10, 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

No branches or pull requests

3 participants