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

Update METbaseimage to use newer versions of Atlas and ecKit #27

Closed
6 of 21 tasks
jprestop opened this issue Jul 24, 2024 · 7 comments · Fixed by #28
Closed
6 of 21 tasks

Update METbaseimage to use newer versions of Atlas and ecKit #27

jprestop opened this issue Jul 24, 2024 · 7 comments · Fixed by #28
Assignees
Labels
alert: NEED ACCOUNT KEY Need to assign an account key to this issue component: build process Build process issue priority: high High Priority requestor: NOAA/EMC NOAA Environmental Modeling Center type: task An actionable item of work

Comments

@jprestop
Copy link
Collaborator

Describe the Task

It was discovered that newer versions of atlas and eckit are being used on WCOSS2. These are versions atlas/0.35.0 and eckit/1.24.4. Update the METbaseimage to use these newer versions.

Time Estimate

1 day

Sub-Issues

Consider breaking the task down into sub-issues.

  • Add a checkbox for each sub-issue here.

Relevant Deadlines

??

Funding Source

??

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Select component(s)
  • Select priority
  • Select requestor(s)

Projects and Milestone

  • Select Repository and/or Organization level Project(s) or add alert: NEED CYCLE ASSIGNMENT label
  • Select Milestone as the next official version or Future Versions

Define Related Issue(s)

Consider the impact to the other METplus components.

Task Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of develop.
    Branch name: feature_<Issue Number>_<Description>
  • Complete the development and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Push local changes to GitHub.
  • Submit a pull request to merge into develop.
    Pull request: feature <Issue Number> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Development issues
    Select: Repository level development cycle Project for the next official release
    Select: Milestone as the next official version
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Close this issue.
@jprestop jprestop added alert: NEED ACCOUNT KEY Need to assign an account key to this issue component: build process Build process issue priority: high High Priority requestor: NOAA/EMC NOAA Environmental Modeling Center type: task An actionable item of work labels Jul 24, 2024
@jprestop jprestop added this to the METbaseimage 3.3 milestone Jul 24, 2024
@jprestop jprestop moved this to 🔖 Ready in MET-12.0.0 Development Jul 24, 2024
@JohnHalleyGotway
Copy link
Collaborator

@jprestop, @hsoh-u pointed out an important detail during the MET Development Meeting on 7/25/24. According to the Atlas Release page, Atlas version 0.33.0 and later requires compiler support for the C++17 standard. This section of the MET User's Guide notes that C++11 support is required, but not C++17.

So the question is, how do we deal with this situation?

Here are some options:

  1. Update to Atlas 0.35.0 "everywhere" and change the MET documentation to indicate that the C++17 standard is now required.
  2. Provide separate "tar files" for C++11 (i.e. Atlas < 0.33.0) and for C++17 (i.e. Atlas >= 0.33.0). For the METbaseimage and for WCOSS2, we'd use the C++17 tar files, but we'd provide the C++11 version for users with older compilers. Perhaps this would mean more logic in the compile_met_all.sh script and/or the website instructions?

How do you want to handle this wrinkle?

@jprestop
Copy link
Collaborator Author

@hsoh-u Thank you being aware that Atlas version 0.33.0 and later require compiler support for the C++17 standard.

@JohnHalleyGotway @hsoh-u Would more of the MET code base need to change if we move to requiring support for the C++17 standard?

I see here that the new Intel oneAPI compilers are using C++17 as the default standard: "With 2023.0 release, ICX/ICPX compiler is switching to C++17 from C++14 as the default standard while doing C++ compilation." And, at some point, the classic Intel compilers won't be supported any longer, although it's not exactly clear when that will be. And, it could vary based on the system and support for that system.

Currently, I lean toward "Update to Atlas 0.35.0 'everywhere' and change the MET documentation to indicate that the C++17 standard is now required." as I believe C++17 was released in December 2017, but I don't know if that would require other changes to the MET code base and I don't have a good idea for how much hardship that might cause for users with older systems.

@JohnHalleyGotway
Copy link
Collaborator

@jprestop I don't think it would require any changes to the MET code base itself. The main concern is how much of a hardship that might cause for users with older systems. Before switching to requiring support for the C++17 standard, we should get confirmation from our primary funding partners (e.g. NOAA/EMC/GSL/CPC/etc, Met Office, Air Force, NRL, etc) and double-check that C++17 compliant compilers work well on all the "existing builds" machines we support.

If the existing builds machines are fine and our funding partners have no objections, I'm fine with that change.

@jprestop
Copy link
Collaborator Author

jprestop commented Aug 7, 2024

@JohnHalleyGotway In order to get MET to compile on WCOSS2, I had to change references to "c++11" and "cxx11" with "c++17" and "cxx17" and then rerun bootstrap. I think that means that we don't really have ability to provide separate "tar files" for C++11 (i.e. Atlas < 0.33.0) and for C++17 (i.e. Atlas >= 0.33.0). Am I arriving at that conclusion correctly?

@JohnHalleyGotway
Copy link
Collaborator

Julie, great point. I wasn't expecting that switching from 11 to 17 would be required to get MET to compile with the updated Atlas version.

I suppose this means we'd need to make this change immediately for the MET 12.0.0 release... or, if not, update the installation instructions to include rerunning bootstrap.

This is already on the agenda for the METplus governance meeting today. Seems to me that switching to 17 now is the simpler/preferred option. But if there is pushback, revising our installation instructions to include bootstrap could be a fallback option.

@jprestop
Copy link
Collaborator Author

jprestop commented Aug 7, 2024

@JohnHalleyGotway I'm modifying the agenda for the METplus Advisory Group meeting with this new information. While we would revise our installation instructions to include running bootstrap, that would require a version of aclocal that is 1.16 or higher, which may be problematic for groups as well (note that the version of aclocal on WCOSS2 is only 1.15.1 - I had to download the code to my machine, run bootstrap, package it up, and transfer it to WCOSS2), but I suppose we would do that and put the package on the server for groups to grab. Of course, that is extra effort and makes the steps more complicated.

@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Aug 23, 2024

@jprestop there's a chicken and egg problem here. You have changes to the compile_MET_all.sh script in the dtcenter/MET branch named feature_2948_c++17. The Dockerfile in METbaseimage needs to pull that script from MET in order to be built... but those updates won't be in MET's develop branch until that feature branch has been merged. But that feature branch can't be merged until METbaseimage has been updated to provide a good compilation environment.

To solve this two-way dependency, I manually built the updated met-base images on my laptop and pushed them up to DockerHub using the commands listed below. Once they're available on DockerHub, you can do the following:

  1. Test that your feature_2948_c++17 MET branch compiles well.
  2. Submit/review/approve the PR to merge into MET's develop branch.
  3. Once MET's develop branch is updated, get rid of your changes to the METbaseimage Dockerfile, reverting back to the existing default settings, but keep your README updates.
  4. Create a new v3.3 METbaseimage release on GitHub. That will trigger GHA to go rebuild the images I built manually and overwrite them on DockerHub.

These commands were adapted from the GHA job script named .github/jobs/build_docker_image.sh.

# From top-level directory containing the Dockerfiles
cd METbaseimage

# Build dtcenter/met-base:v3.3
docker build -t dtcenter/met-base:v3.3 -f Dockerfile . >& docker_build_met_base.log &
tail -f docker_build_met_base.log

# Build dtcenter/met-base-unit-test:v3.3
docker build -t dtcenter/met-base-unit-test:v3.3 -f Dockerfile.unit_test_env --build-arg MET_BASE_TAG=v3.3 . >& docker_build_met_base_unit_test.log &
tail -f docker_build_met_base_unit_test.log

# Build dtcenter/met-base-metviewer (not strictly necessary)
docker build -t dtcenter/met-base-metviewer:v3.3 -f Dockerfile.metviewer --build-arg MET_BASE_TAG=v3.3 . >& docker_build_met_base_metviewer.log &
tail -f docker_build_met_base_metviewer.log

# Manually push the results to DockerHub (requires sufficient DockerHub permission)
docker push dtcenter/met-base:v3.3
docker push dtcenter/met-base-unit-test:v3.3
docker push dtcenter/met-base-metviewer:v3.3

Note that METviewer DOES INCLUDE references to v3.2:

grep v3.2 `find METviewer -type f`
METviewer/internal/scripts/docker/Dockerfile:ARG MET_BASE_TAG=v3.2
METviewer/internal/scripts/docker/Dockerfile.copy:ARG MET_BASE_TAG=v3.2
METviewer/internal/scripts/docker/Dockerfile.apptainer:ARG MET_BASE_TAG=v3.2

You could update these to v3.3 after creating the METbaseimage v3.3 release. While it isn't strictly required, it'd probably make things less confusing to keep those versions consistent.

The following images are now available on DockerHub:

jprestop added a commit that referenced this issue Sep 3, 2024
@jprestop jprestop moved this from 🟢 Ready to 🔎 In review in MET-12.0.0 Development Sep 3, 2024
@jprestop jprestop linked a pull request Sep 3, 2024 that will close this issue
13 tasks
jprestop added a commit that referenced this issue Sep 3, 2024
jprestop added a commit that referenced this issue Sep 3, 2024
* Per #27, modified argument for branch and tar file version name, and added description of changes to README.md

* Per #27, changed MET_COMPILE_SCRIPT_BRANCH back to develop, left MET_TAR_FILE_VERSION_NAME as met-base-v3.3

* Per #27, modifying capitalization for consistency

Co-authored-by: John Halley Gotway <[email protected]>

---------

Co-authored-by: John Halley Gotway <[email protected]>
@github-project-automation github-project-automation bot moved this from 🔎 In review to 🏁 Done in MET-12.0.0 Development Sep 3, 2024
@JohnHalleyGotway JohnHalleyGotway changed the title Update METbaseimage to use newer versions of atlas and eckit Enhance METbaseimage to use newer versions of Atlas and ecKit Oct 16, 2024
@JohnHalleyGotway JohnHalleyGotway changed the title Enhance METbaseimage to use newer versions of Atlas and ecKit Update METbaseimage to use newer versions of Atlas and ecKit Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alert: NEED ACCOUNT KEY Need to assign an account key to this issue component: build process Build process issue priority: high High Priority requestor: NOAA/EMC NOAA Environmental Modeling Center type: task An actionable item of work
Projects
Status: 🏁 Done
Development

Successfully merging a pull request may close this issue.

2 participants