Skip to content

Conversation

jedwards4b
Copy link
Contributor

Description

Proposed change to handle list of support libraries in the driver rather than in cime.

Checklist

  • My code follows the style guidlines of this proejct (black formatting)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that excerise my feature/fix and existing tests continue to pass
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding additions and changes to the documentation

Copy link

codecov bot commented Aug 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.33%. Comparing base (256f24c) to head (48153cf).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4836      +/-   ##
==========================================
+ Coverage   55.28%   55.33%   +0.05%     
==========================================
  Files         266      266              
  Lines       38491    38493       +2     
  Branches     8323     8323              
==========================================
+ Hits        21279    21302      +23     
+ Misses      14856    14848       -8     
+ Partials     2356     2343      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jedwards4b for working on this PR. I could confirm that it builds and runs the Kokkos lib/kernels successfully in CAM, together with your other PR (ESCOMP/CMEPS#584).

CIME/build.py Outdated
if comp_interface == "nuopc" and (not ufs_driver or ufs_driver != "nems"):
libs.append("CDEPS")
# Build shared code of CDEPS nuopc data models
build_script = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is pointless, no? It's already set before the if clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a merge error, I'll clean it up - thanks.


if uses_kokkos(case) and comp_interface != "nuopc":
libs.append("ekat")
if uses_kokkos(case) and comp_interface != "nuopc":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartgol if I understood your suggestion from #4837 correctly, the uses_kokkos function can be deprecated since the host model will define the shared libs in its config_component.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, this code is only still here for backward compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my idea. I don't want to inject knowledge about host model tpls into CIME. All CIME should care about is a list of libs names, and where to find their build scripts.

@jasonb5
Copy link
Collaborator

jasonb5 commented Aug 20, 2025

@jedwards4b Could you document this under Variables section of https://github.com/ESMCI/cime/blob/master/doc/source/ccs/model-configuration/config-files.rst. Lets add a Build section to put this under. Should probably include a snippet of how the host would populate this via buildnml.

Can we comment which sections are being kept for backwards compatibility.

@jedwards4b
Copy link
Contributor Author

jedwards4b commented Aug 20, 2025

An issue that I think we still need to resolve is that some support libraries depend on others and
when all the libraries are defined in one place as is currently done it is easy to maintain an order so
that libraries are built in the correct order. How do we do this if several different buildnml scripts contribute
to the overall variable? One thought is that each buildnml include the dependencies specifying the order locally,
then in build.py in cime use libs = list(dict.fromkeys(libs)) to get an ordered list of unique libraries.

@bartgol
Copy link
Collaborator

bartgol commented Aug 20, 2025

An issue that I think we still need to resolve is that some support libraries depend on others and when all the libraries are defined in one place as is currently done it is easy to maintain an order so that libraries are built in the correct order. How do we do this if several different buildnml scripts contribute to the overall variable? One thought is that each buildnml include the dependencies specifying the order locally, then in build.py in cime use libs = list(dict.fromkeys(libs)) to get an ordered list of unique libraries.

Perhaps a non-trivial API change, but...what about asking that the buildlib.foo scripts also offer a command line option like --deps that return a list of libs names? So CIME can 1) get all lib names, 2) get build scripts for all libs, 3) call build.xyz --deps (perhaps with try/catch in case build script does not support this, in which case we assume "no deps", and host model already list them in "correct order") for all libs, to gather deps of each lib, 4) order libs so that if libA comes before libB, we can build B after A.

Alternatively, we can ask that buildnml (or config_compset.xml, depending on where this logic should be impl-ed) adds ALL libs (including deps) in the correct order. So, eg., if EAMxx in E3SM needs kokkos and for some reason kokkos depends on csm_share, it must add the libs "csm_share,kokkos" (and not just "kokkos"). CIME will concat all lists, and remove duplicates.

@jedwards4b
Copy link
Contributor Author

@bargol your alternate solution is what I was suggesting and have implemented.

@jedwards4b
Copy link
Contributor Author

@bartgol I have added ESCOMP/MOM_interface#276 to show what this looks like in a particular component and have tested this together with the CMEPS and cime changes.

@jedwards4b jedwards4b marked this pull request as ready for review August 27, 2025 14:06
@jedwards4b
Copy link
Contributor Author

@jasonb5 I am ready to merge this with you're review.

Copy link
Collaborator

@jasonb5 jasonb5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@jedwards4b Added some tests and a warning about using a deprecated method.

@jedwards4b jedwards4b merged commit c9af5ec into ESMCI:master Aug 28, 2025
10 of 11 checks passed
@jedwards4b jedwards4b deleted the move_liblist_to_driver branch August 28, 2025 13:46
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.

move support libraries to driver
4 participants