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

Add some checks of compset validity #4754

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

billsacks
Copy link
Member

@billsacks billsacks commented Feb 15, 2025

I am opening this for discussion on whether people see value on having some checks like this in place, and if so, which checks should live in CIME (applying to all models) and which should be moved to a model-specific customization.

The recent motivation for this was that, in a recent CESM tag, we accidentally left out the river model from our fully-coupled compsets, and this initially went undetected. One contributor to this issue was the change made to CIME a few years ago which allows you to leave components out of compset long names, leading to them being set to stub components; there were good reasons for this change, but it does lead to more potential for error, both in defining compset aliases and in users creating their own compsets. But I had been thinking for a long time that it would be good to have some compset validation like this in place, so this recent issue just gave me the final motivation to prototype something like this.

I have divided these checks into three categories:

  1. "Standard" validity checks (currently just a rule about X components)
  2. Model-specific validity checks (many of these are derived from the checks @alperaltuntas developed in VisualCaseGen)
  3. Compset alias validity checks (suggested by @briandobbins) (i.e., checking / enforcing the meaning of A compsets, B compsets, etc.)

The division of checks into (1) and (2) is somewhat subjective (I could imagine some things I have currently put in (2) being moved to (1)). I'd like to hear thoughts on which, if any, of these categories of checks feels worth putting in place, and I'd also welcome discussion on which specific checks should be in place in each category. (There are currently checks in both (2) and (3) that would have prevented the recent issue in CESM.)

I recognize that there are tradeoffs here between, on the one hand, protecting users and ourselves from mistakes; and on the other hand, providing flexibility to do non-standard things. A lot of the discussion that I see happening here is around where that balance should fall. To at least partially address this tension, I imagine putting in place a flag to create_newcase and create_test that would allow bypassing these checks.

There aren't yet any unit tests for this, but I would add some if people want to move ahead with some of these compset validity checks.

Test suite: so far just manual testing
Test baseline: N/A
Test namelist changes: none
Test status: bit for bit

Fixes none

User interface changes?: N

Update gh-pages html (Y/N)?: N

@mnlevy1981
Copy link
Contributor

Did you check in with @alperaltuntas to see what he does in visualCaseGen? He already has a system in place to check things like "if you have an active ocean you can't have a stub atmosphere" and it probably makes sense to add to his checks rather than duplicate the intent in a different manner.

@billsacks
Copy link
Member Author

Did you check in with @alperaltuntas to see what he does in visualCaseGen? He already has a system in place to check things like "if you have an active ocean you can't have a stub atmosphere" and it probably makes sense to add to his checks rather than duplicate the intent in a different manner.

Yes. I should have mentioned that I used those checks as a reference point for many of the checks here... I just edited the comment to note that and credit @alperaltuntas . There are some small differences: a few cases where I deliberately made checks either stronger or weaker than what @alperaltuntas had, and it would probably be worth reconciling these.

I don't love that this means that the same logic is duplicated in both places, but I'm not sure what to do about that. The problem is that it's hard to integrate the visualCaseGen checks into CIME's standard operation because of the external library dependencies. Alper and I exchanged some emails about this, and my sense was that the best path forward would be to just duplicate the logic here, though I'm definitely open to feedback on that.

However, it may be that we drop all of the checks in the _model_specific_validity_checks function (which is what duplicates logic in visualCaseGen), just keeping the _alias_validity_checks; at least, that was @briandobbins 's initial thinking.

@jasonb5
Copy link
Collaborator

jasonb5 commented Feb 18, 2025

@rljacob @jgfouca Would this be useful for E3SM?

@rljacob
Copy link
Member

rljacob commented Feb 18, 2025

Yes it would be.

@billsacks
Copy link
Member Author

Based on discussion at this week's CSEG meeting: For CESM, we'd like to have the checks in _alias_validity_checks, but not the checks in _model_specific_validity_checks or in _check_x_comps: the feeling was that the latter checks may be too restrictive and/or too hard to maintain.

I had originally mentioned the idea of having a flag to bypass these checks, but my feeling is that, if the only checks are of aliases, then this flag is unnecessary. However, if E3SM is keeping more checks in place, perhaps you'll want such a flag?

So, questions for E3SM (@rljacob @jasonb5 @jgfouca ) to help guide how to move forward here:

  1. Do you have a sense of which of these checks you'd like?
  2. For the alias checks, my impression from https://acme-climate.atlassian.net/wiki/spaces/DOC/pages/1698332707/Compset+Naming+first+letter is that your checks will differ from ours; do you agree?
  3. At least if I'm right about (2), it seems like we'll want to use the model-specific customization hooks that @jasonb5 introduced. @jasonb5, at some point I'd like to either talk to you to get a sense of what will be needed for this, or (if you prefer) just let you demonstrate the addition of hooks for this for E3SM that we can replicate for CESM.

This isn't urgent, though it would be great if we can get this in within the next couple of months.

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

Successfully merging this pull request may close these issues.

4 participants