From f8f9553d2a7cd2c24ffc6df4bf0afe353f4482b5 Mon Sep 17 00:00:00 2001 From: Jirka Borovec Date: Thu, 22 Oct 2020 17:42:07 +0200 Subject: [PATCH] Updated Review guidelines (markdown) --- Review-guidelines.md | 59 ++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/Review-guidelines.md b/Review-guidelines.md index 946d705..7ab8290 100644 --- a/Review-guidelines.md +++ b/Review-guidelines.md @@ -1,43 +1,60 @@ “Reviews allow for discussion of proposed changes and help ensure that the changes meet the repository’s contributing guidelines and other quality standards.“ (from GitHub documentation) +## How do I process own PRs + +* Decide if my PR is (a) ready to review or (b) I just need to complex testing of my contribution or (c) I want to share my work-in-progress and ask someone for help: + - (a) -> make it as regular PR + - (b,c) -> make it as Draft PR +* Make PR title reasonably short and sufficiently self-explainable so just after reading it anyone can get a basic idea of what it is about. +* Write short summary of your work, [link issue](https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue) which it fixes or closes, eventually what is missing to be done as a checkpoint-list. +* Add a label to classify the PR if it is fix or enhancement, logger to another part of code so reviewers can simpler take it according to their domain of expertise... +* Add milestone where it shall fit so we can put it to timeline, mainly distinguish if it goes to closes bugfix release 1.0.x or feature release 1.x.0 + +## General principles + +* **We value everyone putting time and effort into PyTorch Lightning, but not every change should be included.** +* We encourage everyone to open issues describing what they want to do before going off and doing it. +* Every feature we add takes attention and maintenance, but most importantly- might break something else. +* Discuss the tradeoffs in the PR or associated issue. + ## How do I review a PR? ### 1. Review the PR Description + - Follow the steps from self submitting PR above ^^. - Make sure that the PR title summarizes the change (not "fixes bug #2168"). - Make sure there's an issue associated with this PR. No issue associated? Look for the relevant issue or ask the author to create one. - Make sure PR has the right labels (bugfix, docs, tests, enhancements, etc.). - Make sure PR description includes: * A detailed summary of what changed * The motivation for the change -### 2. For new features or enhancements- ask yourself if we want this change in the product? -* We value everyone putting time and effort into PyTorch Lightning, but not every change should be included. -* We encourage everyone to open issues describing what they want to do before going off and doing it. -* Every feature we add takes attention and maintenance, but most importantly- might break something else. -* Discuss the tradeoffs in the PR or associated issue. -### 3. Make sure the PR's scope makes sense. + +### 2. Make sure the PR's scope makes sense. +* For new features or enhancements- ask yourself if we want this change in the product? * Refactors should always be in separate PRs than changes. -* Every PR should change/fix **exactly one thing**. Otherwise, ask the author to split to smaller PRs. -### 4. Verify **the fix solves the issue properly.** -* It is usually good practice to clone the branch and try it out yourself. -* Tests ar passing but something breaks when you try running locally? That means we are missing a test case. Ask author to add a test case/add it yourself. -### 5. Verify the code is **correct and readable**. +* Every PR should change/fix **exactly one thing**. Otherwise, ask the author to split into smaller PRs. +* Any code changes (may be skipped form typos in docs) both features and bug-fixes shall be cover by tests existing or newly added ones, If there is a bugfix and all tests were passing on master before and the PR does not add a new test to reveal the issue, ask the author to add it... + +### 3. Verify the code is **correct and readable**. * Code is read way more often than it is written or modified, so readability is always important. You should aim to understand every changed line. If you don't understand it, it's likely someone else won't either. Ask questions and comments for things you don't understand. -* Make sure error scenarios and edge cases are well handled. - - Would this work for single node? multi node? - - CPU? GPU? TPU? - - Anything with spawn needs to be serializable. Spawn is basically like: a car is driving down a single lane- this is the main process. When spawn is called, that car gets copied into 8 other cars (8 gpus). That car then goes into its own lane. The car is trained on that lane, but the original car isn’t... so consider what happens in those scenarios. - - DDP is like the same analogy but instead of copying the car 8 times, we copy it 7 times and the main car then joins those 7 to train like all. - - Make sure this works with 16 bit precision. * If you don’t think a variable name accurately represents what’s in it, suggest a better one. -* When making suggestions, mention that explicitly. +* Preferably use GH suggestion tooling which makes clear what and a=how you recommend to change. * When something **has** to change in the code, explain **what** should be changed and more importantly, **why** it should be changed (it is always useful to include links to external documentation/articles that provide more detailed information on the subject). -### 6. Verify the changes are **covered by tests**. + +### 4. Verify the changes are **covered by tests**. * Are there sufficient tests to ensure this change doesn’t break in the future? * Read the tests and make sure they will break if the code does. The code in the tests is just as important as the code in the product. * No tests? Ask the author to add them or add them yourself. -### 7. Check if the change is mentioned in **CHANGELOG** (no need for typos, docs and CI). +* Make sure error scenarios and edge cases are well handled. + - Would this work for a single node? multi-node? + - CPU? GPU? TPU? + - Anything with spawn needs to be serializable. _Spawn is basically like: a car is driving down a single lane- this is the main process. When spawn is called, that car gets copied into 8 other cars (8 GPUs). That car then goes into its own lane. The car is trained on that lane, but the original car isn’t... so consider what happens in those scenarios._ + - _DDP is like the same analogy but instead of copying the car 8 times, we copy it 7 times and the main car then joins those 7 to train like all._ + - Make sure this works with 16-bit precision. + +### 5. Check if the change is mentioned in **CHANGELOG** (no need for typos, docs, and CI). ## Approving PRs + If you checked all the above and everything is looking good, you may approve the PR in your review or request changes. As a rule of thumb: - Approve: if you are confident that the issue or feature is fully addressed and the content can be merged as it is now (no extra conditions in comments). @@ -47,7 +64,7 @@ As a rule of thumb: Remember that if you approve a PR and it reaches the minimum number of required approvals and given that all tests pass, the branch will be automatically merged into master. -## Reviewers are friends! +## Reviewers are our friends and partners! - **Be nice and constructive**, remember your fist PR and what helped you / made you motivated to continue contributing. - **Use suggestions** for simple edits when you know what it should be instead of leaving a long description (you can still add a short justification of your proposal) so the author can simply accept it.