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

Jump to bottom

Add pull request template for Github #915

Merged
merged 4 commits into from
Dec 2, 2022
Merged

Add pull request template for Github #915

merged 4 commits into from
Dec 2, 2022

Conversation

karl-richter
Copy link
Collaborator

@karl-richter karl-richter commented Oct 27, 2022 •
edited
Loading

🔬 Background

  • Why is this change needed? Is there a related issue or a new feature to be added?

🔮 Key changes

  • Explain the main changes introduced by this pull request for the reviewer.

📋 Review Checklist

  • I have performed a self-review of my own code.
  • I have commented my code, added docstrings and data types to function definitions.
  • I have added pytests to check whether my feature / fix works.

Please make sure to follow our best practices in the Contributing guidelines.

@karl-richter karl-richter added the type:docs Improvements or additions to documentation label Oct 27, 2022
@karl-richter
Copy link
Collaborator Author

@ourownstory @noxan @Kevin-Chen0 what do you think, should we add a pr template like this to better structure the pr review? (the icons are debatable)

@github-actions
Copy link

github-actions bot commented Oct 27, 2022 •
edited
Loading

917c180

Model Benchmark

Benchmark Metric main current diff
AirPassengers SmoothL1Loss 0.00031 0.00032 1.79% ✅
AirPassengers MAE 6.35364 6.37421 0.32% ✅
AirPassengers RMSE 7.68085 7.75532 0.97% ✅
AirPassengers Loss 0.00023 0.00023 1.55% ✅
AirPassengers RegLoss 0 0 0.0% ✅
AirPassengers SmoothL1Loss_val 0.06051 0.06031 -0.32% ✅
AirPassengers MAE_val 85.1099 84.9838 -0.15% ✅
AirPassengers RMSE_val 108.276 108.103 -0.16% ✅
PeytonManning SmoothL1Loss 0.00587 0.00587 0.0% ✅
PeytonManning MAE 0.34839 0.34839 0.0% ✅
PeytonManning RMSE 0.48617 0.48617 0.0% ✅
PeytonManning Loss 0.00464 0.00464 0.0% ✅
PeytonManning RegLoss 0 0 0.0% ✅
PeytonManning SmoothL1Loss_val 0.03038 0.03038 -0.0% ✅
PeytonManning MAE_val 0.92518 0.92518 -0.0% ✅
PeytonManning RMSE_val 1.13074 1.13074 -0.0% ✅
YosemiteTemps SmoothL1Loss 0.00086 0.00086 -0.01% ✅
YosemiteTemps MAE 1.43672 1.43794 0.09% ✅
YosemiteTemps RMSE 2.14749 2.14874 0.06% ✅
YosemiteTemps Loss 0.00064 0.00064 -0.0% ✅
YosemiteTemps RegLoss 0 0 0.0% ✅
YosemiteTemps SmoothL1Loss_val 0.00097 0.00096 -0.81% ✅
YosemiteTemps MAE_val 1.71173 1.70236 -0.55% ✅
YosemiteTemps RMSE_val 2.2758 2.26615 -0.42% ✅

Model Training

PeytonManning

YosemiteTemps

AirPassengers

CML watermark

@karl-richter karl-richter self-assigned this Oct 27, 2022
@noxan
Copy link
Collaborator

noxan commented Oct 29, 2022 •
edited
Loading

@karl-richter I like the idea very much, great initiative. A bit of feedback and open ended questions from my side:

  1. I wouldn't know what to put under status quo - I mean in contrast to what I write in key changes -> maybe I didn't get it, otherwise I'd rather remove this section
  2. The list of checks sounds nice. Would propose to reduce this list to the actual actionable items. Possibly some general guide we could link in contribution instructions might be great (e.g. passing github actions, code formatting, etc)
  3. Not exactly related to this pull request, yet kind of in line with your original idea to improve PR reviews: is there any way to reduce the number of automated posts (performance, codecov, etc) on each pull request, make the content of them collapsible or link to somewhere else? I feel like it's quite hard to see the actual review discussion with that much "noise" now part of [dev-ops] Collapsible metrics reports #926
  4. In line with point 3, does this codecov check add any value?

Happy to follow up, like your initiative :)

@noxan noxan self-requested a review October 29, 2022 22:18
@noxan noxan changed the title added pr template Add pull request template for Github Nov 1, 2022
@karl-richter karl-richter added the status: needs review PR needs to be reviewed by Reviewer(s) label Nov 3, 2022
@Kevin-Chen0
Copy link
Collaborator

@karl-richter I like the idea very much, great initiative. A bit of feedback and open ended questions from my side:

  1. I wouldn't know what to put under status quo - I mean in contrast to what I write in key changes -> maybe I didn't get it, otherwise I'd rather remove this section
  2. The list of checks sounds nice. Would propose to reduce this list to the actual actionable items. Possibly some general guide we could link in contribution instructions might be great (e.g. passing github actions, code formatting, etc)
  3. Not exactly related to this pull request, yet kind of in line with your original idea to improve PR reviews: is there any way to reduce the number of automated posts (performance, codecov, etc) on each pull request, make the content of them collapsible or link to somewhere else? I feel like it's quite hard to see the actual review discussion with that much "noise" now part of [dev-ops] Collapsible metrics reports #926
  4. In line with point 3, does this codecov check add any value?

Happy to follow up, like your initiative :)

I agree with Richard, and propose the following:

  • Rename Status Quo to something like Issue, which detail the underlying issue or problem that the PR is trying to fix. If there is already a linked Issue, then you can simply write "See Development". This can be an optional section.

  • Key Changes is fine.

  • Add the following below to the Review Checklist (if you think it is needed):

  • I have resolved any merge conflicts.

  • I have tagged at least one Reviewer and selected a sprint milestone.

  • The Status label is set to "Status: Needs Reviewer" or "Status: Ready".

@Kevin-Chen0 Kevin-Chen0 added this to the Release 0.5.1 milestone Nov 22, 2022
@Kevin-Chen0 Kevin-Chen0 added status: needs update PR has outstanding comment(s) or PR test(s) that need to be resolved and removed status: needs review PR needs to be reviewed by Reviewer(s) labels Nov 22, 2022
@github-actions
Copy link

github-actions bot commented Nov 22, 2022 •
edited
Loading

c7663c8

Model Benchmark

Benchmark Metric main current diff
YosemiteTemps MAE_val 1.72949 1.72949 0.0% ✅
YosemiteTemps RMSE_val 2.27386 2.27386 0.0% ✅
YosemiteTemps Loss_val 0.00096 0.00096 0.0% ✅
YosemiteTemps RegLoss_val 0 0 0.0% ✅
YosemiteTemps epoch 84 84 0.0% ✅
YosemiteTemps MAE 1.45189 1.45189 0.0% ✅
YosemiteTemps RMSE 2.16631 2.16631 0.0% ✅
YosemiteTemps Loss 0.00066 0.00066 0.0% ✅
YosemiteTemps RegLoss 0 0 0.0% ✅
PeytonManning MAE_val 0.64636 0.64636 0.0% ✅
PeytonManning RMSE_val 0.79276 0.79276 0.0% ✅
PeytonManning Loss_val 0.01494 0.01494 0.0% ✅
PeytonManning RegLoss_val 0 0 0.0% ✅
PeytonManning epoch 37 37 0.0% ✅
PeytonManning MAE 0.42701 0.42701 0.0% ✅
PeytonManning RMSE 0.57032 0.57032 0.0% ✅
PeytonManning Loss 0.00635 0.00635 0.0% ✅
PeytonManning RegLoss 0 0 0.0% ✅
AirPassengers MAE_val 15.2698 15.2698 0.0% ✅
AirPassengers RMSE_val 19.4209 19.4209 0.0% ✅
AirPassengers Loss_val 0.00195 0.00195 0.0% ✅
AirPassengers RegLoss_val 0 0 0.0% ✅
AirPassengers epoch 89 89 0.0% ✅
AirPassengers MAE 9.82902 9.82902 0.0% ✅
AirPassengers RMSE 11.7005 11.7005 0.0% ✅
AirPassengers Loss 0.00056 0.00056 0.0% ✅
AirPassengers RegLoss 0 0 0.0% ✅
Model training plots

Model Training

PeytonManning

YosemiteTemps

AirPassengers

@codecov-commenter
Copy link

Codecov Report

Merging #915 (5ffcb09) into main (f0d45f3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #915   +/-   ##
=======================================
  Coverage   87.68%   87.68%           
=======================================
  Files          17       17           
  Lines        4433     4433           
=======================================
  Hits         3887     3887           
  Misses        546      546

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

noxan
noxan requested changes Nov 24, 2022
View reviewed changes
Copy link
Collaborator

@noxan noxan left a comment

Choose a reason for hiding this comment

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

@karl-richter just marking this one as changes requested based on kevin's feedback

@karl-richter
Copy link
Collaborator Author

@karl-richter I like the idea very much, great initiative. A bit of feedback and open ended questions from my side:

  1. I wouldn't know what to put under status quo - I mean in contrast to what I write in key changes -> maybe I didn't get it, otherwise I'd rather remove this section
  2. The list of checks sounds nice. Would propose to reduce this list to the actual actionable items. Possibly some general guide we could link in contribution instructions might be great (e.g. passing github actions, code formatting, etc)
  3. Not exactly related to this pull request, yet kind of in line with your original idea to improve PR reviews: is there any way to reduce the number of automated posts (performance, codecov, etc) on each pull request, make the content of them collapsible or link to somewhere else? I feel like it's quite hard to see the actual review discussion with that much "noise" now part of [dev-ops] Collapsible metrics reports #926
  4. In line with point 3, does this codecov check add any value?

Happy to follow up, like your initiative :)

I agree with Richard, and propose the following:

  • Rename Status Quo to something like Issue, which detail the underlying issue or problem that the PR is trying to fix. If there is already a linked Issue, then you can simply write "See Development". This can be an optional section.
  • Key Changes is fine.
  • Add the following below to the Review Checklist (if you think it is needed):
  • I have resolved any merge conflicts.
  • I have tagged at least one Reviewer and selected a sprint milestone.
  • The Status label is set to "Status: Needs Reviewer" or "Status: Ready".

Thanks for your feedback! I adopted the first part, but Richard and I decided to not add the info with the status label and milestone since only the dev-core team has the rights to do so.

@noxan noxan self-requested a review December 2, 2022 05:45
@noxan noxan merged commit f8b889f into main Dec 2, 2022
@noxan noxan deleted the docs/pr_template branch December 2, 2022 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noxan noxan noxan approved these changes

@Kevin-Chen0 Kevin-Chen0 Awaiting requested review from Kevin-Chen0

@ourownstory ourownstory Awaiting requested review from ourownstory

Assignees

@karl-richter karl-richter

Labels
status: needs update PR has outstanding comment(s) or PR test(s) that need to be resolved type:docs Improvements or additions to documentation
Projects
None yet
Milestone
Release 0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@karl-richter @noxan @Kevin-Chen0 @codecov-commenter