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

Conditional for NEON usermods is too broad #2039

Closed
wwieder opened this issue Jun 22, 2023 · 15 comments · Fixed by #2044
Closed

Conditional for NEON usermods is too broad #2039

wwieder opened this issue Jun 22, 2023 · 15 comments · Fixed by #2044
Labels
bug something is working incorrectly priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations

Comments

@wwieder
Copy link
Contributor

wwieder commented Jun 22, 2023

This statement is still true in a BGC case because of the extra wildcard

https://github.com/ESCOMP/CTSM/blob/bb2a8d2c0c05ebb5417724f98fb0586dc7584ae6/cime_config/usermods_dirs/NEON/defaults/shell_commands#L35C1-L36C1

Something like the following would be more accurate

if [[ $compset =~ .*CLM[0-9]+%SP* ]]; then

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 22, 2023

The one problem there is that SP isn't always immediately after the % in the compset name. One way to do this would be to have a wildcard imbetween the % and "SP" that doesn't match underscores.

So maybe...

if [[ $compset =~ .*CLM[0-9]+%[^_]*SP.* ]]; then

@wwieder
Copy link
Contributor Author

wwieder commented Jun 22, 2023

Yes! That suggestion works @ekluzek

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 22, 2023

It looks like my comment only applies to the FATES-SP compset which doesn't matter now, and the NWP compsets like: 2000_DATM%NLDAS2_CLM50%NWP-SP_SICE_SOCN_MOSART_SGLC_SWAV (I2000Ctsm50NwpSpNldas)

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 22, 2023

These regular expressions are notoriously tricky, but for longevity good to get right.

@ekluzek ekluzek added priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations bug something is working incorrectly labels Jun 22, 2023
@wwieder
Copy link
Contributor Author

wwieder commented Jun 23, 2023

I don't understand why, but @ekluzek's suggestion if [[ $compset =~ .*CLM[0-9]+%[^_]*SP.* ]]; then works as intended when creating a clone, but not for using run_neon.py for a new case and building it?

@briandobbins
Copy link
Contributor

briandobbins commented Jun 23, 2023 via email

@briandobbins
Copy link
Contributor

Updated. And, just to be more clear, a './run_neon.py' from your ~/CTSM directory should've worked fine, whereas 'qcmd -- run_neon ...' would've launched via the central directory, and not had that regex fix. (It does now.)

@wwieder
Copy link
Contributor Author

wwieder commented Jun 28, 2023

One more issue that needs correction is for shell commands at SJER. Which should look like this example from TEAK.

Were there other manual changes you made for the tutorial @briandobbins and @TeaganKing ?
@ekluzek is this the right place to log these other NEON bugs that can be addressed in #2044?

@TeaganKing
Copy link
Contributor

TeaganKing commented Jun 28, 2023

There was a CTSM shell command update that needs to be updated in the cloud instance specifically in order to switch off the mpi-serial option, but that change is not desired for the general CTSM use. Perhaps there would be some way to have an if statement for the case when we are on the cloud versus on Cheyenne, or we could continue updating the CTSM version used in the cloud.

Other than that, there were a few tutorial changes that were already pushed to the repository prior to Flux Course, and some manual changes to the scripts used for setting up the AWS instance (but not involving the CTSM nor tutorial repositories).

If we are making some additional updates, can we also make the run_neon.py update to ensure that we use is not instead of != and avoid printing additional warnings? This is also mentioned in this issue

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 28, 2023

@wwieder @TeaganKing yes let's collect all the little things you had to do for the tutorial here. So please add anything you haven't mentioned yet. I'll organize the things you've already mentioned and make sure they are coming in the new PR.

@briandobbins is there a way to detect if the cloud is being used?

@wwieder
Copy link
Contributor Author

wwieder commented Jul 25, 2023

@ekluzek are there any upcoming PRs that we can include these fixes for NEON usermods?

@ekluzek
Copy link
Collaborator

ekluzek commented Jul 25, 2023

I've got this fix started in #2044 and it's listed in upcoming tags as another set of simple fixes.

@wwieder
Copy link
Contributor Author

wwieder commented Oct 19, 2023

@ekluzek we didn't get to discuss when your long list of simple fixes will be coming in the upcoming tag queue. Is this something that's basically ready to go? I'm asking because we need to update the NEON container, and it would be good have have this merged to main.

Thanks
Will

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 20, 2023

@wwieder the plan is to get back to that ASAP, which should be right away. When do you need to update the NEON container? I hope we can coordinate this and make it happen when you need it...

@wwieder
Copy link
Contributor Author

wwieder commented Oct 20, 2023

Thanks, @ekluzek. We can't update the container until Brian is back, but after he returns I'm hoping to be able to do this quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants