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

Remove duplication between gen_mksurfdata_jobscript_multi and gen_mksurfdata_jobscript_single #2439

Open
ekluzek opened this issue Mar 25, 2024 · 4 comments
Labels
code health improving internal code structure to make easier to maintain (sustainability) enhancement new capability or improved behavior of existing capability test: mksurfdata Test mksurfdata_esmf before merging

Comments

@ekluzek
Copy link
Collaborator

ekluzek commented Mar 25, 2024

This was brought up in #2290. gen_mksurfdata_jobscript_single has parts that are duplicated with gen_mksurfdata_jobscript_multi and hence changes have to go in two places. This was for example an issue in #2427 which means that extra testing needs to be done because of that duplication.

An improvement to the design would be to have gen_mksurfdata_jobscript_single as the base level script that is expanded on by gen_mksurfdata_jobscript_multi which then doesn't duplicate code, but only adds the additional functionality needed there.

This is something that we will do as a post-CTSM5.2 development.

@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability code health improving internal code structure to make easier to maintain (sustainability) labels Mar 25, 2024
@ekluzek
Copy link
Collaborator Author

ekluzek commented Mar 26, 2024

Note, this also helps with #2341 and will help getting mksurfdata_esmf work on any CESM supported machine.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Mar 28, 2024

Digging into the issues with external updates for ctsm5.2 I now see that some development here is important to do. I've done some work here, I've tried to limit it to what's important in order to have things working and to easily identify problems with externals as quick as possible.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Mar 29, 2024

One idea to do later would be to separate single and multi into a class that has a base class used by both, and then extends it for each specific script.

This was brought up here:

https://github.com/ESCOMP/CTSM/pull/2427/files#r1543707838

Athough #2427 does some work on this. Some other duplication that would be useful to remove would be:

  • Remove duplication in running the .env_mach_specific.sh script
  • The write of the part of the script that does the actual
  • The write of the bash error checks for previous commands
  • The write of the ending parts for success...
  • Move of some utilities at the top to a shared utility level

@ekluzek
Copy link
Collaborator Author

ekluzek commented Mar 29, 2024

A really good point that @samsrabin gave here for doing this is that the current structure does NOT make it obvious that changes in gen_mksurfdata_jobscript_single MAY also the multi script. That link is unlikely to be obvious to the Developer working on single, and as such they may do something that breaks the multi script. This is a really good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability) enhancement new capability or improved behavior of existing capability test: mksurfdata Test mksurfdata_esmf before merging
Projects
None yet
Development

No branches or pull requests

3 participants