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.

Sign up for GitHub

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

Jump to bottom

Temporary addition of GNU 7.3 to GHA #136

Closed
odlomax opened this issue May 10, 2023 · 13 comments
Closed

Temporary addition of GNU 7.3 to GHA #136

odlomax opened this issue May 10, 2023 · 13 comments

Comments

@odlomax
Copy link
Contributor

odlomax commented May 10, 2023 •
edited
Loading

Compiler support

As previously discussed, the Met Office has a temporary requirement where we have to make sure that Atlas builds and runs using GNU 7.3 compilers.

GNU 7.3 supports all of the C+17 language additions and most of the standard library additions, so the impact on Atlas should be minimal. Supported features.

We currently believe we should be able to remove this requirement over the next six to twelve months.
@tlmquintino
Copy link
Member

Thanks for this info. @wdeconinck I think we must then also add this to the rest of the stack.
Can you please liaise with @dtip ?

@wdeconinck
Copy link
Member

wdeconinck commented May 10, 2023 •
edited
Loading

@tlmquintino if you think that is necessary. Otherwise I can add just a few lines in the older atlas GitHub actions that I till have that uses Github-owned runners, for the remaining 6 months or so. Depends how high in the stack the UKMO wants this requirement to be.

@odlomax
Copy link
Contributor Author

odlomax commented May 10, 2023

@tlmquintino if you think that is necessary. Otherwise I can add just a few lines in the older atlas GitHub actions that I till have that uses Github-owned runners, for the remaining 6 months or so. Depends how high in the stack the UKMO wants this requirement to be.

@tlmquintino @wdeconinck

Thanks for getting back. UKMO is basically restricted to GNU 7.3 until we're fully up and running with with our new HPC -- that should happen by the end of this year.

We're currently using the following ECMWF packages in our JEDI work with JCSDA:

  • eckit
  • fckit
  • atlas
  • fiat
  • ectrans
  • odc

Our main concern is that some exotic parts of C++17 might slip into Atlas/eckit/fckit and we lose our capability to run it on the HPC. Once we're on the new HPC, this problem goes away. (until we start having fun with C++20...)

@tlmquintino
Copy link
Member

@wdeconinck since this involves eckit and odc too I would prefer this is done in the new CI/CD framework.
Please liaise with @dtip or @figi44 to inject GNU 7.3 in our build matrix.

@odlomax
Copy link
Contributor Author

odlomax commented May 10, 2023

I'm just tagging @yaswant here as he handles the software stack on our side.

wdeconinck added a commit that referenced this issue May 16, 2023
@wdeconinck
Add gnu-7 ci to github actions with github-hosted runners (#136)
43c22a9
@wdeconinck
Copy link
Member

In the mean time of having it added to the new CI/CD framework, I have added it to the older ci with Github-hosted runners (see above referenced commit).
That is currently testing eckit/fckit/atlas with gnu-7 installed via apt-get install gcc-7 g++-7 gfortran-7.
Note that this is gcc 7.5 instead of 7.3 I don't know if that version difference is important though.

@yaswant
Copy link

yaswant commented May 17, 2023

Thank you @wdeconinck - is there any way we can set minor version to 3? Its not required from CXX17 perspective and not relevant in the scope of the current discussion. Sharing this for information only.

Fortran-mpi related complication: F2018 features needed to support the build of the mpi_f08 module - we cant build this module with mpich or openmpi in gcc-7.3 environment. And this is the gnu compiler version we are stuck with current Cray.

@wdeconinck
Copy link
Member

@yaswant It seems not trivial to install gcc-7.3 when the default 'gcc-7' is 7.5 (see https://askubuntu.com/questions/1225026/how-can-i-install-a-specific-version-of-gcc-on-ubuntu-18-04)
The route is probably compiling a specific gcc version manually.
I'll leave that for the newer custom CI/CD self-hosted runners that are being developed.

Which code is using the mpi_f08 module?

@yaswant
Copy link

yaswant commented May 19, 2023

@wdeconinck no problem.

My colleague was testing mpi_f08 module in ops based JEDI interface for the UM (ops-um-jedi). If I understand correctly fckit does not support shared memory so he used raw mpi there1. It is unlikely that anyone using fckit will be doing that.

Footnotes

  1. We are delaying this implementation for now. ↩

@tlmquintino
Copy link
Member

Looking at the release notes for GCC 7.3 to 7.5 it looks like they were only bug fixes.
See https://gcc.gnu.org/gcc-7/changes.html

So unless you want to reproduce buggy behaviour, I would argue that using 7.5 is a very good proxy to using 7.3 for the purposes of ensuring the builds dont break and unit tests are passing.

@wdeconinck
Copy link
Member

wdeconinck commented Jun 5, 2023 •
edited
Loading

As some fluke experience in recent pull-request showed (see #145), the gnu 7.5 had a c++ 14 compiler bug fixed compared to 7.3 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64095); thanks @odlomax !
That shows that we cannot rely on gnu-7.5 for gnu-7.3 compatibility.

wdeconinck added a commit to JCSDA-internal/atlas that referenced this issue Jun 6, 2023
@wdeconinck
Merge branch 'develop' into feature/normalise_lats_in_kdtree
51d0fef 
* develop: (56 commits)
  Implement field::for_each capabilities (ecmwf#139)
  Avoid harmless FE_INVALID with nvhpc build
  Avoid harmless FE_INVALID with nvhpc build
  Remove ATLAS_FPE=0 environment variable as not needed anymore now
  Avoid harmless FE_DIVBYZERO with nvhpc build
  Optimize modules and dependencies for caching
  Add HPC build options matrix
  Workaround compiler bug in nvhpc-22.11-release build
  Update GHA nvidia-21.9 to nvidia-22.11
  Avoid and suppress some compiler warnings with nvhpc
  Fix bug where DelaunayMeshGenerator with 1 partition was not setting the grid in the mesh (ecmwf#143)
  Use Eigen 3.4
  Disable floating point signals for tests on nvidia
  Add nvidia compiler specific HPC build config
  Add function to build mesh from imported connectivity data (ecmwf#135)
  Disable GHA "linux gnu-12" OpenMP for CXX as "omp.h" header is not found :(
  Add gnu-12 ci to github actions (github-hosted runners)
  Add gnu-7 ci to github actions with github-hosted runners (ecmwf#136)
  Setup horizontal_dimensions() for BlockStructuredColumns fields
  Add function Field::horizontal_dimension() -> std::vector<idx_t>
  ...
@dtip
Copy link
Contributor

dtip commented Jul 6, 2023

We're now testing the ECMWF stack with gnu 7.3.1 on CentOS 7.9

PR: ecmwf-actions/downstream-ci#14

Shout if you run into any issues.

@dtip dtip closed this as completed Jul 6, 2023
@odlomax
Copy link
Contributor Author

odlomax commented Jul 6, 2023

We're now testing the ECMWF stack with gnu 7.3.1 on CentOS 7.9

PR: ecmwf-actions/downstream-ci#14

Shout if you run into any issues.

Thanks @dtip, @wdeconinck, @tlmquintino!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@wdeconinck @tlmquintino @yaswant @odlomax @dtip