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 Repositories #681

Merged
merged 12 commits into from
Dec 3, 2023
Merged

Conversation

biljanaorescanin
Copy link
Contributor

This PR is to update repos in components.yaml file to newer versions and to add option to ldas_setup to use either cas or sky.

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin, @mathomp4 : After seeing Matt's edits of README.md, I now realize that my latest commit isn't going to work.
I guess the idea of the preceding commit by Matt would have the user to choose Milan via the n-tasks-per-node setting. That should work, but we might need more documentation in the batinp file to explain the SLES12 (cas, sky) vs. SLES15 (mil) difference. Thoughts?

@mathomp4
Copy link
Member

@biljanaorescanin, @mathomp4 : After seeing Matt's edits of README.md, I now realize that my latest commit isn't going to work. I guess the idea of the preceding commit by Matt would have the user to choose Milan via the n-tasks-per-node setting. That should work, but we might need more documentation in the batinp file to explain the SLES12 (cas, sky) vs. SLES15 (mil) difference. Thoughts?

@gmao-rreichle Yes. You can't do that. In the GCM I use the fact that CMake knows where you build to "lock" a user into what they can run on:

https://github.com/GEOS-ESM/GEOSgcm_App/blob/develop/gcm_setup#L439

At CMake time, cmake fills in the BUILT_ON_SLES15 variable with TRUE or FALSE. My guess is working with @weiyuan-jiang it should be possible to do something similar with ldas_setup by maybe throwing an Error if a user tries to use a SLES12 build on SLES15 and vice versa.

@mathomp4
Copy link
Member

Also it might be a while before I figure out how to do nightly testing scripts to work on Milans. The main issue is that discover-cron is SLES12 and some bits of the scripting run there while other bits run on the compute nodes you get. That mix of OSs causes nightmares (see 5 releases of ESMA_env in 3 days)

Disentangling that will be a challenge. I don't think impossible, but a challenge. For now, I'd recommend using those scripts only on SLES12.

@biljanaorescanin
Copy link
Contributor Author

biljanaorescanin commented Oct 26, 2023

@mathomp4 it successfully compiled with ./parallel_build.csh -mil it created two directories build-SLES15 and install-SLES15.
@gmao-rreichle I will remove added constrain for mil .

@biljanaorescanin
Copy link
Contributor Author

We pass all Nightly Tests.

@biljanaorescanin biljanaorescanin marked this pull request as ready for review October 27, 2023 11:53
@biljanaorescanin biljanaorescanin requested review from a team as code owners October 27, 2023 11:53
tclune
tclune previously approved these changes Oct 27, 2023
Copy link
Contributor

@tclune tclune left a comment

Choose a reason for hiding this comment

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

infrastructure changes ok

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin, @mathomp4 :
After meeting with @weiyuan-jiang and @biljanaorescanin today, we decided to go ahead with this PR and tick up the repo versions but leave the Milan capability for later because it's not ready yet. To this end, I removed the Milan-specific code and comments from the PR. While I was at it, I also removed the Haswell-specific stuff since Haswells are now extinct.

Since I touched code in ldas_setup, the PR probably needs to be tested again. Having said that, I also noticed that all of the repos now have newer versions than what's on the present PR at the moment. If the goal is to update to the latest version, shouldn't we change components.yaml one more time so it includes the latest version of each repo at this time? I assume this works for the following:
Baselibs: 7.16.0
env: v4.23.0
cmake: v3.36.0
GMAO_Shared: v1.9.6
MAPL: v2.42.1

The only exception is GEOS_Util, for which the latest version v2.0.4 includes changes of remap_restarts.py, which is addressed separately in #680.

Unless @mathomp4 has an objection, I suggest that @biljanaorescanin should edit components.yaml again using the latest repo versions (except GEOS_Util) and then retest. If that works, we can go ahead and merge the PR.

For reference and later use, here's a diff showing the Milan-specific code and text that I removed/edited:
https://github.com/GEOS-ESM/GEOSldas/compare/4b157b44c72d1b2206cd9e02ffeab2f055f6719f..2006fa2d1c1542da32aa93ed41947d7126df8990?w=1

@biljanaorescanin
Copy link
Contributor Author

Since I touched code in ldas_setup, the PR probably needs to be tested again. Having said that, I also noticed that all of the repos now have newer versions than what's on the present PR at the moment. If the goal is to update to the latest version, shouldn't we change components.yaml one more time so it includes the latest version of each repo at this time? I assume this works for the following: Baselibs: 7.16.0 env: v4.23.0 cmake: v3.36.0 GMAO_Shared: v1.9.6 MAPL: v2.42.1

I've tested PR with suggested options and for Nightly tests, one test fails, that is aggglobalcs test for assim comparison.

Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

I verified that the comparison failure of the AGGGLOBALCS/assim test is just roundoff, as expected. Approving.

@gmao-rreichle gmao-rreichle merged commit 5669de0 into develop Dec 3, 2023
5 checks passed
@gmao-rreichle gmao-rreichle deleted the feature/borescan_update_components branch December 3, 2023 16:09
@@ -57,7 +69,7 @@ a) Set up the job as follows:
cd (build_path)/GEOSldas/install/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this would need to be something like cd (build_path)/GEOSldas/install-SLES12/bin
Let's fix it when we include the SLES15 vs SLES12 instructions in #693

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants