Revise PR template. #10774

bdice · 2022-05-03T13:27:17Z

Description

I recently revamped our cuDF CONTRIBUTING guide. I would like to consider replacing the current PR template (which has a fairly daunting amount of text that is immediately deleted by many contributors) with a short checklist of actionable items and a reference to the CONTRIBUTING guide for longer content.

I kept this draft very minimal. Reviewers can see other examples here for inspiration: https://axolo.co/blog/p/part-3-github-pull-request-template. Happy to crowdsource others' thoughts here.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

jrhemstad · 2022-05-03T14:16:51Z

.github/PULL_REQUEST_TEMPLATE.md

+## 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.


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

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.

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

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.

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...

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.

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".

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

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.

I’d still like to see the word “category” added to those labels’ descriptions but that can be done separately from this PR.

wence-

Could possibly port some of the culled text into contributing?

wence- · 2022-05-03T14:14:10Z

.github/PULL_REQUEST_TEMPLATE.md

-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.


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).

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.

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.

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).

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.

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

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

.github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Jake Hemstad <[email protected]>

bdice · 2022-05-03T21:06:51Z

@jrhemstad @shwina @wence- @karthikeyann Thank you very much for your thoughtful input! I opened this PR for review. I have two action items in mind, outside the scope of this PR:

Update the CONTRIBUTING guide with some of the lost information from this PR template
Work with @karthikeyann and the ops team to adapt the bot's squash commit messages to filter out the checklist

For now, I'm seeking approval on the language of the PR template. I will hold off on merging this until we have the aforementioned items complete (or in review).

.github/PULL_REQUEST_TEMPLATE.md

wence-

I think the proposed changes look good. If wanting to include information about benchmark/test coverage perhaps the contributing guide should say how to check? I think codecov will comment on code that is not covered by the tests, but I don't know how to check that benchmarks cover what I add (and I did look!)

karthikeyann

LGTM 👍

karthikeyann · 2022-05-06T13:17:57Z

I don't know how to check that benchmarks cover what I add (and I did look!)

benchmarks are not run as part of CI right now.

…emplate

github-actions · 2022-06-22T14:06:41Z

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

ajschmidt8 · 2022-07-12T19:24:46Z

FYI, I just merged @karthikeyann's PR below. Those changes are now deployed and incorporated into the auto-merger.

extract description from PR body text ops-bot#103

ajschmidt8

Approved now that rapidsai/ops-bot#103 is merged and deployed.

karthikeyann · 2022-07-13T19:09:25Z

rerun tests

codecov · 2022-07-13T21:01:44Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@0e74fca). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08   #10774   +/-   ##
===============================================
  Coverage                ?   86.31%           
===============================================
  Files                   ?      144           
  Lines                   ?    22748           
  Branches                ?        0           
===============================================
  Hits                    ?    19636           
  Misses                  ?     3112           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e74fca...ae5c1e7. Read the comment docs.

bdice

I'm going to merge this, since it has 4 approvals. I will leave the adjacent conversations open, which are not in scope for this PR.

bdice · 2022-07-22T21:12:51Z

@gpucibot merge

This PR simplifies the Pull request template to be identical to the new simpler template used by cuDF, adding in #rapidsai/cudf#10774 Closes #602 Authors: - Mark Harris (https://github.com/harrism) Approvers: - AJ Schmidt (https://github.com/ajschmidt8) - Bradley Dice (https://github.com/bdice) URL: #617

Simplifies PR template to match recent updates for cuDF, cuSpatial. See rapidsai/cudf#10774, rapidsai/cuspatial#617. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Mark Harris (https://github.com/harrism) - Jake Hemstad (https://github.com/jrhemstad) - AJ Schmidt (https://github.com/ajschmidt8) URL: #1080

bdice added 5 commits May 3, 2022 08:14

Add minimal PR template checklist.

3a4a754

Add helpful note about draft PRs.

c6f31fb

Minimize.

845588f

Remove colon.

729ee77

Reword.

b19822e

bdice self-assigned this May 3, 2022

bdice mentioned this pull request May 3, 2022

Mention 2 cpp-reviewer requirement in pull request template #10768

Merged

bdice added doc Documentation non-breaking Non-breaking change labels May 3, 2022

jrhemstad reviewed May 3, 2022

View reviewed changes

wence- reviewed May 3, 2022

View reviewed changes

jrhemstad reviewed May 3, 2022

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

Rephrase description.

abf98f1

Co-authored-by: Jake Hemstad <[email protected]>

karthikeyann mentioned this pull request May 3, 2022

strip PR template in merge commit message rapidsai/ops-bot#102

Closed

Update note about CHANGELOG.

2cf2704

bdice marked this pull request as ready for review May 3, 2022 21:02

bdice requested a review from a team as a code owner May 3, 2022 21:02

bdice added the 5 - DO NOT MERGE Hold off on merging; see PR for details label May 3, 2022

shwina reviewed May 3, 2022

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved

wence- approved these changes May 5, 2022

View reviewed changes

jrhemstad approved these changes May 5, 2022

View reviewed changes

karthikeyann approved these changes May 6, 2022

View reviewed changes

bdice changed the base branch from branch-22.06 to branch-22.08 May 23, 2022 14:04

Merge remote-tracking branch 'upstream/branch-22.08' into revise-pr-t…

ae5c1e7

…emplate

github-actions bot added the inactive-30d label Jun 22, 2022

jrhemstad approved these changes Jul 8, 2022

View reviewed changes

ajschmidt8 approved these changes Jul 12, 2022

View reviewed changes

github-actions bot removed the inactive-30d label Jul 12, 2022

wence- approved these changes Jul 13, 2022

View reviewed changes

bdice commented Jul 22, 2022

View reviewed changes

bdice removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jul 22, 2022

rapids-bot bot merged commit 3099c68 into rapidsai:branch-22.08 Jul 22, 2022

This was referenced Jul 25, 2022

[DOC] Update and improve PR template rapidsai/cuspatial#602

Closed

[DOC] contributing.md should point readers to relevant developer guides #11348

Closed

harrism mentioned this pull request Aug 2, 2022

Simplify PR template rapidsai/cuspatial#617

Merged

3 tasks

bdice mentioned this pull request Aug 3, 2022

Simplify PR template. rapidsai/rmm#1080

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise PR template. #10774

Revise PR template. #10774

bdice commented May 3, 2022

jrhemstad May 3, 2022

bdice May 3, 2022 •

edited

Loading

shwina May 3, 2022

karthikeyann May 3, 2022 •

edited

Loading

bdice May 3, 2022 •

edited

Loading

bdice May 3, 2022 •

edited

Loading

karthikeyann May 6, 2022

bdice May 6, 2022

karthikeyann May 6, 2022

bdice Jul 8, 2022

wence- left a comment

wence- May 3, 2022

wence- May 3, 2022

bdice May 3, 2022

wence- May 3, 2022

bdice May 3, 2022

harrism May 10, 2022 •

edited

Loading

harrism May 10, 2022

bdice commented May 3, 2022

wence- left a comment

karthikeyann left a comment

karthikeyann commented May 6, 2022

github-actions bot commented Jun 22, 2022

ajschmidt8 commented Jul 12, 2022 •

edited

Loading

ajschmidt8 left a comment

karthikeyann commented Jul 13, 2022

codecov bot commented Jul 13, 2022 •

edited

Loading

bdice left a comment

bdice commented Jul 22, 2022

Revise PR template. #10774

Revise PR template. #10774

Conversation

bdice commented May 3, 2022

Description

Checklist

Choose a reason for hiding this comment

bdice May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann May 3, 2022 • edited Loading

Choose a reason for hiding this comment

bdice May 3, 2022 • edited Loading

Choose a reason for hiding this comment

bdice May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice commented May 3, 2022

wence- left a comment

Choose a reason for hiding this comment

karthikeyann left a comment

Choose a reason for hiding this comment

karthikeyann commented May 6, 2022

github-actions bot commented Jun 22, 2022

ajschmidt8 commented Jul 12, 2022 • edited Loading

ajschmidt8 left a comment

Choose a reason for hiding this comment

karthikeyann commented Jul 13, 2022

codecov bot commented Jul 13, 2022 • edited Loading

Codecov Report

bdice left a comment

Choose a reason for hiding this comment

bdice commented Jul 22, 2022

bdice May 3, 2022 •

edited

Loading

karthikeyann May 3, 2022 •

edited

Loading

bdice May 3, 2022 •

edited

Loading

bdice May 3, 2022 •

edited

Loading

harrism May 10, 2022 •

edited

Loading

ajschmidt8 commented Jul 12, 2022 •

edited

Loading

codecov bot commented Jul 13, 2022 •

edited

Loading