Support importing buildnml from cime_config/{compname}_cime_py #4130
Conversation
Proof of concept commit: see update_cime_config branch in https://github.com/mnlevy1981/POP2-CESM.git for companion component. If preview_namelist can import {compname}_cime_py, then the script will call buildnml() from this package instead of using run_sub_or_cmd(). To help find this package, the script will add each component's cime_config/ directory to the system path (and the POP branch above has a directory cime_config/pop_cime_py/ with a simple __init__.py file in it). Lots of improvements still needed: 1. Support putting buildnml.py in SourceMods/ and figure out a way to import it 2. Is there a cleaner way to ensure buildnml() is coming from where we expect it to be? 3. Does it make more sense to push some of this logic into run_sub_or_cmd()?
The previous commit added logic to preview_namelists.py to specifically allow the import of buildnml if a component was set up correctly; this commit reverts preview_namelist to what is on master, but adds a wrapper to run_sub_or_cmd() in util.py that attempts to import and run the command before falling back to the old run_sub_or_cmd() function. preview_namelists.py has been updated to use this new import_and_run_sub_or_cmd() function (which perhaps could use a better name)
|
@billsacks and I talked yesterday, and ended up with some pretty nice changes. One big remaining concern that needs to be addressed before taking this PR out of draft status is how to deal with users who want to put
But I think supporting code in source mods may make it very difficult to refactor python code in |
|
Thanks for your work on this @mnlevy1981 ! I agree that it's going to be awkward to support testmods with this, but if we want to keep this backward compatible, I would vote for option (2) in your last comment. |
|
Maybe we should also issue a warning that this use of SourceMods for buildnml is deprecated if we detect that someone is using it. One idea (maybe overkill?) is that we could open an issue here in cime to remove support for this deprecated feature, then in the warning invite people to make a comment in the issue if this feature is critical for their workflow. If we haven't heard from many people in some amount of time, then we could remove that feature? |
|
I agree that we should not support putting cime python code into SourceMods. |
I'm okay with this rule of thumb, but should it apply to |
|
I would call it cime code. Are there frequent cases where we make this substitution? If the user needs to make a change in buildnml then it seems to me that they should create a branch of the component and make the change there. |
I had added comments reminding myself of a couple of different approaches to try for implementing the features on this branch. Now that at path has been chosen, those notes are not necessary.
|
I've removed support for putting One question: should the component put any other modules in this directory? I think we've discussed how |
There was a problem hiding this comment.
Thanks a lot for your work on this @mnlevy1981 ! This looks good to me.
Regarding your question:
One question: should the component put any other modules in this directory? I think we've discussed how buildlib is not written in a way to easily refactor in this way, but I wasn't sure if this new import scheme could be used for any other scripts.
The one other thing I can think of is cime_config/SystemTests (used by ctsm and other components: https://github.com/ESCOMP/CTSM/tree/master/cime_config/SystemTests). I'm fine with that being added later, though.
I feel like the next step here will be figuring out a robust & consistent pattern for allowing components to also include some python code from a different location. For ctsm, for example, I think we'll eventually want buildnml (and the SystemTests) to be able to access python code in the main ctsm python module (https://github.com/ESCOMP/CTSM/tree/master/python/ctsm ). Maybe this should be handled in an "official" way via config_files (adding a specification for an optional directory to add to sys.path for this component), or maybe we should just do this in a more ad-hoc way (e.g., having buildnml.py adjust the sys.path itself). (I prefer the more official way of using config_files, because I think it will be less error-prone long-term.) But that can also be added later.
Proof of concept commit: see ESCOMP/POP2-CESM#62 for companion component PR.
Basically, we want CIME to import
buildnml()for each component froma package rather than using the logic in
run_sub_or_cmd(). For thefirst commit, I just modified
preview_namelists.pydirectly, but I thinkthese changes probably belong in
run_sub_or_cmd()itself?Test suite: no testing yet, still working out the details of the change
Test baseline: N/A
Test namelist changes: N/A
Test status: N/A
Fixes #4076
User interface changes?: N
Update gh-pages html (Y/N)?: N (I don't think so, at least)