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

jwallwork23 · 2024-12-10T14:30:59Z

core/test/XiosReadWrite_test.cpp

    xios_handler.close_context_definition();
+    // FIXME: Why do we need to re-set the calendar timestep and start here?
+    //        These are already set in the Xios constructor and the test fails if the following two
+    //        lines are moved above the close_context_definition.
+    xios_handler.setCalendarTimestep(Duration("P0-0T01:30:00"));
+    xios_handler.setCalendarStart(TimePoint("2023-03-17T17:11:00Z"));


I'm currently in discussion with the XIOS developers on why this is occurring. Apparently the MFE I provided works fine in XIOS3. (I'm using XIOS2 at revision 2638.)

I considered updating to XIOS3 but found that some of the demos hang with that so figured it'd be better to address this specific issue first.

Fixed the issue above locally with a hack as in fe1c9b2. My latest commit triggered the Docker image to be rebuilt so hopefully it will work in there, too.

Okay so initially the test suite with XIOS failed but then once the Docker image had built, I re-triggered it and it passed!

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

@@ -227,7 +227,7 @@ ModelState ParaGridIO::readForcingTimeStatic(
        std::vector<double> timeVec(timeDim.getSize());
        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

@@ -37,7 +37,6 @@ static const std::map<int, std::string> keyMap = { { Xios::ENABLED_KEY, "xios.en
 //! 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

@@ -117,6 +129,9 @@ class ModelMetadata {
 #ifdef USE_MPI
    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.

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 added 6 commits December 10, 2024 11:35

Modify build system for wider-scale use of XIOS

105568d

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

6ff28af

Rename updateCalendar as setCalendarStep

3d15fae

Use ModelMetadata::incrementTime in XIOS read/write test

ee75b9d

Create XIOS handlers for other tests build with XIOS support

752b24f

Same for ModelMetadata::setTime

a5b639b

jwallwork23 force-pushed the issue756_xios-calendar-increment branch from 954b07d to b8975d7 Compare December 10, 2024 11:36

jwallwork23 added 9 commits December 10, 2024 13:57

Enable XIOS; remember to close context defs

57bb749

Initialise xiosHandler to nullptr

8238fb6

Don't build RectGrid_test with XIOS

4868f88

Pass timestep to Xios constructor when contextId is specified

260e050

At the core of the issue in the read/write test

326d519

Add extra workflow job for testing without XIOS

9cf905a

Don't clear streams in Xios

fe2118d

Fix ParaGrid_test

955c5c2

Reset XIOS calendar step in ModelMetadata::setTime

0ca57ad

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

jwallwork23 commented Dec 10, 2024

View reviewed changes

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

jwallwork23 added 2 commits December 10, 2024 16:20

Apply a hack to get XIOS2 working

fe1c9b2

Issue fixed by XIOS hack

350702e

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

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

jwallwork23 commented Dec 11, 2024

View reviewed changes

jwallwork23 added 2 commits December 11, 2024 09:23

Drop FIXME no longer relevant

00f1a5e

assertIsClose no longer needed

28b2453

timspainNERSC requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align update of nextSIM-DG and XIOS calendars #757

Align update of nextSIM-DG and XIOS calendars #757

jwallwork23 commented Dec 9, 2024 •

edited

Loading

jwallwork23 Dec 10, 2024

jwallwork23 Dec 10, 2024

jwallwork23 Dec 10, 2024

jwallwork23 Dec 11, 2024

jwallwork23 Dec 11, 2024

jwallwork23 Dec 11, 2024

jwallwork23 commented Dec 11, 2024

timspainNERSC left a comment

timspainNERSC Dec 13, 2024

jwallwork23 Dec 13, 2024

Align update of nextSIM-DG and XIOS calendars #757

Are you sure you want to change the base?

Align update of nextSIM-DG and XIOS calendars #757

Conversation

jwallwork23 commented Dec 9, 2024 • edited Loading

Align update of nextSIM-DG and XIOS calendars

Task List

Change Description

Test Description

Pre-Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwallwork23 commented Dec 11, 2024

timspainNERSC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwallwork23 commented Dec 9, 2024 •

edited

Loading