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

BrianCurtis-NOAA · 2023-12-19T02:01:05Z

Commit Queue Requirements:

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.
Add list of all failed regression tests in "Regression Tests" section.

PR Information

Description

Commit Message

Priority

Bugfix (This PR contains a bug fix and should be prioritized.)
High (This PR contains a feature or fix needed for a time-sensitive project (eg, retrospectives, implementations))
Normal

Prioriry Reasoning

Blocking Dependencies

Git Issues Fixed By This PR

Changes

Subcomponent (with links)

Input data

No changes are expected to input data.
Changes are expected to input data:
- New input data.
- Updated input data.

Regression Tests:

No changes are expected to any regression test.
Changes are expected to the following tests:

FAILED REGRESSION TESTS

Libraries

Not Needed
Needed
- Create separate issue in JCSDA/spack-stack asking for update to library. Include library name, library version.
- Add issue link from JCSDA/spack-stack following this item

Testing Log:

RDHPCS
- Hera
- Orion
- Hercules
- Jet
- Gaea
- Derecho
WCOSS2
- Dogwood/Cactus
- Acorn
CI
- Completed
opnReqTest
- N/A
- Log attached to comment

<!-- 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>
* Linking to a subcomponent PR and issues add - <Group>/<Fork>/pull/<number> or - <Group>/<Fork>/issues/<number>
-->
## Commit Queue Requirements:
<!-- Please note: PRs will be scheduled in the Commit Queue in the order received and only after all pre-requisite testing is complete and all PR requirements (e.g. Issues created and noted, Subcomponent PRs reviewed and accepted) are met. -->
- [ ] 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.
- [ ] Add list of all failed regression tests in "Regression Tests" section.

## PR Information

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

### Commit Message
<!--
Please provide the following concise information:
Description of all changes - 1 line
Please list all individual issue titles addressed with github links at the end in perenthesis (using #<number> or <group>/<fork>/issues/<number>).
-->


### Priority
- [ ] Bugfix (This PR contains a bug fix and should be prioritized.)
- [ ] High (This PR contains a feature or fix needed for a time-sensitive project (eg, retrospectives, implementations))
- [ ] Normal
#### Prioriry Reasoning
<!-- Provide reasoning for your priority above. This is required for Bugfix and High priority items. -->

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

### Git Issues Fixed By This PR
<!-- Example: - Closes #1698  or - Closes NOAA-EMC/fv3atm/issues/729 -->


## Changes

### 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
- [ ] CMEPS
- [ ] CMakeModules
- [ ] FV3
- [ ] GOCART
- [ ] HYCOM
- [ ] MOM6
- [ ] NOAHMP
- [ ] WW3
- [ ] stochastic_physics
- [ ] none

### Input data
- [ ] No changes are expected to input data.
- [ ] Changes are expected to input data:
  - [ ] New input data.
  - [ ] Updated input data.

### Regression Tests:
- [ ] No changes are expected to any regression test.
- [ ] Changes are expected to the following tests:
<details><summary>FAILED REGRESSION TESTS</summary>
<!-- List failed regression tests here or add "None" -->

</details>

### Libraries
<!-- Library updates take time. If this PR needs updates to libraries, please make sure to accomplish the following tasks -->
- [ ] Not Needed
- [ ] Needed
  - [ ] 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" -->

<!-- STOP!!! THE FOLLOWING IS FOR CODE MANAGERS ONLY. PLEASE DO NOT FILL OUT -->
### Testing Log:
- RDHPCS
  - [ ] Hera
  - [ ] Orion
  - [ ] Hercules
  - [ ] Jet
  - [ ] Gaea
  - [ ] Derecho
- WCOSS2
  - [ ] Dogwood/Cactus
  - [ ] Acorn
- CI
  - [ ] Completed
- opnReqTest
  - [ ] N/A
  - [ ] Log attached to comment

BrianCurtis-NOAA · 2023-12-19T02:15:20Z

@jkbk2004 @DeniseWorthen @junwang-noaa @FernandoAndrade-NOAA @zach1221

Please look this through and let me know if there's anything I'm missing or a change needed to make this work for everyone. I've taken into account what I've heard from others, so please do add any input from what you're hearing.

FernandoAndrade-NOAA · 2023-12-19T02:26:28Z

It looks like Cheyenne is still being referenced on lines 13 and 89

BrianCurtis-NOAA · 2023-12-19T02:31:31Z

It looks like Cheyenne is still being referenced on lines 13 and 89

Right, confirming, it's Hera, Derecho and Hercules that will support the GNU tests (soon)?

.github/pull_request_template.md

DeniseWorthen · 2023-12-19T13:23:44Z

.github/pull_request_template.md

+- [ ] 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.
+- [ ] Add list of all failed regression tests in "Regression Tests" section.


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.

@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?

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

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

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.

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.

.github/pull_request_template.md

jkbk2004 · 2023-12-19T13:47:47Z

I have to note that a lot of priority PRs have been requested without evidence of meeting all the criteria.

BrianCurtis-NOAA · 2023-12-19T14:05:10Z

I have to note that a lot of priority PRs have been requested without evidence of meeting all the criteria.

It's an interesting issue to have, isn't it. With UFS applications moving rapidly into operations, this might become more of a norm where you have a large sum of priority PR's but can't ignore the normal priority PR's. Do you try to fit in a normal priority PR once a week, once every 2 weeks, once a month?

BrianCurtis-NOAA · 2023-12-19T14:10:56Z

I have to note that a lot of priority PRs have been requested without evidence of meeting all the criteria.

It's an interesting issue to have, isn't it. With UFS applications moving rapidly into operations, this might become more of a norm where you have a large sum of priority PR's but can't ignore the normal priority PR's. Do you try to fit in a normal priority PR once a week, once every 2 weeks, once a month?

I read your comment wrong, but what I said still holds. High priority should only be for implementations/retros, and bugfix has to be something that impacts the current dev branch negatively. (wrong answers, typically)

jkbk2004 · 2023-12-19T14:14:17Z

I have to note that a lot of priority PRs have been requested without evidence of meeting all the criteria.

It's an interesting issue to have, isn't it. With UFS applications moving rapidly into operations, this might become more of a norm where you have a large sum of priority PR's but can't ignore the normal priority PR's. Do you try to fit in a normal priority PR once a week, once every 2 weeks, once a month?

I read your comment wrong, but what I said still holds. High priority should only be for implementations/retros, and bugfix has to be something that impacts the current dev branch negatively. (wrong answers, typically)

I understand. I think it will get better when we cut daily baseline workload and reprioritize and start using weekly and monthly stable baseline runs.

DeniseWorthen · 2023-12-19T14:47:45Z

We don't really have a list of requirements that constitute "priority". I think wanting to start some set of runs is not the same as having allocated CPU resources that start on a specific date, or needing a change completed in order to finalize a configuration for an implementation.

BrianCurtis-NOAA · 2023-12-19T18:56:35Z

I've updated with most (if not all) of your suggestions. Please let me know of anything else!

.github/pull_request_template.md

DeniseWorthen · 2023-12-19T18:59:36Z

.github/pull_request_template.md

+- [ ] High (This PR contains a feature or fix needed for a time-sensitive project (eg, retrospectives, implementations))
+- [ ] Normal
+#### Prioriry Reasoning
+<!-- Provide reasoning for your priority above. This is required for Bugfix and High priority items. -->


Does "bugfix" really need a reason? Why would we not want to fix bugs as a priority?

Maybe the word "justification" might be better instead of "reasoning"?

Again, I can think of no justification for not prioritizing a bug fix.

OK but let's say you have 5 bugfix PR's, they're all classified at the top of the commit queue automatically. Maybe with reasoning, one of them ends up needing the highest of highest priorities.

@DeniseWorthen I think that it's possible most items coming into PR's for UFSWM are considered a bug fix (Look at #2043 ). I think we should make sure that only critical bug fixes should be prioritized. I am OK removing the "justification", as long as I can change "Bugfix" to "Critical Bugfix".

.github/pull_request_template.md

…nodi + #2047, #2053, and #2056 (#2044) FV3 diagnostic fixes, CCPP fixes for model crashes, new PR template - UFS: - commit message in PR template (#2053) - fix hercules crashes (#2015) - CMEPS & FV3: Bad data from in CCPP CLM Lake physics scheme caused model crashes - Communicate changes to lake ice (Closes #2055, NOAA-EMC/CMEPS#105, NOAA-EMC/fv3atm#741) - unit mismatch (NOAA-EMC/fv3atm#736) - FV3: correct errors in diagnostic calculations - snodi had weasdi data in it (NOAA-EMC/fv3atm#736) - revisions to RUC LSM snowfall melting and accumulation (NOAA-EMC/fv3atm#739)

BrianCurtis-NOAA added 2 commits December 19, 2023 01:55

Initial Updates to template

315f27d

Fix type for checkboxes and add instructions section to pr template

d2e1e08

Cheyenne->Derecho, update GNU machines in template

243690f