Skip to content

Properly handle toolchains defining openmp toolchain opt as dict (relevant for NVHPC toolchain)#5093

Merged
lexming merged 13 commits intoeasybuilders:developfrom
Thyre:toolchain-openmp-disable-flag
Feb 4, 2026
Merged

Properly handle toolchains defining openmp toolchain opt as dict (relevant for NVHPC toolchain)#5093
lexming merged 13 commits intoeasybuilders:developfrom
Thyre:toolchain-openmp-disable-flag

Conversation

@Thyre
Copy link
Copy Markdown
Collaborator

@Thyre Thyre commented Jan 15, 2026

In 956f038, we've added a dict for NVHPC to not only enable OpenMP when 'openmp': True is passed, but also disable OpenMP when either 'openmp': False or no value is passed.

This is due to NVHPC enabling OpenMP by default, causing issues with e.g. programs using the OpenMP Tools Interface as well.

Unfortunately, the handling of arguments in _set_compiler_flags didn't consider OpenMP as an argument where toolchains can pass a dict. Because of that, enabling OpenMP with nvidia-compilers or NVHPC causes the following effect, described by @pavelToman:

If I have toolchainopts = {'openmp': True} in ELPA FLAGS there are False True:

eb-shell> echo $CFLAGS
-O2 -tp=host -Mflushz False True

When I have toolchainopts = {'openmp': False} then it seems ok with:

eb-shell> echo $CFLAGS
-O2 -tp=host -Mflushz

but I think there is missing -nomp flag. When I delete the 'openmp': {False: '-nomp', True: '-mp'} from the COMPILER_UNIQUE_OPTION_MAP and run ELPA with toolchainopts = {'openmp': True}  I've got:

eb-shell> echo $CFLAGS
-O2 -tp=host -Mflushz -fopenmp

To fix this, copy the handling of vectorize to also be used for openmp. This requires removing openmp from COMPILER_OPTIONS to avoid the default handling earlier on. Since many toolchains simply specify a string or list of strings, keep backwards compatibility by converting them to a dict, passing an empty string for 'openmp': False (which is the default).

Also harden both openmp and vectorize against bogus passed values to avoid EasyBuild crashing with a KeyError.

@Thyre Thyre added this to the next release (5.2.1?) milestone Jan 15, 2026
@Thyre Thyre added the bug fix label Jan 15, 2026
@Thyre
Copy link
Copy Markdown
Collaborator Author

Thyre commented Jan 15, 2026 

FAIL: test_independence (test.framework.toolchain.ToolchainTest.test_independence)
Test independency of toolchain instances.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/runner/41bf98bba00dbba1b05ea742347ce4813c2c7ddf/lib/python3.11/site-packages/test/framework/toolchain.py", line 2338, in test_independence
    self.assertEqual(str(tc.variables['CFLAGS']), expected_cflags, msg)
  File "/tmp/runner/41bf98bba00dbba1b05ea742347ce4813c2c7ddf/lib/python3.11/site-packages/easybuild/base/testing.py", line 89, in assertEqual
    super().assertEqual(a, b, msg=msg)
AssertionError: '-O2 -ftree-vectorize -fopenmp -test -fno-math-errno' != '-O2 -ftree-vectorize -test -fno-math-errno -fopenmp'
- -O2 -ftree-vectorize -fopenmp -test -fno-math-errno
?                      ---------
+ -O2 -ftree-vectorize -test -fno-math-errno -fopenmp
?                                           +++++++++
 : Expected $CFLAGS found for toolchain GCC: -O2 -ftree-vectorize -test -fno-math-errno -fopenmp

That test seems to be brittle...
I'll take a look. Maybe worth making this test independent of the order of arguments.

@Thyre
Move OpenMP flag in test_independence test
d7f5662 
Test assumes a fixed order of flags, which might be relevant for certain
optimizations, but OpenMP can appear at any place. Since the changes to
framework causes the flag to appear earlier, update the test to reflect
this as well.

Signed-off-by: Jan André Reuter <[email protected]>
@Thyre Thyre force-pushed the toolchain-openmp-disable-flag branch from b4f01ad to d7f5662 Compare January 15, 2026 10:40
@Thyre Thyre requested review from boegel and lexming January 15, 2026 10:50
This was referenced Jan 22, 2026
Closed
Merged
@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 27, 2026

@lexming Can you take a look at this?

boegel
boegel reviewed Jan 28, 2026
View reviewed changes
Comment thread easybuild/tools/toolchain/compiler.py
boegel
boegel previously requested changes Jan 28, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

I would definitely like to see this fix covered by the tests, i.e. we should verify that openmp toolchain options has the correct effect for a toolchain based on nvidia-compilers...

@Thyre
Copy link
Copy Markdown
Collaborator Author

Thyre commented Jan 28, 2026

I would definitely like to see this fix covered by the tests, i.e. we should verify that openmp toolchain options has the correct effect for a toolchain based on nvidia-compilers...

Agreed. I'll need to figure out how to extend the existing dummy EasyConfigs by nvidia-compilers.

@Thyre
Copy link
Copy Markdown
Collaborator Author

Thyre commented Jan 28, 2026

@boegel, tried to add a test for OpenMP & vectorize with nvidia-compilers. Lets see if this also passes in the CI...

@Thyre Thyre requested a review from boegel January 29, 2026 08:14
Thyre
Thyre commented Jan 29, 2026
View reviewed changes
Comment thread test/framework/easyconfigs/test_ecs/n/nvidia-compilers/nvidia-compilers-25.9.eb Outdated
lexming
lexming reviewed Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

thanks for the fix! LGTM

I just a have a minor comment on the tests

Comment thread test/framework/toolchain.py Outdated
Comment thread test/framework/toolchain.py Outdated
Comment thread test/framework/toolchain.py Outdated
@Thyre Thyre mentioned this pull request Feb 4, 2026
Open
@lexming
Copy link
Copy Markdown
Contributor

lexming commented Feb 4, 2026

Merging, thanks @Thyre!

@lexming lexming enabled auto-merge February 4, 2026 15:38
@lexming lexming disabled auto-merge February 4, 2026 15:38
@lexming lexming dismissed boegel’s stale review February 4, 2026 15:38

Review addressed by author

@lexming lexming merged commit 358da6a into easybuilders:develop Feb 4, 2026
40 checks passed
@rsdmse rsdmse mentioned this pull request Feb 6, 2026
Closed
@boegel boegel changed the title Properly handle toolchains defining openmp toolchain opt as dict Properly handle toolchains defining openmp toolchain opt as dict (relevant for NVHPC toolchain) Feb 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lexming lexming lexming approved these changes

@boegel boegel Awaiting requested review from boegel

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

5.2.1

Development

Successfully merging this pull request may close these issues.

3 participants

@Thyre @boegel @lexming