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 external land use data tool as a submodule to fates #1239

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glemieux
Copy link
Contributor

@glemieux glemieux commented Aug 20, 2024

Description:

This simple PR adds the land use data tool as a submodule to fates in the tools directory. The intent of this is to provide more "natural" discovery of the land use data tool, while allowing the tool to live in its own repository. It also removes the older luh2 directory which is superseded by the submodule.

Collaborators:

@ckoven

Expectation of Answer Changes:

No expected answer changes as this affects tooling only.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided

Documentation

Test Results:

Testing in the following way TBD:

  • successful recursive git submodule checkout using CTSM and E3SM host models

@glemieux glemieux added type: tools This PR adds or updates support tools. No regression testing necessary. PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. labels Aug 20, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Aug 20, 2024

@glemieux looking at this, this should work fine with git-fleximod on the CESM/CTSM side of things. But, we'll want to make sure. There might be some additional fields that should (or could) be added for it. We should think about that a bit. I think the way this works with submodules is that it gives you the latest version on master for the submodule -- which is maybe what's desired here. But, that does mean that behavior of the tool can change out from under you as the main branch undergoes development...

@glemieux
Copy link
Contributor Author

glemieux commented Aug 20, 2024

@ekluzek I was just testing out the git-fleximod on my local ctsm repo. Conducting the update procedure as I understand it (i.e. update .gitmodules and checkout and commit the test fates branch) appears to have worked correctly: the landusedata directory is populated with the data tool repo contents. I also tested the case in which I have made the aforementioned update and then locally checked out within fates a different branch and git-fleximod appropriately checked out the PR branch on my test ctsm branch. That said it did report the following:

e    tools/landusedata has no fxtag defined in .gitmodules, module at v0.1.0-24-g88108f3
               fates updated to tool-landuse-submodule

I'm not sure if this will be confusing to ctsm users, so perhaps it should be defined in the ctsm .gitmodules file in a future fates api update?

Your point about the behavior is well taken. For now we just really want to get the latest commit from the tool repo main branch as there really isn't any need to coordinate the tool's version relative to the fates version, but there could come a time in the future in which that would be desirable.

@glemieux
Copy link
Contributor Author

Testing note: using git submodule update --init --recursive in e3sm works as expected and pulls in the land use data tool contents.

@ekluzek
Copy link
Collaborator

ekluzek commented Aug 20, 2024

@glemieux we do see a similar message for cprnc in cime, so this is maybe OK.

You could add the following two that would add a little more clarity for git-fleximod in CESM and be ignored by submodules in E3SM:

fxrequired = AlwaysRequired
# Standard Fork to compare to with "git fleximod test" to ensure personal forks aren't committed
fxDONOTUSEurl = https://github.com/NGEET/tools-fates-landusedata

But, then this might be confusing on the E3SM side of things. And it might not help enough on the CESM side.

If you lifted it outside of FATES and handled by the HLM that would allow it to be done differently between the two. But, if what we want for both is to get the head of the main branch does that help enough?

@glemieux
Copy link
Contributor Author

@ekluzek adding this to the fates .gitmodules sounds like a good idea, but I tried implementing this on a fork from this branch and it didn't see to clear up that particular output. Am I understanding that was the intent of your suggestion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. type: tools This PR adds or updates support tools. No regression testing necessary.
Projects
Status: Under Review
Development

Successfully merging this pull request may close these issues.

2 participants