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

Set OPENMP=true in load script by default #364

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Apr 19, 2022

A developer can call ./conda/configure_compass_env.py with --without_openmp to prevent this variable from being set.

closes #347

@xylar xylar added clean-up dependencies and deployment Changes relate to creating conda and Spack environments, and creating a load script labels Apr 19, 2022
@xylar xylar self-assigned this Apr 19, 2022
@xylar xylar force-pushed the make_openmp_true_but_default branch from 26d3c18 to c65d0f0 Compare April 19, 2022 07:40
@xylar xylar marked this pull request as ready for review April 19, 2022 07:51
@xylar
Copy link
Collaborator Author

xylar commented Apr 19, 2022

@matthewhoffman, could you let me know if it's okay for MALI to have OPENMP=true even if you don't actually have OpenMP directives in your code? Is there overhead involved in doing that that you don't want to have?

@xylar
Copy link
Collaborator Author

xylar commented Apr 19, 2022

@mark-petersen, are you okay with this solution? Or do you feel more comfortable with the current behavior in which users have to specify OPENMP=true?

@xylar
Copy link
Collaborator Author

xylar commented Apr 19, 2022

@sbrus89, please take a look if you can.

@xylar
Copy link
Collaborator Author

xylar commented Apr 19, 2022

Testing

I made the build script once without --no_openmp and verified that:

export OPENMP=true

was in the load script. I then used the load script to build MPAS-Ocean and verified that it also showed OPENMP = true at the beginning of the build. Then, I rebuilt the load script, this time with --no_openmp and verified that export OPENMP=true was no longer present. This time, when I build MPAS-Ocean, I see OPENMP = at the beginning of the build.

@mark-petersen
Copy link
Collaborator

@xylar I just tested this on badger with:

./conda/configure_compass_env.py --conda /usr/projects/climate/mpeterse/miconda3 --compiler intel \
  --no_openmp --env_name no_openmp
source load_no_openmp_badger_intel_impi.sh

Without the --no_openmp flag I indeed get

echo $OPENMP
true

but if I then use a load script that was created with --no_openmp and source it, OPENMP=true remains. I think OPENMP should be either be blank or (better) set to false, even though MPAS only checks if it is true. Here is my sequence:

echo $OPENMP
    (blank)
source load_dev_compass_1.1.0-alpha.1_badger_intel_impi.sh (built without --no_openmp)
echo $OPENMP
   true
source load_no_openmp_badger_intel_impi.sh (built with --no_openmp)
echo $OPENMP
  true

This is the wrong logic, because one expects a flag like --no_openmp to have a resulting action.

are you okay with this solution? Or do you feel more comfortable with the current behavior in which users have to specify OPENMP=true?

I think this is a nice feature. Users can take advantage of it or set OPENMP themselves, as they prefer.

@xylar
Copy link
Collaborator Author

xylar commented Apr 20, 2022

@mark-petersen

This is the wrong logic, because one expects a flag like --no_openmp to have a resulting action.

Okay, I thought it was intuitive that this would not set OPENMP either way. So if you had previously sourced a load script or set OPENMP yourself, it would be unaffected. I can change the option to --without_openmp and set OPENMP=false.

In general, it is really hard to "deactivate" an compass load script. So it's not guaranteed to be safe to source one load script and then source another. (You will see some weird messages related to Spack if you source the same load script twice -- worth figuring out at some point.)

@xylar xylar force-pushed the make_openmp_true_but_default branch from c65d0f0 to e205eda Compare April 20, 2022 13:49
@xylar
Copy link
Collaborator Author

xylar commented Apr 20, 2022

@mark-petersen, could you see if the modified (and rebased) version works for you and is more intuitive?

It created a sensible load script for me in a local test. That's all I had time to test so far.

Copy link
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Yes, I tested this and it works perfectly now. Thanks!

Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

I tested the MALI integration suite and this appears to have no effect. In my first test the integration suite took 3 minutes longer than usual after compiling with the OPENMP=true set by the compass env, but I think that was just machine variability, because I tried it a second time and it ran 2 minutes faster than usual. There is no reason to think including this flag would affect anything, because MALI has no OpenMP directives in the MPAS code at present.

Copy link
Collaborator

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

@xylar, this works as expected for me too.

@xylar
Copy link
Collaborator Author

xylar commented Apr 21, 2022

Thanks everyone! I'll resolve the conflict and then merge.

A developer can call `./conda/configure_compass_env.py` with
`--no_openmp` to prevent this variable from being set.
@xylar xylar force-pushed the make_openmp_true_but_default branch from e205eda to 8f8a1e7 Compare April 21, 2022 17:09
@xylar
Copy link
Collaborator Author

xylar commented Apr 21, 2022

Rebased and "tested" in then sense of creating a load script with and without OPENMP.

@xylar xylar merged commit ca6799e into MPAS-Dev:master Apr 21, 2022
@xylar xylar deleted the make_openmp_true_but_default branch April 21, 2022 17:59
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request May 2, 2022
… (PR #4910)

MPAS Standalone: Add OpenMP support to gnu-nersc

For some reason this was omitted when OpenMP support was added 8 years
ago. This has become a problem in compass because we switched to building
with OpenMP on by default: MPAS-Dev/compass#364
Does not impact E3SM builds.

[BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request May 3, 2022
MPAS Standalone: Add OpenMP support to gnu-nersc

For some reason this was omitted when OpenMP support was added 8 years
ago. This has become a problem in compass because we switched to
building with OpenMP on by default: MPAS-Dev/compass#364
Does not impact E3SM builds.

[BFB]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up dependencies and deployment Changes relate to creating conda and Spack environments, and creating a load script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threaded tests should use OPENMP=true builds
4 participants