search

mom-ocean / MOM5

The Modular Ocean Model
https://mom-ocean.github.io/
GNU Lesser General Public License v3.0
82 stars 95 forks source link

Use ACCESS-NRI fork of generic tracers with `accessom_coupler` #391

Closed dougiesquire closed 2 months ago

dougiesquire commented 3 months ago

See #388 for context and details.

Note, this PR includes the ACCESS-NRI fork of generic_tracers as a git submodule, but we could instead use git sub-tree if that is preferred.

Note also, this PR removes the need for the ACCESS-OM-BGC build type (and hence the CSIRO_BGC pp def). The ACCESS-OM build type now always builds with the USE_OCEAN_BGC pp def and generic tracer WOMBAT can be configured at runtime. I’ve verified that always building with USE_OCEAN_BGC doesn’t impact performance when generic tracers are not configured. To do this, I ran two ACCESS-OM2 experiments on the NCI Gadi supercomputer:

I ran three years of each. The experiments were run at the same time. The chksum outputs in the logs are identical between the experiments and the walltimes reported by PBS for each year are given in the table below.

  Exp 1 Exp 2
year 1 959 s 908 s
year 2 1031 s 1002 s
year 3 1148 s 1028 s
average 1046 s 979 s

Closes #388

dougiesquire commented 3 months ago

I don't have permission to request a review. @russfiedler, @aidanheerdegen, @aekiss?

aekiss commented 3 months ago

Looks OK on a quick glance but I really don't have the expertise to review this properly, e.g. whether it will have unexpected knock-on effects.

@aidanheerdegen do you have bandwidth to take a look?

aidanheerdegen commented 3 months ago

Note, this PR includes the ACCESS-NRI fork of generic_tracers as a git submodule, but we could instead use git sub-tree if that is preferred.

Is there another option where GFDL-generic-tracers is linked as a compiled library?

I think we've had this discussion somewhere else, but I can't recall the answer.

dougiesquire commented 3 months ago

Is there another option where GFDL-generic-tracers is linked as a compiled library?

Possibly, but I'm not sure I have the time to investigate this. We need these changes for ESM1.6 which is supposed to be running by the end of the year. I'm going on leave from the start of Sept until Jan next year.

Relatedly, the CI is failing because I'm currently using git submodule which isn't being inited.

aidanheerdegen commented 3 months ago

The CI fails, probably because it isn't doing a recursive clone

https://github.com/mom-ocean/MOM5/actions/runs/10089465396/job/27946629510?pr=391#step:10:554

This is kind of a reason to use the ACCESS-NRI fork where we support the CI. Regardless we'd either have to fix this, or remove the ACCESS models from the CI tests and assume our workflow is to work on the fork and pick up issues with CI on our supported models before changes get propagated to this repo.

dougiesquire commented 3 months ago

Regardless we'd either have to fix this, or remove the ACCESS models from the CI tests and assume our workflow is to work on the fork and pick up issues with CI on our supported models before changes get propagated to this repo.

A simple fix would be to use git subtree rather than submodule

aidanheerdegen commented 3 months ago

A simple fix would be to use git subtree rather than submodule

True. Kinda depends how much you expect that code to change.

Possibly, but I'm not sure I have the time to investigate this. We need these changes for ESM1.6 which is supposed to be running by the end of the year. I'm going on leave from the start of Sept until Jan next year.

Valid.

Ok subtree looks like a decent option in the first instance. It's a small amount of code in a sub-dir isolated from the rest of the codebase.

However, if there is a need to test multiple versions of the coupling with the same version of MOM5 that would be much easier with a spack package and an independently compiled library. If this is the case then we could look at looping in someone else to assist with the library/compilation.

dougiesquire commented 2 months ago

Thanks much for the review @aidanheerdegen! I've incorporated your changes. However, I've force pushed them since--as we decided offline--I'll move this PR over to the ACCESS-NRI fork of MOM5 for now and close this PR. We will resubmit a PR with these changes here once we've finalised the ACCESS build process.