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.

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

.testing: Fix concurrency errors in tc4 rules #255

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

marshallward
Copy link
Member

This patch fixes two issues in the preprocessing of tc4.

  • ocean_hgrid.nc is marked as a dependency of gen_data
  • Multiple ouputs are handled more safely in the Makefile

Expanding on the second point: We were directing one rule to produce two output files, which resulted in the rule being run twice when invoked in parallel (make -j).

This has been replaced with the recommended solution for handling concurrent outputs: use one to generate both, and connect the second to the first with a separate rule.

I have also generalized the make command in the .testing Makefile.

This should address (and hopefully fix) some intermittent errors in the .testing build on Gaea.

This patch fixes two issues in the preprocessing of tc4.

* ocean_hgrid.nc is marked as a dependency of gen_data
* Multiple ouputs are handled more safely in the Makefile

Expanding on the second point: We were directing one rule to produce two
output files, which resulted in the rule being run twice when invoked in
parallel (make -j).

This has been replaced with the recommended solution for handling
concurrent outputs: use one to generate both, and connect the second to
the first with a separate rule.

I have also generalized the `make` command in the .testing Makefile.

This should address (and hopefully fix) some intermittent errors in the
.testing build on Gaea.
@marshallward
Copy link
Member Author

marshallward commented Nov 24, 2022

This is equivalent to #254 but also includes the second ocean_hgrid.nc dependency fix. GitHub wouldn't let me reuse the other PR due to the force-push.

@marshallward
Copy link
Member Author

marshallward commented Nov 24, 2022

I ran 100 tests and it didn't fail. Contrast to #254 which failed about 1 in 10 times for me.

Either way, the race condition on ocean_hgrid.nc should have been obvious in retrospect.

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #255 (dd9cda9) into dev/gfdl (321e0eb) will not change coverage.
The diff coverage is n/a.

❗ Current head dd9cda9 differs from pull request most recent head 7a962e5. Consider uploading reports for the commit 7a962e5 to get more accurate results

@@            Coverage Diff            @@
##           dev/gfdl     #255   +/-   ##
=========================================
  Coverage     37.08%   37.08%           
=========================================
  Files           263      263           
  Lines         73388    73388           
  Branches      13677    13677           
=========================================
  Hits          27219    27219           
  Misses        41136    41136           
  Partials       5033     5033           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

This PR looks reasonable too, and it has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/17528.

@Hallberg-NOAA Hallberg-NOAA merged commit 571ddd9 into NOAA-GFDL:dev/gfdl Nov 25, 2022
@marshallward marshallward deleted the tc4_rule_fixes branch February 20, 2023 16:20
marshallward pushed a commit that referenced this pull request Oct 25, 2023
* Makes set_u_at_v and set_v_at_u public

* First draft for fpmix

* Change name of logical

Replaces LU_pred to L_diag, since now this logical only controls
if diagnostics should be posted.

* Updates to vertFPmix

This commit adds the latest updates to the vertFPmix subroutine
after Bill Large did some cleaning. We have highlight places in
the code where work must be done.

* Add missing use for vertFPmix

* Add omega_w2x to fluxes and forces

omega_w2x is the counter-clockwise angle of the wind stress with
respect to the horizontal abscissa (x-coordinate) at tracer
points [rad]. This variable is needed in the vertPFmix subroutine.

* Add mssing call to get_param for FPMIX

This line of code was lost during the last merge.

* Pass wavebands from coupler to wave_parameters_CS

This commit passes the waveband information recieved
from the coupler to wave_parameters_CS. This information
is set to public so that it can be used elsewhere. To
exercise this code the following must be set:

SURFBAND = COUPLER
WAVE_METHOD = SURFACE_BANDS

No answer changes.

* Describe local variables and make code consistent

* Removed L_diag and moved variables in vertFPmix

* Revert order of variables in vertFPmix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants