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

Adds TIME macro to .testing/Makefile #1230

Merged
merged 2 commits into from
Oct 23, 2020
Merged

Adds TIME macro to .testing/Makefile #1230

merged 2 commits into from
Oct 23, 2020

Conversation

adcroft
Copy link
Collaborator

@adcroft adcroft commented Oct 22, 2020

This simply provides a means to avoid out-of-order output when running make test -j in parallel. By default we can get output like this:

PASS: Diagnostics tc4.dim.q.diag agree.
PASS: Diagnostics tc4.openmp.diag agree.
PASS: Diagnostics tc4.dim.r.diag agree.

real	0m16.012s
user	0m0.129s
sys	0m0.268s

real	0m15.761s
user	0m0.116s
sys	0m0.318s

real	0m15.940s
user	0m0.112s
sys	0m0.302s

real	0m15.705s
user	0m0.177s
sys	0m0.251s
DONE: tc2.a.dim.l; no runtime errors.
DONE: tc2.a.nan; no runtime errors.
DONE: tc2.dim.z; no runtime errors.

but by adding TIME = to config.mk or invoking in bash with TIME="" make test -j the times are omitted and output cleaner.

This simply provides a means to avoid out-of-order output when running
`make test -j` in parallel. By default we can get output like this:

```
PASS: Diagnostics tc4.dim.q.diag agree.
PASS: Diagnostics tc4.openmp.diag agree.
PASS: Diagnostics tc4.dim.r.diag agree.

real	0m16.012s
user	0m0.129s
sys	0m0.268s

real	0m15.761s
user	0m0.116s
sys	0m0.318s

real	0m15.940s
user	0m0.112s
sys	0m0.302s

real	0m15.705s
user	0m0.177s
sys	0m0.251s
DONE: tc2.a.dim.l; no runtime errors.
DONE: tc2.a.nan; no runtime errors.
DONE: tc2.dim.z; no runtime errors.
```

but by adding `TIME =` to config.mk or invoking in bash
with `TIME="" make test -j` the times are omitted and
output cleaner.
@adcroft adcroft requested a review from marshallward October 22, 2020 21:14
@codecov-io
Copy link

Codecov Report

Merging #1230 into dev/gfdl will increase coverage by 0.04%.
The diff coverage is 33.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1230      +/-   ##
============================================
+ Coverage     46.08%   46.13%   +0.04%     
============================================
  Files           214      224      +10     
  Lines         69399    71022    +1623     
============================================
+ Hits          31984    32763     +779     
- Misses        37415    38259     +844     
Impacted Files Coverage Δ
...g_src/external/GFDL_ocean_BGC/FMS_coupler_util.F90 0.00% <0.00%> (ø)
...fig_src/external/GFDL_ocean_BGC/generic_tracer.F90 0.00% <0.00%> (ø)
...c/external/GFDL_ocean_BGC/generic_tracer_utils.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/kdtree.f90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_core.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_types.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/write_ocean_obs.F90 0.00% <0.00%> (ø)
config_src/solo_driver/MESO_surface_forcing.F90 0.00% <0.00%> (ø)
config_src/solo_driver/user_surface_forcing.F90 0.00% <0.00%> (ø)
src/ALE/MOM_regridding.F90 31.47% <0.00%> (-0.17%) ⬇️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f0e8e9...107077f. Read the comment docs.

@marshallward
Copy link
Collaborator

I almost wonder if TIME should be unset on default, partly for the reasons that you outlined (ambiguity of results when run in parallel).

But this is an incrementally positive step and we can disable it later if there is any feedback to do so.

(NOTE: This does not touch code so does not require a regression test.)

@marshallward marshallward merged commit e12cb3d into mom-ocean:dev/gfdl Oct 23, 2020
@adcroft adcroft deleted the testing-makefile-tidy-time branch November 9, 2020 23:11
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.

3 participants