-
Notifications
You must be signed in to change notification settings - Fork 19
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
Propagate errors in SetServices upwards #1002
Propagate errors in SetServices upwards #1002
Conversation
…Services return codes so that errors in a GridComp's SetServices are reported properly.
(Sorry, still working on getting that CLA signed. Just submitting this here and now because I'm also fixing it in GCHP right now.) |
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.
Good catch. I'm surprised this has not been found before - I guess SetServices tends to have few/easy bugs.
Blocking for CLA. Yay lawyers! |
I'll ping the admins again today and see if they've figured out who need to sign it. Sorry again about the delay with that! |
No need to apologize. Delays are expected for this sort of thing. Might take weeks of back and forth once the right lawyer is involved. They may want to contact the NASA lawyer, etc. |
@LiamBindle A big MAPL Generic refactoring just went into |
@mathomp4 Thanks for letting me know. If the refactoring includes a fix so SetServices errors are propagated, we can close this. |
This issue has been automatically marked as stale because it has not had recent activity. If there are no updates within 7 days, it will be closed. You can add the "long term" tag to prevent the Stale bot from closing this issue. |
This issue should not go stale. It's an "easy" one that should be addressed. |
This issue was resolved. Do not need anymore |
@LiamBindle If you get a chance - please confirm that we've fixed this separately. (Came up as in issue independently.) |
This issue has been automatically marked as stale because it has not had recent activity. If there are no updates within 7 days, it will be closed. You can add the "long term" tag to prevent the Stale bot from closing this issue. |
Making a comment to satisfy Stale Bot. @LiamBindle when you have a chance: is this taken care of already as @tclune and @weiyuan-jiang suggest? |
@LiamBindle What's the status of your CLA signing capability? Would be nice to pull these in |
Hey @mathomp4, sorry I missed this. I didn't see it. I gave up on the CLA process. It got lost between lawyers, and it turned into a real headache. I'd suggest fixing it in the offical code and not merging this request. |
Description
I don't think errors in the GEOS-Chem gridded component's SetServices are getting caught properly. A failed
_VERIFY
in SetServices doesn't stop the simulation.Is there supposed to be a
_VERIFY(STATUS)
and_VERIFY(SS_STATUS)
, whereSS_STATUS
is the userRC from ESMF_GridCompSetServices, in MAPL_AddChildFromMeta?Related Issue
Motivation and Context
I'm working on improving the error reporting in GCHP. Errors related to the config files often result in hard crashes and the error messages can be hard to decipher. I believe this is because
MAPL_AddChildFromMeta
isn't verifying theuserRC
(STATUS
from the grid comp's SetServices).How Has This Been Tested?
I ran a GCHP simulation with an intentional error in the root config (GCHP.rc) file. The intentional error was what happened in geoschem/GCHP#134.
Log without this PR
Log after this PR
Types of changes
Checklist: