Align update of nextSIM-DG and XIOS calendars #757

jwallwork23 · 2024-12-09T15:03:17Z

Align update of nextSIM-DG and XIOS calendars

Fixes #756
Fixes #740
Refinement of #751

Task List

Defined the tests that specify a complete and functioning change (It may help to create a design specification & test specification)
Implemented the source code change that satisfies the tests
Documented the feature by providing worked example
Updated the README or other documentation
Completed the pre-Request checklist below

Change Description

This PR adds functionality for incrementing XIOS' calendar and hooks it up in the ModelMetadata::incrementTime method.

More specifically:

Define Xios::incrementCalendar.
Rename Xios::updateCalendar and Xios::setCalendarStep (for consistency with Xios::getCalendarStep.
Reconfigure the build system so that XIOS can be used more widely (i.e., in the library as well as the tests).

Test Description

The new method is used in the XIOS read/write test. Unfortunately, I found a bug in XIOS (see comments below). However, this is fixed with a minor hack in the install-xios.sh script. We should be able to avoid this hack when we update to XIOS3 (see #761).

Pre-Request Checklist

The requirements of this pull request are fully captured in an issue or design specification and are linked and summarised in the description of this PR
No new warnings are generated
The documentation has been updated (or an issue has been created to track the corresponding change)
Methods and Tests are commented such that they can be understood without having to obtain additional context
This PR/Issue is labelled as a bug/feature/enhancement/breaking change
File dates have been updated to reflect modification date
This change conforms to the conventions described in the README

core/test/XiosReadWrite_test.cpp

jwallwork23 · 2024-12-11T09:18:39Z

.github/workflows/test_suite.yml

+  test-ubuntu-mpi-noxios:
+
+    runs-on: ubuntu-22.04
+    container:
+      image: ghcr.io/nextsimhub/nextsimdg-dev-env:latest
+
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: build and compile with MPI but not XIOS
+      run: |
+        . /opt/spack-environment/activate.sh
+        mkdir -p build && cd build
+        cmake -DENABLE_MPI=ON -DENABLE_XIOS=OFF -DCMAKE_CXX_COMPILER="$(which mpic++)" ..
+        make -j 4
+
+    - name: run MPI tests
+      run: |
+        . /opt/spack-environment/activate.sh
+        apt update
+        apt install -y wget
+        cd build
+        (cd core/test && wget "ftp://ftp.nersc.no/nextsim/netCDF/partition_metadata_1.nc")
+        for component in core physics dynamics
+        do
+            cd $component/test
+            for file in $(find test* -maxdepth 0 -type f)
+            do
+              echo $file
+              nprocs=$(echo $file | sed -r "s/.*MPI([0-9]+)/\1/")
+              mpirun --allow-run-as-root --oversubscribe -n $nprocs ./$file
+            done
+            cd -
+        done
+
+  test-ubuntu-mpi-xios:


Note that this change is the same as the one in #758.

jwallwork23 · 2024-12-11T09:19:46Z

core/src/ParaGridIO.cpp

        timeVar.getVar(timeVec.data());
        // Get the index of the largest TimePoint less than the target.
-        targetTIndex = std::find_if(begin(timeVec), end(timeVec), [time](double t) {
+        targetTIndex = std::find_if(std::begin(timeVec), std::end(timeVec), [time](double t) {


This is required to avoid compile errors about begin and end being overloaded and it not being clear which version to use.

jwallwork23 · 2024-12-11T09:20:41Z

core/src/Xios.cpp

 //! Enable XIOS in the 'config'
 void enableXios()
 {
-    Configurator::clearStreams();


This change is important when you set up XIOS after you've already configured other parts of nextSIM-DG.

jwallwork23 · 2024-12-11T09:32:31Z

Oh nice - turns out the XIOS update fixes issue #740. (Dropped workaround in 28b2453.)

timspainNERSC

The XIOS handler living in ModelMetadata feels out of place. Perhaps define it as a singleton in Xios.hpp/.cpp?

timspainNERSC · 2024-12-13T09:56:54Z

core/src/include/ModelMetadata.hpp

    const std::string bboxName = "bounding_boxes";
 #endif
+#ifdef USE_XIOS
+    Xios* xiosHandler = nullptr;


Is ModelMetadata the best place for the XIOS handler? It doesn't (instinctively) feel like metadata data to me. Perhaps implementing the infrastructure necessary to return an Xios singleton.

Also, wherever it ends up, might a smarter pointer be better than a raw pointer?

The way I currently have it set up in #751 is that multiple classes (ParaGridIO and ModelMetadata) have pointers to it. A singleton would make sense in general, yes. It might make test files containing multiple tests that use XIOS a little trickier to set up, but that's not really the important thing. I'll look into how to implement it. Also good point on using smart pointers.

Opened PR #763 to address this issue, which would merge into this PR if approved.

@timspainNERSC what kind of smart pointer were you considering? I guess we could do something like unique_ptr but this slightly different to the singleton functionality we need. We really want a single instance rather than a unique pointer. But perhaps both is extra protection?

I think a shared_ptr is more likely to be the type of smart pointer that we need for a singleton.

Thinking further, the singleton function should return a reference, rather than any kind of smart or dumb pointer. If an object needs to store a pointer to the object, then a plain pointer should be fine, as the ownership of the object is clear: it belongs to the singleton function.

@AdelekeBankole , place the unique_ptr into xios.hpp

This discussion will be resolved upon merging #763 with recent changes.

TomMelt

Once #763 is merged in this looks good to go 🚀

jwallwork23 · 2025-04-08T12:19:32Z

~~Let's merge this following #807.~~

Oops, wrong PR.

jwallwork23 · 2025-04-29T12:58:25Z

[Rebased on top of develop.]

@TomMelt

Supersedes #808. As discussed in the meeting this morning, this PR hooks the `Xios` class into the `Finalizer`. Since no `Model`s are created in the standalone XIOS unit tests, they need to call `Finalizer::finalize` explicitly. It seems the same is also true of the `ConfigOutput` and `ParaGrid` tests. So actually the changeset isn't that different than @TomMelt's one in #808.

Note that this PR merges into the branch for #757, not `develop`. This PR implements the `Xios` handler object as a singleton, as requested. This means that nextSIM-DG classes don't need to own pointers to the current working `Xios` instance. There can be only one `Xios` instance. The first time the static `Xios::getInstance` method is called, it creates the singleton and returns a pointer to it. On subsequent calls, this method simply returns a pointer to the singleton. Unfortunately, I had to rework the XIOS read/write test. The singleton design pattern isn't suitable for the way it's currently written, so I re-separated out the reading and writing parts and re-combined the 2D and 3D cases.

jwallwork23 · 2025-04-30T08:50:43Z

The same is true here as in #758 because of the (duplicate) workflow change.

Ah, with this PR we have an issue that the repo settings expect the test-ubuntu-mpi action to pass, but it's been split out into test-ubuntu-mpi-noxios and test-ubuntu-mpi-xios. So we'll need to edit the repo settings in order to merge it.

timspainNERSC

That looks good, now

jwallwork23 · 2025-04-30T09:16:40Z

That looks good, now

Perfect, thanks @timspainNERSC. Please could you temporarily disable the requirement for the test-ubuntu-mpi test to pass before merging? Once this has been merged, there will then be test-ubuntu-mpi-noxios and test-ubuntu-mpi-xios jobs that we can require instead. (I guess I could alternatively rename test-ubuntu-mpi-noxios as test-ubuntu-mpi but I feel it's good to be explicit here.)

jwallwork23 · 2025-05-06T09:16:24Z

Will merge once the periodic Docker build passes.

jwallwork23 added the ICCS Tasks or reviews for the ICCS team label Dec 9, 2024

jwallwork23 self-assigned this Dec 9, 2024

jwallwork23 marked this pull request as draft December 10, 2024 11:30

jwallwork23 force-pushed the issue756_xios-calendar-increment branch 2 times, most recently from 38e2e5c to 0ca57ad Compare December 10, 2024 14:28

jwallwork23 commented Dec 10, 2024

View reviewed changes

core/test/XiosReadWrite_test.cpp Outdated Show resolved Hide resolved

jwallwork23 added the enhancement New feature or request label Dec 10, 2024

jwallwork23 marked this pull request as ready for review December 10, 2024 17:54

jwallwork23 requested review from TomMelt, einola and timspainNERSC December 11, 2024 09:14

jwallwork23 commented Dec 11, 2024

View reviewed changes

timspainNERSC requested changes Dec 13, 2024

View reviewed changes

This was referenced Jan 2, 2025

Implement Xios handler as a singleton #763

Merged

Implement dumpModelState using XIOS #767

Closed

TomMelt approved these changes Jan 8, 2025

View reviewed changes

jwallwork23 added 10 commits April 29, 2025 13:57

Modify build system for wider-scale use of XIOS

7ab73ca

Add Xios::incrementCalendar and use in ModelMetadata::incrementTime

0b69471

Rename updateCalendar as setCalendarStep

75a6693

Use ModelMetadata::incrementTime in XIOS read/write test

c6073d9

Create XIOS handlers for other tests build with XIOS support

a702d03

Same for ModelMetadata::setTime

84efc30

Enable XIOS; remember to close context defs

63af787

Initialise xiosHandler to nullptr

e227594

Don't build RectGrid_test with XIOS

bd22464

Pass timestep to Xios constructor when contextId is specified

884019b

jwallwork23 added 3 commits April 29, 2025 13:57

Issue fixed by XIOS hack

1fae693

Drop FIXME no longer relevant

80da356

assertIsClose no longer needed [skip ci]

e4b4870

jwallwork23 force-pushed the issue756_xios-calendar-increment branch from 28b2453 to e4b4870 Compare April 29, 2025 12:58

jwallwork23 and others added 15 commits April 29, 2025 13:58

Enable XIOS in tests

4c9bf10

Define Xios handler as singleton

59fb5b4

Use singleton in most XIOS tests

730cb20

Use Xios singleton in other tests

d66e0c0

Recombine dims; re-separate read and write

ac864e2

Revert date; drop unused mutex include

a42f5e5

Implement getInstance method as a reference to static Xios instance

448ce1c

Remove added line [skip ci]

968e837

Avoid setting defaults for constructor twice

12d3002

Prevent copying

687b625

Fix reference usage [skip ci]

66315ee

Attempt at setting up finalizer

2d5b825

Call finalizer in tests

f65ed66

jwallwork23 requested a review from timspainNERSC April 30, 2025 08:43

timspainNERSC approved these changes Apr 30, 2025

View reviewed changes

Merge branch 'develop' into issue756_xios-calendar-increment

4afa5ce

jwallwork23 merged commit d4ce351 into develop May 6, 2025
7 checks passed

github-project-automation bot moved this from Review required to Done in neXtSIM_DG overview May 6, 2025

jwallwork23 deleted the issue756_xios-calendar-increment branch May 6, 2025 10:34

jwallwork23 mentioned this pull request May 6, 2025

Configure XIOS from .cfg file #824

Merged

12 tasks

Align update of nextSIM-DG and XIOS calendars #757

Align update of nextSIM-DG and XIOS calendars #757

Uh oh!

Conversation

jwallwork23 commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Align update of nextSIM-DG and XIOS calendars

Task List

Change Description

Test Description

Pre-Request Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwallwork23 commented Dec 11, 2024

Uh oh!

timspainNERSC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomMelt Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomMelt left a comment

Choose a reason for hiding this comment

Uh oh!

jwallwork23 commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwallwork23 commented Apr 29, 2025

Uh oh!

jwallwork23 commented Apr 30, 2025

Uh oh!

timspainNERSC left a comment

Choose a reason for hiding this comment

Uh oh!

jwallwork23 commented Apr 30, 2025

Uh oh!

jwallwork23 commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jwallwork23 commented Dec 9, 2024 •

edited

Loading

TomMelt Jan 22, 2025 •

edited

Loading

jwallwork23 commented Apr 8, 2025 •

edited

Loading