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

ECLand: Initial CI test pipeline for GNU, Intel and MacOS in double-precision #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mlange05
Copy link

@mlange05 mlange05 commented Feb 5, 2025

An initial test setup that runs CI in double-precision for some of the standard compilers. The install scripts are taken from ecRad and ecWAM, largely and simply execute the to standard tests via ctest.

Currently supported (and not yet supported):

  • GNU-10 works fine with Ubuntu-provided Netcdf
  • GNU-14 worked fine with Ubuntu-provided netcdf, but failed one of the ctests - removed for now.
  • Intel-classic works fine with bootstrapped netcdf
  • Nvidia compiles and run with bootstrapped netcdf, but ultimately fails tests - so disabled for now.
  • MacOS/clang works fine with brew-installed netcdf

Please note that single precision failed to compile with several toolchains due to a utility routine (MINMAX) being called from precision-variant and precision-invariant code paths (with JPRB and JPRD). I've left the hooks in the workflow file for fixing in a separate PR.

@mlange05 mlange05 marked this pull request as ready for review February 6, 2025 08:24
Copy link

@awnawab awnawab left a comment

Choose a reason for hiding this comment

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

This looks great 👌 Thanks a lot for adding the CI setup and working through the fiddly netcdf issues 🙏 I've left just a few small comments that would be nice to cleanup before we merge.

include:

- name: linux gnu-10
os: ubuntu-20.04
Copy link

Choose a reason for hiding this comment

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

We probably want to bump this up to ubuntu-22.04; 20.04 will be deprecated soon.

compiler_cc: gcc-10
compiler_cxx: g++-10
compiler_fc: gfortran-10
ctest_options: -V -E memory
Copy link

Choose a reason for hiding this comment

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

Do we need these? I couldn't find a test called "memory", so not sure if we need this exclude here.

# caching: true

- name : linux intel-classic
os: ubuntu-20.04
Copy link

Choose a reason for hiding this comment

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

Same comment as above about ubuntu version.

sudo apt-get install libnetcdff-dev
fi

pip3 install numpy
Copy link

Choose a reason for hiding this comment

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

Why do we need numpy?

Copy link
Contributor

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

This looks very good to me!

Only thing is, that I don't see any use of Python in the action, so you could skip the Python set-up step. Make sure to then also remove the python_version property from the matrix entries.

Comment on lines +84 to +87
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this use any Python?

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