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

Depend on openmpi instead of nci-openmpi and add restart reproducibility #49

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Depend on openmpi instead of nci-openmpi and add restart reproducibility #49

merged 5 commits into from
Mar 27, 2024

Conversation

harshula
Copy link
Contributor

@harshula harshula self-assigned this Feb 21, 2024
@harshula
Copy link
Contributor Author

This change was triggered by ACCESS-NRI/spack-packages#61 .

@harshula harshula requested a review from CodeGat March 8, 2024 00:18
CodeGat
CodeGat requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@CodeGat CodeGat left a comment

Choose a reason for hiding this comment

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

We'll need to update the version before merging it - if you rebase onto main it should help out telling what needs to be done.

@github-actions GitHub Actions
Copy link
Contributor

The model version in the spack.yaml has not been updated.
Either update it manually, or comment the following to have it updated and committed automatically:

  • !bump major for feature releases
  • !bump minor for bugfixes

@aidanheerdegen
Copy link
Member

!bump minor

@github-actions GitHub Actions
Copy link
Contributor

✅ Version bumped from 2023.11.23 to 2023.11.24 ✅

@github-actions GitHub Actions
Copy link
Contributor

This ACCESS-NRI/ACCESS-OM2 model will be deployed with the following versions:

  • 2023.11.24 as a Release (when merged).
  • 2023.11.24-2 as a Prerelease (during this PR). This can be accessed on Gadi via spack at /g/data/vk83/prerelease/apps/spack/0.20/spack once deployed.

It will be deployed using:

If this is not what was expected, commit changes to config/versions.json.

@aidanheerdegen
Copy link
Member

Pre-release deployment is reporting

/bin/bash: line 5: /g/data/vk83/prerelease/apps/spack/0.20/spack-config/spack-enable.bash: No such file or directory

Despite this change

https://github.com/ACCESS-NRI/build-cd/pull/31/files

Seems that on gadi it is still called spack-start.sh

$ ls /g/data/vk83/prerelease/apps/spack/0.20/spack-config/
LICENSE  README.md  spack-start.bash  tools  v0.20

@CodeGat
Copy link
Contributor

CodeGat commented Mar 20, 2024

Might need to check the version tagged in config/versions.json in the morning - is it spack-enable.bash in that tag? Or spack-start?

@github-actions GitHub Actions
Copy link
Contributor

The model version in the spack.yaml has not been updated.
Either update it manually, or comment the following to have it updated and committed automatically:

  • !bump major for feature releases
  • !bump minor for bugfixes

@github-actions GitHub Actions
Copy link
Contributor

This ACCESS-NRI/ACCESS-OM2 model will be deployed with the following versions:

  • 2023.11.24 as a Release (when merged).
  • 2023.11.24-3 as a Prerelease (during this PR). This can be accessed on Gadi via spack at /g/data/vk83/prerelease/apps/spack/0.20/spack once deployed.

It will be deployed using:

If this is not what was expected, commit changes to config/versions.json.

@CodeGat
Copy link
Contributor

CodeGat commented Mar 21, 2024

!bump major

@github-actions GitHub Actions
Copy link
Contributor

✅ Version bumped from 2023.11.24 to 2024.03.0 ✅

@github-actions GitHub Actions
Copy link
Contributor

This ACCESS-NRI/ACCESS-OM2 model will be deployed with the following versions:

  • 2024.03.0 as a Release (when merged).
  • 2024.03.0-4 as a Prerelease (during this PR). This can be accessed on Gadi via spack at /g/data/vk83/prerelease/apps/spack/0.20/spack once deployed.

It will be deployed using:

If this is not what was expected, commit changes to config/versions.json.

aidanheerdegen
aidanheerdegen previously approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

LGTM

@aidanheerdegen
Copy link
Member

We shouldn't merge until we have done some performance testing, confirming the impact is minimal

@CodeGat
Copy link
Contributor

CodeGat commented Mar 21, 2024

Good point @aidanheerdegen - I've requested changes so it shouldn't be able to merge

@aidanheerdegen
Copy link
Member

Seems the pre-release build is still using nci-openmpi

$ grep openmpi /g/data/vk83/prerelease/apps/spack/0.20/spack/var/spack/environments/access-om2-2024_03_0-4/spack.lock                                 
          "name": "nci-openmpi",
          "name": "nci-openmpi",
      "name": "nci-openmpi",
        "path": "/apps/openmpi/4.0.2",
          "openmpi/4.0.2"
          "name": "nci-openmpi",
          "name": "nci-openmpi",
          "name": "nci-openmpi",
          "name": "nci-openmpi",
          "name": "nci-openmpi",

@CodeGat
Copy link
Contributor

CodeGat commented Mar 22, 2024

What does this mean, exactly...?

@CodeGat
Copy link
Contributor

CodeGat commented Mar 22, 2024

@harshula

@github-actions GitHub Actions
Copy link
Contributor

This ACCESS-NRI/ACCESS-OM2 model will be deployed with the following versions:

  • 2024.03.0 as a Release (when merged).
  • 2024.03.0-5 as a Prerelease (during this PR). This can be accessed on Gadi via spack at /g/data/vk83/prerelease/apps/spack/0.20/spack once deployed.

It will be deployed using:

If this is not what was expected, commit changes to config/versions.json.

@CodeGat
Copy link
Contributor

CodeGat commented Mar 22, 2024

@harshula The openmpi thing was fixed - we hadn't tagged the nci-openmpi->openmpi fix in spack-config.

@harshula
Copy link
Contributor Author

Hi @CodeGat , Is there anything else we need to do?

@CodeGat
Copy link
Contributor

CodeGat commented Mar 22, 2024

I assume some kind of performance testing of some kind before proper deployment

@aidanheerdegen aidanheerdegen changed the title Depend on openmpi instead of nci-openmpi Depend on openmpi instead of nci-openmpi and add restart reproducibility Mar 25, 2024
@aidanheerdegen aidanheerdegen mentioned this pull request Mar 25, 2024
Closed
@access-hive-bot
Copy link

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/access-om2-bit-repro-testing/1960/1

aidanheerdegen
aidanheerdegen approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Looks good. Performance checks suggest no issue with model slowdown.

ACCESS-NRI/access-om2-configs#35

@aidanheerdegen aidanheerdegen merged commit bf1f97c into main Mar 27, 2024
10 checks passed
@harshula harshula deleted the nci-openmpi-to-openmpi branch May 3, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodeGat CodeGat CodeGat approved these changes

@aidanheerdegen aidanheerdegen aidanheerdegen approved these changes

Assignees

@harshula harshula

Labels
priority:blocker
Projects
ACCESS-OM2 Spack
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harshula @aidanheerdegen @CodeGat @access-hive-bot