Updates in template.cupid and config_workflow.xml for cupid workflow #210
Conversation
billsacks
left a comment
billsacks
left a comment
There was a problem hiding this comment.
This looks really great, and is a nice step towards having CUPiD integrated more fully into the user workflow. As @TeaganKing requested, I did a bunch of testing of the bash logic, and this all seems like it's working! I have a few small suggestions, and am also pointing out variables that aren't yet used (but it's fine to keep these place-holders in for now).
|
Thanks for all your comments, @billsacks ! Summary of currently unused vars:
|
…fig_for_cesm_case.py
Teagan and I added a bunch of xml variables to env_postprocessing.xml (in a CUPiD PR), so template.cupid now uses those variables to modify the CUPiD config. Also, we moved case.cupid into the default workflow (but RUN_POSTPROCESSING is FALSE, so it won't run unless the user changes that variable to TRUE)
TeaganKing
changed the title
billsacks
left a comment
billsacks
left a comment
There was a problem hiding this comment.
Thanks for your comments and changes, @TeaganKing - and for all of your work in getting CUPiD plugged in to the CESM workflow! This looks good to me, though I noticed one thing that I think may be a typo - see my one new comment below. Once you check on that, let me know if you feel this is ready to merge from your perspective; if so, I can merge it.
machines/template.cupid
Outdated
| if [ "${CUPID_RUN_ADF}" == "TRUE" ]; then | ||
| {{ SRCROOT }}/tools/CUPiD/helper_scripts/generate_adf_config_file.py \ | ||
| --cesm-root {{ SRCROOT }} \ | ||
| --cupid-config-loc {{ SRCROOT }}/tools/CUPiD/examples/{{ CUPID_EXAMPLE }} \ |
There was a problem hiding this comment.
Just want to check here: is it correct to have {{ CUPID_EXAMPLE }} here (which I think suggests it gets filled in from the template?) instead of "${CUPID_EXAMPLE}" (from the xml variable)? I'm thinking it should be the latter, but I may be missing something.
Actually, I see that this has some dependencies, so I'm thinking we won't want to merge this until the dependencies are merged, but let me know if I'm misunderstanding. |
|
Yes, this does have some dependencies, so we need to hold off on actually merging it. I just wanted to see if I had adequately addressed your requested changes. Thanks for all of your suggestions! |
billsacks
left a comment
billsacks
left a comment
There was a problem hiding this comment.
Thanks for the fix @TeaganKing - looks good to me now!
4 participants
This PR will provide updates to
template.cupidin order to utilize additional xml variables which provide more flexibility for running CUPiD via the CESM workflow.