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.

Sign up for GitHub

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

Jump to bottom

Moving critical toolchain scripts to /python and creating wrapper scripts for them #2156

Closed
wants to merge 39 commits into from
Closed

Moving critical toolchain scripts to /python and creating wrapper scripts for them #2156

wants to merge 39 commits into from

Conversation

slevis-lmwg
Copy link
Contributor

@slevis-lmwg slevis-lmwg commented Sep 13, 2023 •
edited
Loading

Description of changes

  • Move scripts to /python/ctsm/site_and_regional
    subset_data.py (DONE before this PR)
    run_neon.py
    neon_surf_wrapper.py
    modify_singlept_site_neon.py
  • Make wrapper scripts for them in /tools/site_and_regional
    subset_data (DONE before this PR)
    run_neon
    neon_surf_wrapper
    modify_singlept_site_neon
  • Add testing

Specific notes

Contributors other than yourself, if any:
@TeaganKing and hackathon participants (2023/9/13)

CTSM Issues Fixed (include github issue #):
Fixes #1441

Are answers expected to change (and if so in what way)?
No

Any User Interface Changes (namelist or namelist defaults changes)?
The wrapper scripts do not have .py suffixes.

Testing performed, if any:
./run_neon --help
./run_neon --neon-sites ABBY
...the latter still running as we speak.

@slevis-lmwg slevis-lmwg self-assigned this Sep 13, 2023
@TeaganKing TeaganKing self-assigned this Sep 13, 2023
@slevis-lmwg slevis-lmwg added the code health improving internal code structure to make easier to maintain (sustainability) label Sep 13, 2023
@TeaganKing TeaganKing added PR status: work in progress and removed code health improving internal code structure to make easier to maintain (sustainability) labels Sep 13, 2023
@slevis-lmwg slevis-lmwg added code health improving internal code structure to make easier to maintain (sustainability) testing additions or changes to tests and removed PR status: work in progress labels Sep 13, 2023
@TeaganKing
Copy link
Contributor

TeaganKing commented Oct 5, 2023 •
edited
Loading

A few comments on unit test additions that I made after the hackathon today:

  1. For neon_surf_wrapper, execute(subset_command) and execute(modify_command) should probably be in the sys tests; the parser test probably isn't really worth unit testing, but I added it anyways so that the unit testing structure for neon_surf_wrapper is ready for any future additions.
  2. Perhaps valid_neon_sites in modify_singlept_site_neonshould come from a similar list as that in run_neon rather than a hardcoded list within modify_singlept_site_neon; this may be outside the scope of this PR but also probably not too hard to change.
  3. Some of the modify_singlept_site_neon tests would require pre-existing files. Where can we find these? I marked a few notes where this could be useful with "TODO".
  4. These still need pylint/black checking. I'll plan to do that in a separate commit (along with run_neon formatting) in order to avoid the 'blame' issue for those files.

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 5, 2023

@TeaganKing this all sounds really great. I just wanted to comment on your item "3" above.

What I've done with files that we need for the python system tests is I'll usually copy them from existing files that we have from inputdata. Since, we can't guarantee inputdata find files will be accessible, it's best to explicitly add the files that are needed for testing with the names we need for the tests. We've set this up using "git lfs" which does require a small amount of setup. See the README files in that directory for this. We still try to only put files in there that are as small as possible.

@TeaganKing
Copy link
Contributor

Got it! Thanks for these details. I did see some files that were stored with Git LFS. I think we'll just need to determine which files we want to use (or if we can use any files that are already there), and then I'll be sure to add them with large file storage.

@ekluzek
Copy link
Collaborator

ekluzek commented Nov 2, 2023

@TeaganKing @samsrabin has the solution above! 3 point swish for him!

I tried his suggestion out and it's now working for me. So let's still meet this afternoon and go over what to do for the final version.

@TeaganKing
Copy link
Contributor

Sounds great! And that's fantastic, thanks so much for sharing your previous experience and solutions to this issue, @samsrabin !

@samsrabin
Copy link
Collaborator

All credit to @billsacks for the trace there!

slevis-lmwg
slevis-lmwg commented Nov 2, 2023
View reviewed changes
python/ctsm/test/test_sys_run_neon.py Outdated
@@ -58,7 +58,7 @@ def test_one_site(self):

# assert that BART directories were created during setup
self.assertTrue("BART" in glob.glob(self._tempdir + "/BART*")[0])
#self.assertTrue("BART" in glob.glob("BART*")[0])
# self.assertTrue("BART" in glob.glob("BART*")[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to keep this alternate line around as a comment, or could we remove? I'm fine either way if there's justification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, I can remove that! Thanks for catching this.

@TeaganKing
Copy link
Contributor

After discussing some additional changes with @ekluzek and @slevis-lmwg , I have made those changes and updated the ChangeLog. So, this is ready for a new tag!

slevis-lmwg
slevis-lmwg commented Nov 2, 2023
View reviewed changes
.git-blame-ignore-revs
@@ -10,6 +10,9 @@ b88e1cd1b28e3609684c79a2ec0e88f26cfc362b
b771971e3299c4fa56534b93421f7a2b9c7282fd
9de88bb57ea9855da408cbec1dc8acb9079eda47
8bc4688e52ea23ef688e283698f70a44388373eb
4ee49e3e516ca7dee5df378f65664f93a7db4415
0207bc98dd5c75cd69a0e788bc53e41093712f5c
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder to add e4d3868
@TeaganKing I'm happy to do this unless you get to it first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@slevis-lmwg
Copy link
Contributor Author

Sorry, I forgot to ask something else:
Does this PR introduce a new /testinputs file? If so, is there anything that I need to do about that during the merge?

@TeaganKing
Copy link
Contributor

No updates to testinputs; we just used pre-existing test inputs to perform all of these tests.

@TeaganKing
Copy link
Contributor

We will need to update the tag to dev153 in ChangeLog.

@samsrabin samsrabin added the PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete label Nov 9, 2023
@slevis-lmwg
Merge remote-tracking branch 'escomp/master' into single_point
32d334c 
Resolved conflicts:
.git-blame-ignore-revs
doc/ChangeLog
doc/ChangeSum
@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Nov 14, 2023 •
edited
Loading

make all: OK (in a meeting with @TeaganKing and @ekluzek we decided to ignore pylint suggestions)
clm_pymods: PASS (I had forgotten ./manage_externals/checkout_externals before...)

@slevis-lmwg
Copy link
Contributor Author

The plan is to merge this PR with a few other PRs listed in this card.

@slevis-lmwg slevis-lmwg mentioned this pull request Nov 14, 2023
Merged
@slevis-lmwg slevis-lmwg deleted the single_point branch November 15, 2023 00:45
@ekluzek ekluzek mentioned this pull request Nov 15, 2023
Closed
@wwieder wwieder mentioned this pull request Nov 16, 2023
Merged
@TeaganKing TeaganKing mentioned this pull request Feb 26, 2024
Merged
This was referenced May 1, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TeaganKing TeaganKing TeaganKing left review comments

@wwieder wwieder wwieder approved these changes

@ekluzek ekluzek ekluzek approved these changes

Assignees

@slevis-lmwg slevis-lmwg

@TeaganKing TeaganKing

Labels
code health improving internal code structure to make easier to maintain (sustainability) PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete testing additions or changes to tests
Projects
None yet
Milestone
No milestone
5 participants
@slevis-lmwg @TeaganKing @ekluzek @samsrabin @wwieder