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

Revise PR template. #10774

Merged
merged 8 commits into from
Jul 22, 2022
Merged

Revise PR template. #10774

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a4a754
Add minimal PR template checklist.
bdice May 3, 2022
c6f31fb
Add helpful note about draft PRs.
bdice May 3, 2022
845588f
Minimize.
bdice May 3, 2022
729ee77
Remove colon.
bdice May 3, 2022
b19822e
Reword.
bdice May 3, 2022
abf98f1
Rephrase description.
bdice May 3, 2022
2cf2704
Update note about CHANGELOG.
bdice May 3, 2022
ae5c1e7
Merge remote-tracking branch 'upstream/branch-22.08' into revise-pr-t…
bdice May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 6 additions & 51 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,7 @@
<!--
## Description
<!-- Describe the changes. Link to any issues closed by this pull request. -->
bdice marked this conversation as resolved.
Show resolved Hide resolved

Thank you for contributing to cuDF :)

Here are some guidelines to help the review process go smoothly.

1. Please write a description in this text box of the changes that are being
made.

2. Please ensure that you have written units tests for the changes made/features
added.

3. There are CI checks in place to enforce that committed code follows our style
and syntax standards. Please see our contribution guide in `CONTRIBUTING.MD`
in the project root for more information about the checks we perform and how
you can run them locally.

4. If you are closing an issue please use one of the automatic closing words as
noted here: https://help.github.com/articles/closing-issues-using-keywords/

5. If your pull request is not ready for review but you want to make use of the
continuous integration testing facilities please mark your pull request as Draft.
https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft

6. If your pull request is ready to be reviewed without requiring additional
work on top of it, then remove it from "Draft" and make it "Ready for Review".
https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review

If assistance is required to complete the functionality, for example when the
C/C++ code of a feature is complete but Python bindings are still required,
then add the label `help wanted` so that others can triage and assist.
The additional changes then can be implemented on top of the same PR.
If the assistance is done by members of the rapidsAI team, then no
additional actions are required by the creator of the original PR for this,
otherwise the original author of the PR needs to give permission to the
person(s) assisting to commit to their personal fork of the project. If that
doesn't happen then a new PR based on the code of the original PR can be
opened by the person assisting, which then will be the PR that will be
merged.

7. Once all work has been done and review has taken place please do not add
features or make changes out of the scope of those requested by the reviewer
(doing this just add delays as already reviewed code ends up having to be
re-reviewed/it is hard to tell what is new etc!). Further, please do not
rebase your branch on the target branch, force push, or rewrite history.
Doing any of these causes the context of any comments made by reviewers to be lost.
If conflicts occur against the target branch they should be resolved by
merging the target branch into the branch used for making the pull request.
Comment on lines -41 to -48
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the advice arguably could be ported into CONTRIBUTING.md. This seems like useful workflow information for contributors (especially new ones). IIUC, cudf uses squash-merges for PRs, so fixing up commits in response to review is not necessary for a clean commit history (and, as noted here, can hinder in the case of multiple rounds of reivew).

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, my previous experience just led me to do rebase/fixups of PRs I recently opened in UCX-Py, but that made reviewing harder, and the squash-merge meant that in some senses it was pointless at the end of the day anyway.

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 considered moving this in the CONTRIBUTING guide but wasn't sure where to put it, or whether it could be condensed. Would you put it in this list? Below the list? https://github.com/rapidsai/cudf/blob/branch-22.06/CONTRIBUTING.md#your-first-issue Overall, the "workflow" information in the existing template felt outdated and heavy-handed in some cases.

Several developers (including myself) make use of amending commits, force-pushing, and rebasing to keep PRs clean for review, even though squash-merging reduces the benefit. I haven't personally experienced comments being lost from rebasing in the last few months, so it's possible that GitHub has improved its handling of these situations. I would be happy to hear from others if that's not the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if it's outdated then there's no real reason to include it. I agree that github seems to be better than a few years ago at handling this kind of code movement (I don't recall having comments resolved by rebase and github losing track for a while).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I think about this more, this suggests an opportunity to rework some of the CONTRIBUTING guide. The differentiation between "Your First Issue" and "Seasoned Developers" is irrelevant and a little presumptive. It'd be clearer to write something like "Development workflow" and "Project planning and organization", and incorporate tips about draft PRs, etc. To keep this focused, I will create a separate PR for the CONTRIBUTING guide before resolving this thread.

Copy link
Member

@harrism harrism May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/50132325/github-advises-force-pushing-can-corrupt-your-pull-request-why

Comments are not lost when you force push, but they can be disassociated from the code when the commit they refer to is lost. You can no longer see the code they are referring to when you click on the file link at the top of the comment.

I believe it would also be possible to lose changes from other contributors or changes made via the github UI (e.g. via code review suggestions).

This is why I recommended we have a rule against force pushing once a PR is under review. I have had force pushes disrupt reviews multiple times. It's especially bad with large PRs where the reviewer may take multiple passes to review. I believe the rule/suggestion should continue to be documented.

As the above linked SO answer suggests,

When you really need to do a force push, you can always create a new pull request, which is much safer and keeps away annoying problems

Copy link
Member

Choose a reason for hiding this comment

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

I wish these templates and docs were RAPIDS-wide rather than specific to cuDF.


Many thanks in advance for your cooperation!

-->
## Checklist
- [ ] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md).
- [ ] New or existing tests cover these changes.
- [ ] The documentation is up to date with these changes.
Comment on lines +6 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

Two additional things came to mind. Feel free to push back on these ideas.

Suggested change
## Checklist
- [ ] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md).
- [ ] New or existing tests cover these changes.
- [ ] The documentation is up to date with these changes.
## Checklist
- [ ] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md).
- [ ] New or existing tests cover these changes.
- [ ] New or existing benchmarks cover these changes.
- [ ] The documentation is up to date with these changes.
- This change is:
- [ ] Breaking
- [ ] Non-breaking

Copy link
Contributor Author

@bdice bdice May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I support mentioning benchmarks in principle -- with the caveat that C++ has the best benchmarking at present, the Python benchmarks are somewhat outdated, and no Java benchmarks exist to my knowledge. I wasn't certain what to do with that.

Breaking/non-breaking is already handled by labels, and the CI requires us to label one or the other. I don't think it is necessary to input that information twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO tests are going to be much more frequent additions than benchmarks (the latter is typically for new features)

Copy link
Contributor

@karthikeyann karthikeyann May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we already have a breaking/non-breaking label requirement for PRs. But it's nice to have it part of template too.
Same comment for category of the PR, (it's hard right now to find which labels are categories), that could be checklist point too.

Copy link
Contributor Author

@bdice bdice May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would strongly discourage duplicating information in the template and labels. The overhead of every checkbox / button adds up. However, I do agree that our categories are hard to locate. This is the official reference, but I don't think it's linked anywhere in the cuDF repository: https://docs.rapids.ai/resources/label-checker/#summary

My preferred solution would be if the CI Label Checker output message said "Missing category label (bug, doc, feature request, improvement)".

Though on second thought, why is "feature request" a supported pull request category? Sounds like it should only apply to issues...

Copy link
Contributor Author

@bdice bdice May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I like this idea. category: doc is longer, but it's a lot easier to distinguish the special category labels. However, the bot's label checker would need to be adapted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not suggesting to include "category" in label itself (also a good idea). Just in description text of the label. That is enough for search to filter "category" when we type "category".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! The description would be a good compromise because it doesn’t require changing bot logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

could @rapidsai/admins or @rapidsai/cudf-admin update this?
add "category" to the description text of the following labels: bug, doc, feature request, improvement.
This allows easily identifying the category labels during PR creation, by simply typing "category" in label box.

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’d still like to see the word “category” added to those labels’ descriptions but that can be done separately from this PR.

bdice marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ conduct. More information can be found at:
describes your planned work. For example, `fix-documentation`.
5. Write code to address the issue or implement the feature.
6. Add unit tests and unit benchmarks.
7. [Create your pull request](https://github.com/rapidsai/cudf/compare).
7. [Create your pull request](https://github.com/rapidsai/cudf/compare). To run continuous integration (CI) tests without requesting review, open a draft pull request.
8. Verify that CI passes all [status checks](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks).
Fix if needed.
9. Wait for other developers to review your code and update code as needed.
Expand Down