-
Notifications
You must be signed in to change notification settings - Fork 542
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
For NCEP regtests, update to spack-stack 1.6, add option for gnu compiler and add Hercules #1145
Merged
MatthewMasarik-NOAA
merged 18 commits into
NOAA-EMC:develop
from
JessicaMeixner-NOAA:feature/gnu
Apr 2, 2024
Merged
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
4cfbd73
update regtests for ncep to remove login for shell and first intel loads
JessicaMeixner-NOAA da63450
updates to matrix_cmake_ncep for gnu and hercules
JessicaMeixner-NOAA 6bc9379
updates for hercules
4af8357
update cmake on hercules
665b2fb
updated hera metis paths for intel and gnu
JessicaMeixner-NOAA 183e86a
update orion intel metis path
JessicaMeixner-NOAA 84b4a37
Merge remote-tracking branch 'EMC/develop' into feature/gnu
JessicaMeixner-NOAA 95446b5
update orion env variables after testing, these are now the same as h…
JessicaMeixner-NOAA 6f338f8
Merge branch 'NOAA-EMC:develop' into feature/gnu
JessicaMeixner-NOAA 37e45ac
Merge branch 'NOAA-EMC:develop' into feature/gnu
JessicaMeixner-NOAA c197660
update 1.6.0 for all platforms and to rocky-8 on hera
JessicaMeixner-NOAA 276823d
Merge branch 'NOAA-EMC:develop' into feature/gnu
JessicaMeixner-NOAA 688f176
update hera modules
JessicaMeixner-NOAA a6d3762
remove extra module for 1.6.0 rocky on hera for gnu
JessicaMeixner-NOAA 07b92a6
add orion-intel metis path
JessicaMeixner-NOAA 894f9a4
update hercules
JessicaMeixner-NOAA 15fe747
initialize array that the rocky8 hera intel transition exposed.
JessicaMeixner-NOAA 5a8da61
move hercules paths to hercules section, revert hera
JessicaMeixner-NOAA File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JessicaMeixner-NOAA these are retained in for orion, but not present for hercules. Also, their position for orion has been moved above the module loads. Are these both intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't see these being used in ufs-weather-model submit scripts. I'm not sure why these are here although the
ulimit -s unlimited
is usually needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those three were originally recommended on WCOSS2 when I was doing testing with SCOTCH a bit ago. They were then also added to orion's job card when there were issues updating modules at a previous time and the issues seemed to go away. I suspect we may need to include them on WCOSS2 when that update happens. We have had all the
export
's together at after the module loads, I don't know whether ordering matters with modules/exports, but I'd vote to choose one or the other to keep all job cards consistent between platformsI find it convenient to have the
cd
to regtests directory right after the#SBATCH
s. Could we keep that location?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthewMasarik-NOAA in my comparions of what is happening in the develop branch on orion and what is happening in this branch in terms of the lodation of #sbatch and the "cd regtests" it is the same. Is this different behavior for you?
Since the hercules tests are working fine without extra envrionement variables beyond the unlimit -s, I prefer to not add anything. We could add the environement variable to remove the one warning, but I'm okay leaving things as is. I'll re-run a test on hercules to see if there are any system changes causing errors for me after my successful tests earlier this week/late last week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JessicaMeixner-NOAA, to tie up a loose end regarding the "cd regtests". Checking now the job card for
hera
, I see it doesn't have either the extraexport
's or thoseulimit
lines, so the "cd regtests" does come directly after the Slurm directives, as I had in mind. However, fororion
, these lines are currently above the "cd regtests", so I was wrong on that. Since nothing changed, no fix needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MatthewMasarik-NOAA
I have tested that we can remove the extra orion variables for our regression tests. Orion is a bit unstable right now so we should re-test that when we have a resolution to #1152 and are ready to re-test this PR.