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

Updates to make GEOSldas infrastructure similar to GEOSgcm #501

Merged
merged 2 commits into from
Dec 22, 2021

Conversation

mathomp4
Copy link
Member

This is a bit of a "backend" sort of PR. Changes include:

  • Update to use Intel 2021.3 CI image (match compiler on Discover)
  • Fixes to CMake that should allow the use of any of the "mepo styles" (i.e., prefix: @MAPL/, postfix: MAPL@/, bare: MAPL/)

I'm calling this "0-diff trivial" as it should be, but as always, @biljanaorescanin should test.

@mathomp4 mathomp4 added the 0-diff trivial very, very obvious 0-diff change label Dec 21, 2021
@mathomp4 mathomp4 requested review from a team as code owners December 21, 2021 20:08
@biljanaorescanin
Copy link
Contributor

I will run tests now.

@mathomp4
Copy link
Member Author

I will run tests now.

Thanks!

@gmao-rreichle
Copy link
Contributor

I will run tests now.

Do we need the full suite of tests? If the model still builds ok, it should be trivially 0-diff, from what I'm seeing and as labeled by @mathomp4.

@biljanaorescanin
Copy link
Contributor

Since tests were already running when you asked @gmao-rreichle I've let them finish. All tests passed.

@mathomp4
Copy link
Member Author

Since @tclune and @bena-nasa are out, we'll probably need @weiyuan-jiang to approve this for the @GEOS-ESM/cmake-team . I can't approve my own PR. (Well, I could as a superuser, but that defeats the purpose. 😄 )

@gmao-rreichle gmao-rreichle merged commit c1c7a66 into develop Dec 22, 2021
@gmao-rreichle
Copy link
Contributor

@mathomp4: I looked at the PR and did not see approval by the Cmake team, but the green button to merge the PR was available, so I thought I should give it a go. To my surprise, I was able to merge the PR, which I shouldn't have been able to do given the lack of approval by the CMake team. Maybe I'm just misinterpreting things, but seems odd.
cc: @biljanaorescanin @weiyuan-jiang

@mathomp4
Copy link
Member Author

@mathomp4: I looked at the PR and did not see approval by the Cmake team, but the green button to merge the PR was available, so I thought I should give it a go. To my surprise, I was able to merge the PR, which I shouldn't have been able to do given the lack of approval by the CMake team. Maybe I'm just misinterpreting things, but seems odd. cc: @biljanaorescanin @weiyuan-jiang

Ohhh. This might be due to #485. I made the @GEOS-ESM/ldas-gatekeepers also codeowners of their own CMake files because, well, I thought you should be! Sometimes you all might add a single file to a sources list and why wait for Tom or I to approve something like that? I think in the beginning Tom and I wanted approve all changes since a lot of people were new to CMake. But now, people are pretty conversant with it, so you should have your own agency. You'd probably consult us for anything fancy anyway. And we are still notified of the CMake change.

We require a codeowner's approval and we require one approving review. So, that meant that one codeowner did approve of this PR... @biljanaorescanin ! (I'll look around GitHub docs and see if maybe there is an "all codeowners" must approve option. Probably not without requiring two approvals for everything...oof.)

@gmao-rreichle
Copy link
Contributor

@mathomp4: I see, and I had already forgotten the extra powers that the LDAS gatekeepers were granted...

We require a codeowner's approval and we require one approving review.

I think this is sensible.

maybe there is an "all codeowners" must approve option. Probably not without requiring two approvals for everything.

I think the old system where only CMake experts can approve CMake changes would be preferable to requiring approval from "all codeowners" or "two approvals". I'm flattered when you say that by now "people are pretty conversant with" CMake, but at least for me that's a bit optimistic. Put differently, if there are CMake changes that I don't understand (ie, most CMake changes), then I'd check with you or @weiyuan-jiang anyway. In practice, having CMake authority for LDAS gatekeepers may not be necessary. I'd either go back to the old system (no CMake authority for LDAS gatekeepers) or keep it as it is now with the understanding that we will tread carefully.

@gmao-rreichle gmao-rreichle deleted the feature/mathomp4/consistent-infrastructure branch December 22, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-diff trivial very, very obvious 0-diff change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants