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

Update PR template organization, adding commit message and priority section. #2053

Closed
wants to merge 6 commits into from
Closed

Update PR template organization, adding commit message and priority section. #2053

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
315f27d
Initial Updates to template
BrianCurtis-NOAA Dec 19, 2023
d2e1e08
Fix type for checkboxes and add instructions section to pr template
BrianCurtis-NOAA Dec 19, 2023
243690f
Cheyenne->Derecho, update GNU machines in template
BrianCurtis-NOAA Dec 19, 2023
0ca12c0
Updates taking into account most suggestions up to this point
BrianCurtis-NOAA Dec 19, 2023
4779ed4
More changes from PR discussion
BrianCurtis-NOAA Dec 19, 2023
a6180c4
Merge branch 'ufs-community:develop' into template_tuneup
BrianCurtis-NOAA Dec 20, 2023
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
76 changes: 41 additions & 35 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,48 @@
<!-- THE FOLLOWING IS FOR THE PR AUTHOR TO FILL OUT
PLEASE DO NOT MODIFY THE TEMPLATE BEYOND FILLING OUT THE PROPER SECTIONS -->
## PR Author Checklist:
<!-- Please complete all items in list. -->
- [ ] I have linked PR's from all sub-components involved in section below. <!-- PLEASE DO NOT LINK SUBCOMPONENT ISSUES -->
- [ ] I am confirming reviews are completed in ALL sub-component PR's.
- [ ] I have run the full RT suite on either Hera/Hercules AND have attached the log to this PR below this line:
- LOG:
- [ ] I have added the list of all failed regression tests to "Anticipated changes" section.
- [ ] I have filled out all sections of the template.

## Description
<!-- INSTRUCTIONS:
- Please fill out all sections of this PR and complete the checklist below
- Please be as descriptive as possible, this is really important.
- Please "fill in" checkboxes. Use [X] for a filled in checkbox or leave it [ ] for an empty checkbox
- Please use github markup as much as possible in linking
i.e.:
* Linking to UFSWM PR's and issues add "- #<pr/issue number>"
BrianCurtis-NOAA marked this conversation as resolved.
Show resolved Hide resolved
* Linking to a subcomponent PR and issues add "- <Group>/<Fork>/pull/#" or "-<Group>/<Fork>/issues/#"
-->
## Commit Queue Requirements:
BrianCurtis-NOAA marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Fill out all sections of this template.
- [ ] All sub component pull requests have been reviewed by their code managers.
- [ ] Run the full RT suite (compared to current baselines) on either Hera/Derecho/Hercules AND have committed the log to my PR branch.
BrianCurtis-NOAA marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Add list of all failed regression tests in "Regression Tests" section.
Copy link
Collaborator

@DeniseWorthen DeniseWorthen Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- [ ] Add list of all failed regression tests in "Regression Tests" section.
- [ ] Add a list of all failed regression tests in the Regression Tests section. If tests fail, I have confirmed that *only* those tests which are expected to fail do in fact fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BrianCurtis-NOAA if we have rt.sh updated with capability of running full test and commit the log file, then we don't need to have this item here, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that would be a change we could bring in with the PR that would make the rt.sh changes.

Copy link
Collaborator

@DeniseWorthen DeniseWorthen Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the step to commit the logs could be automated. But I specifically want developers to verify that only the tests they expected to fail do in fact fail. It should not be up to CMs to go through the list and verify that.

If we automate the pre-test, can we add a synopsis that clarifies
X tests failed comparison,
Y tests ran to completion (if X ne Y, then note a large message would be added)
Z compilations were completed (if Z ne expected number of compiles, then an large message added).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally the PR authors should provide 'new_baselines' file which contains the list of tests that need new baselines, and that file can then be used (by both developer(s) and code managers) to generate new baselines and verify against them. That is more useful than a log file. See #1834.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be a mistake not to emphasize that developers must know (and report) that only the expected tests fail. They can do with a log, or maybe in a 'new baseline' file. But I've seen too many cases where logs are posted and it seems that nobody looked at them----tests didn't compile, they didn't run, or totally unexpected ones failed.


## PR Information

### Description
<!-- Provide a detailed description of what this PR does in the space provided below-->

### Commit Message
<!-- Please provide concise information about the changes in this PR to be used as a commit message for the commit history -->
BrianCurtis-NOAA marked this conversation as resolved.
Show resolved Hide resolved

## Linked Issues and Pull Requests
### Associated UFSWM Issue to close
<!-- Example: "- Closes #1698" -->
### Priority
- [ ] HIGH
- [ ] Medium
- [ ] low
BrianCurtis-NOAA marked this conversation as resolved.
Show resolved Hide resolved

### Blocking Dependencies
<!-- If there are any PR's that are needed to be completed before this one, please add links
to them here -->

### Subcomponent Pull Requests
<!-- format: - <community>/<repo>/pull/<PR number> i.e.: - NOAA-EMC/fv3atm/pull/33 or "None" -->
### Git Issues Fixed By This PR
<!-- Example: "- Closes #1698" or "- Closes NOAA-EMC/fv3atm/issues/729" -->
DeniseWorthen marked this conversation as resolved.
Show resolved Hide resolved


### Blocking Dependencies
<!-- Example: "- Depends on #1733" or "None" -->
## Changes


### Subcomponents involved:
### Subcomponent (with links)
<!-- (add links to subcomponent PR's here) -->
<!-- Example:
[X] FV3
- NOAA-EMC/fv3atm/pull/734
- NOAA-EMC/fv3atm/pull/735
-->
- [ ] AQM
- [ ] CDEPS
- [ ] CICE
Expand All @@ -41,7 +57,6 @@ PLEASE DO NOT MODIFY THE TEMPLATE BEYOND FILLING OUT THE PROPER SECTIONS -->
- [ ] stochastic_physics
- [ ] none

## Anticipated Changes
### Input data
- [ ] No changes are expected to input data.
- [ ] Changes are expected to input data:
Expand All @@ -51,9 +66,8 @@ PLEASE DO NOT MODIFY THE TEMPLATE BEYOND FILLING OUT THE PROPER SECTIONS -->
### Regression Tests:
- [ ] No changes are expected to any regression test.
- [ ] Changes are expected to the following tests:
<!-- Please insert what RT's change and why you expect them to change in the space provided below -->
<details><summary>Tests effected by changes in this PR:</summary>
<!-- ADD ITEMS HERE or add "None" -->
<details><summary>FAILED REGRESSION TESTS</summary>
<!-- List failed regression tests here or add "None" -->

</details>

Expand All @@ -64,14 +78,7 @@ PLEASE DO NOT MODIFY THE TEMPLATE BEYOND FILLING OUT THE PROPER SECTIONS -->
- [ ] Create separate issue in [JCSDA/spack-stack](https://github.com/JCSDA/spack-stack) asking for update to library. Include library name, library version.
- [ ] Add issue link from JCSDA/spack-stack following this item <!-- for example: "- JCSDA/spack-stack/issue/1757" -->


<!-- THE FOLLOWING IS FOR CODE MANAGERS ONLY DO NOT FILL OUT -->
<details><summary>Code Managers Log</summary>

- [ ] This PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR.
- [ ] Move new/updated input data on RDHPCS Hera and propagate input data changes to all supported systems.
- [ ] N/A

<!-- STOP!!! THE FOLLOWING IS FOR CODE MANAGERS ONLY. PLEASE DO NOT FILL OUT -->
### Testing Log:
- RDHPCS
- [ ] Hera
Expand All @@ -87,5 +94,4 @@ PLEASE DO NOT MODIFY THE TEMPLATE BEYOND FILLING OUT THE PROPER SECTIONS -->
- [ ] Completed
- opnReqTest
- [ ] N/A
- [ ] Log attached to comment
</details>
- [ ] Log attached to comment