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

Updatetemplate; Fix model slowness when using threading; Update WW3 for porting and threading issues #383

Merged
merged 36 commits into from
Mar 16, 2021
Merged

Updatetemplate; Fix model slowness when using threading; Update WW3 for porting and threading issues #383

merged 36 commits into from
Mar 16, 2021

Conversation

DeniseWorthen
Copy link
Collaborator

@DeniseWorthen DeniseWorthen commented Jan 18, 2021 •
edited by climbfuji
Loading

  • Updates PR template with added PR checklist
  • Fix model slowness when using threading #258
  • Updates WW3 to the latest develop branch, which has updates for threading (see Issue Fully coupled test case (cpld_bmarkfrac_wave_v16) not thread-safe #367). It also has updates for WW3 to be able to be build on gaea, jet and theoretically with gnu compilers although that has not been tested.

Dependent PRs:

FV3-atm: NOAA-EMC/fv3atm#258

RT logs:

  • hera.intel
  • hera.gnu
  • orion.intel
  • cheyenne.intel
  • cheyenne.gnu
  • gaea.intel
  • jet.intel
  • wcoss_cray
  • wcoss_dell_p3

@DeniseWorthen DeniseWorthen marked this pull request as ready for review January 18, 2021 12:58
climbfuji
climbfuji reviewed Jan 19, 2021
View reviewed changes
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I think we are becoming overly restrictive with the rules around issues. Certainly, there should be one issue for a set of PRs that describes the work, but requiring one for each submodule is unnecessary extra work. And how about small bug fixes that we often combine with other PRs to keep the commit queue shorter? A separate PR for each one-line change?

junwang-noaa
junwang-noaa reviewed Jan 19, 2021
View reviewed changes
.github/pull_request_template.md Outdated
# PR Checklist

- [ ] Ths 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. Please consult the ufs-weather-model [wiki](https://github.com/ufs-community/ufs-weather-model/wiki/Making-code-changes-in-the-UFS-weather-model-and-its-subcomponents) if you are unsure how to do this.
- [ ] An Issue describing the work contained in this PR has been created on both the ufs-weather-model and any relevant subcomponent repositories.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also feel maybe this is too strict to have issues in all the subcomponent repositories. E.g. anytime there is a CCPP update, we need to update fv3 to point to the new CCPP version (also update ufs-weather-model to point to new FV3), since CCPP has an issue already, we usually list the CCPP issue in the FV3 PR (or ufs-weather-model PR), otherwise we may have many FV3/ufs-weather-model issues just for just updating subcomponent version. I think we do need at least one issue for a set of PR, and an issue in subcomponent repository if there are scientific/technical changes in the repository.

@DeniseWorthen
Copy link
Collaborator Author

Thanks, I actually thought I was stating the current policy, not adding further restrictions. I myself have found it a bit of a pain to create issues in sub-components and then again in ufs-weather.

Is the first item (that the PR should be up-to-date w/ all components that are not being changed) OK?

For the second item, my preference is that the primary issue should be created on the subcomponent page; that one should describe what is being changed. It should have enough detail so that the code managers and reviewers are clear on what/why something is being done.

On the ufs-weather side, I'm not sure there needs to be an issue at all in a lot of cases---the PR can simply reference the existing issue in the subcomponent.

I'm happy to make those changes if that is closer to what makes sense to everyone.

@DeniseWorthen DeniseWorthen marked this pull request as draft January 25, 2021 13:09
@DeniseWorthen DeniseWorthen marked this pull request as ready for review February 21, 2021 15:16
@DeniseWorthen DeniseWorthen self-assigned this Feb 21, 2021
@DeniseWorthen DeniseWorthen added the No Baseline Change No Baseline Change label Feb 21, 2021
climbfuji
climbfuji reviewed Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Looks good to me, will approve once fv3atm is merged and submodule/.gitmodules is updated.

@climbfuji
Copy link
Collaborator

Cheyenne is done (both GNU and Intel). Waiting for orion it seems.

@junwang-noaa
Copy link
Collaborator

What is the status of orion RT? Are some jobs waiting in the queue? Is there any failed test?

@DeniseWorthen
Copy link
Collaborator Author

There are 15 jobs in the Q but all completed tests have passed.

@DeniseWorthen
Copy link
Collaborator Author

Two tests failed: fv3_ccpp_gfdlmprad and fv3_ccpp_stretched. The stretched test didn't run at all. I don't see the reason the gfdlmprad failed.

climbfuji
climbfuji approved these changes Mar 16, 2021
View reviewed changes
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Verified that the fv3atm submodule pointer update is correct. Thanks for including the change.

@DeniseWorthen DeniseWorthen merged commit 3be851d into ufs-community:develop Mar 16, 2021
This was referenced Mar 16, 2021
Closed
Closed
AnningCheng-NOAA added a commit to AnningCheng-NOAA/ufs-weather-model that referenced this pull request Mar 16, 2021
@AnningCheng-NOAA
Merge remote-tracking branch 'upstream/develop' into merra2
0c452b2 
* upstream/develop:
  Updatetemplate; Fix model slowness when using threading; Update WW3 for porting and threading issues (ufs-community#383)
  update MOM6 to GFDL 20210308 main branch commit (ufs-community#458)
  Regional inlinepost (ufs-community#364)
  correct benchmark diag_tables for coupled model configurations; move bm_ic directory out of inputdata directory; use aws ec2 for CI test; auto-rt fixes (ufs-community#426)
@DeniseWorthen DeniseWorthen deleted the updatetemplate branch March 16, 2021 15:56
@JessicaMeixner-NOAA JessicaMeixner-NOAA mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DusanJovic-NOAA DusanJovic-NOAA DusanJovic-NOAA approved these changes

@climbfuji climbfuji climbfuji approved these changes

@binli2337 binli2337 binli2337 approved these changes

@junwang-noaa junwang-noaa junwang-noaa approved these changes

@MinsukJi-NOAA MinsukJi-NOAA MinsukJi-NOAA approved these changes

@BrianCurtis-NOAA BrianCurtis-NOAA BrianCurtis-NOAA approved these changes

@JessicaMeixner-NOAA JessicaMeixner-NOAA Awaiting requested review from JessicaMeixner-NOAA

Assignees

@DeniseWorthen DeniseWorthen

Labels
No Baseline Change No Baseline Change Waiting for Reviews The PR is waiting for reviews from associated component PR's.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@DeniseWorthen @climbfuji @junwang-noaa @binli2337 @DusanJovic-NOAA @MinsukJi-NOAA @BrianCurtis-NOAA @JessicaMeixner-NOAA