-
Notifications
You must be signed in to change notification settings - Fork 49
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
Updating modulefiles to work with latest spack-stack #501
Conversation
Do the modulefile changes correspond to the latest in https://github.com/ufs-community/ufs-weather-model/tree/develop/modulefiles? |
@grantfirl Yes, these changes should correspond to the latest weather model modules (spack-stack 1.6.0) |
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.
@mkavulich These changes look good. Just a small comment on a documentation change that I think should be reverted.
scm/doc/TechGuide/chap_quick.rst
Outdated
@@ -491,10 +501,11 @@ To see the full list of available options, use the ``--help`` flag: | |||
|
|||
The run script’s full set of options are described below, where optional abbreviations are included in brackets. | |||
If using the main branch, you should run the above command to ensure you have the most up-to-date list of options. | |||
There are no required arguments, but at least one of ``--case`` or ``--file`` must be specified. |
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.
This sentence is confusing. I would revert this change.
Can you add a sentence below that if no --suite is selected, the default SDF will be used? Which is GFS_V16
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 don't quite understand why this is confusing, is it the use of the term "required argument"? The script right now fails in a non-intuitive way if users don't specify one of these arguments, so I wanted to add a note about that specific requirement.
Maybe a better way forward is to just fix the script so it gives a clear error message when it doesn't get an argument for --file
or --case
, does that sound better?
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.
It's confusing, at least to me, that there are (a) no required arguments, but (b) --case or --file must be specified as an argument.
But yeah, I agree that the better approach is to modify run_scm.py to report error if (not args.case) and (not args.file), and reflect that here in the docs (plus the bit about the default suite).
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 for the feedback; I pushed an update, let me know what you think.
a2b6b09
to
adca29b
Compare
@mkavulich Can you pull in the latest main before we merge this, please? |
…orking and they arent necessary)
adca29b
to
0e51f53
Compare
The modulefiles have not been updated in a while and they do not work on some platforms. This PR updates the modulefiles for the latest spack-stack installations (1.6.0) on Derecho, Hera, Jet, and Orion; all the existing. It removes the Jet and Orion GNU modulefiles (I don't think they ever actually worked, these are not officially supported installations). Also, just a few fixes and clarifications for the users guide about spack-stack.
Successfully compiled on Derecho (intel and GNU), Hera (intel and GNU), Jet (Intel) and Orion (Intel). Many regression tests are failing at runtime (35/50), but this is a known problem that will be fixed in other PRs.
Finally, a fix to the supported suites list to remove SCM_RAP (which is no longer supported).