Skip to content

change deprecated NVHPC toolchain classes to inherit from supported counterparts#5096

Merged
boegel merged 10 commits intoeasybuilders:developfrom
lexming:fix-deprecation-nvhpc
Jan 20, 2026
Merged

change deprecated NVHPC toolchain classes to inherit from supported counterparts#5096
boegel merged 10 commits intoeasybuilders:developfrom
lexming:fix-deprecation-nvhpc

Conversation

@lexming
Copy link
Contributor

@lexming lexming commented Jan 20, 2026

fixes #5095

We only care about downwards compatibility where deprecated modules can be loaded and provide their newer counterparts. This PR simplifies the definition of NVHPC deprecated toolchain classes by making them a child class of the new ones.

This PR also adds a DEPRECATED attributed to deprecated toolchain classes, to be able to easily filter them out of --list-toolchains.

@lexming lexming added this to the next release (5.2.1?) milestone Jan 20, 2026
@lexming
Copy link
Contributor Author

lexming commented Jan 20, 2026

@Thyre
Copy link
Collaborator

Thyre commented Jan 20, 2026

Trying the test cases in #5095, both still fail:

$ eb --from-pr=23755 --robot /opt/EasyBuild/apps/ebfiles_repo --experimental --try-toolchain=nvidia-compilers,25.9-CUDA-12.9.1 --include-easyblocks-from-pr=3906 --try-update-deps
== Temporary log file in case of crash /tmp/eb-hemyy1p_/easybuild-o33js9rw.log
== easyblock boost.py included from PR #3906
ERROR: Failed to process easyconfig /tmp/eb-hemyy1p_/files_pr23755/b/Boost/Boost-1.88.0-nvidia-compilers-25.3.eb: No toolchain element 'NVHPC' found for toolchain {'name': 'nvidia-compilers', 'version': '25.3', 'toolchain': {'name': 'system', 'version': ''}, 'versionsuffix': '', 'parsed': True, 'hidden': False, 'full_mod_name': 'Core/nvidia-compilers/25.3', 'short_mod_name': 'nvidia-compilers/25.3', 'system': True}: nvidia-compilers EasyConfig @ /opt/EasyBuild/prog/local/easybuild/easyconfigs/n/nvidia-compilers/nvidia-compilers-25.3.eb
$ eb --from-pr=23755 --include-easyblocks-from-pr=3906
[...]
== FAILED: Installation ended unsuccessfully: List of toolchain dependency modules and toolchain definition do not match (found ['GCCcore/14.2.0', 'binutils/2.42-GCCcore-14.2.0', 'numactl/2.0.19-GCCcore-14.2.0'] vs expected {'NVHPC'}) (took 31 secs)
== Results of the build can be found in the log file(s) /tmp/eb-ukj94tri/easybuild-Boost-1.88.0-20260120.153758.GJrwj.log
== Summary:
   * [FAILED]  Boost/1.88.0-nvidia-compilers-25.3

@lexming
Copy link
Contributor Author

lexming commented Jan 20, 2026

@Thyre missed to commit that change, fixed in 3831601
Try again please

@Thyre
Copy link
Collaborator

Thyre commented Jan 20, 2026

This looks better 😄

[reuter1@jsczen3c2 ~]$ eb --from-pr=23755 --include-easyblocks-from-pr=3906
[...]
== preparing...
  >> loading toolchain module: nvidia-compilers/25.3
  >> (no build dependencies specified)
  >> loading modules for (runtime) dependencies:
  >>  * bzip2/1.0.8-GCCcore-14.2.0
  >>  * zlib/1.3.1-GCCcore-14.2.0
  >>  * XZ/5.6.3-GCCcore-14.2.0
  >>  * zstd/1.5.6-GCCcore-14.2.0
  >>  * ICU/76.1-GCCcore-14.2.0
  >> defining build environment for nvidia-compilers/25.3 toolchain
== ... (took 26 secs)
== configuring...
  >> running shell command:
        ./bootstrap.sh --with-toolset=pgi --prefix=/project/def-maintainers/reuter1/rocky9/zen3/software/Boost/1.88.0-nvidia-compilers-25.3 --without-libraries=python,mpi
$ eb --from-pr=23755 --robot /opt/EasyBuild/apps/ebfiles_repo --experimental --try-toolchain=nvidia-compilers,25.9-CUDA-12.9.1 --include-easyblocks-from-pr=3906 --try-update-deps
[...]
== preparing...
  >> loading toolchain module: nvidia-compilers/25.9-CUDA-12.9.1
  >> (no build dependencies specified)
  >> loading modules for (runtime) dependencies:
  >>  * bzip2/1.0.8
  >>  * zlib/1.3.1
  >>  * XZ/5.8.1
  >>  * zstd/1.5.7
  >>  * ICU/77.1
  >> defining build environment for nvidia-compilers/25.9-CUDA-12.9.1 toolchain
== ... (took 1 secs)
== configuring...
  >> running shell command:
        ./bootstrap.sh --with-toolset=pgi --prefix=/opt/EasyBuild/apps/software/Boost/1.88.0-nvidia-compilers-25.9-CUDA-12.9.1 --without-libraries=python,mpi
        [started at: 2026-01-20 16:44:39]
        [working dir: /opt/EasyBuild/apps/build/Boost/1.88.0/nvidia-compilers-25.9-CUDA-12.9.1/boost_1_88_0]
        [output and state saved to /tmp/eb-_azewr54/run-shell-cmd-output/bootstrapsh-gyiwqsza]

Copy link
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.

lgtm

@boegel boegel merged commit 2a57715 into easybuilders:develop Jan 20, 2026
40 checks passed
@lexming lexming deleted the fix-deprecation-nvhpc branch January 21, 2026 08:04
# load NVidiaCompilers, deprecated NVHPC corresponds to it
tc = NvidiaCompilers(name='NvidiaCompilers', version='2024a') # Common usage
# Might be checked by pre-5.2.0 users
self.assertIsInstance(tc, NVHPC)
Copy link
Contributor

@Flamefire Flamefire Jan 21, 2026

Choose a reason for hiding this comment

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

Why did you remove this? This was one of the breaking changes: Prior users relied on checking if isinstance(tc, NVHPC) which was broken by removing NVHPC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it does not matter. We care that the deprecated toolchain is still there, can be loaded and provides the new one. The opposite is irrelevant, nobody should load the new toolchain and expect to get the deprecated one.

Copy link
Contributor

Choose a reason for hiding this comment

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

nobody should load the new toolchain and expect to get the deprecated one.

But people use the old toolchain already by importing the old one not the new one. This change here (and in 5.2.0) breaks existing usage, e.g. in hooks:

from easybuild.toolchains.compilers import NVHPC

def some_hook(ec):
  tc = ec.get_toolchain()
  if isinstance(tc, NVHPC):
    ec['cuda_compute_capabilities'] = '8.0'

Because we changed the name to be Nvidia-compilers this does not work anymore and needs to be adapted to if isinstance(tc, NVHPC, NvidiaCompilers): to work with 5.1 and 5.2

Hence my ABC-Meta trick to allow the existing code to work even with the 5.2.0 change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not break anything. That check is catching the old toolchain, and continues to be able to catch the old toolchain.

You are right that this code will not catch the new toolchain, and that is correct IMO. If you want to catch the new stuff, you need to update your hooks and be actively aware that your hooks are catching the new stuff. The old and new toolchains are very much different, so the code behind that check is not guaranteed to work with the new stuff at all in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

The old and new toolchains are very much different, so the code behind that check is not guaranteed to work with the new stuff at all in any case.

Is this true? Because #4927 has commit f1761f8

rename NVHPC compiler toolchain to nvidia-compilers

So I understood this as a rename of compiler.NVHPC to compiler.NvidiaCompilers with no (real) differences.

Note that I'm not referring to toolchains.NVHPC but to the previously existing "compiler"

Copy link
Contributor Author

@lexming lexming Jan 21, 2026

Choose a reason for hiding this comment

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

Yes, that is true, we started with just a rename. But that does not mean that we will keep backward compatibility with old compiler.NVHPC at all levels. And things have already diverged, not in structure (as you mention compiler.NvidiaCompilers holds the same spot as former compiler.NVHPC) but in functionality.

The new easyblocks are rather different, attributes and methods have changed. So we cannot guarantee that a hook for the old NVHPC will continue to work with the new one at any level. Therefore, it is risky to have hooks running inadvertently with the new code. Even if that isinstance check would work, code underneath can potentially fail without an update.

This does not mean that we don't care about backward compatibility at all. We already made sure that existing easyconfigs for the old NVHPC continue to work in the same way with the new nvidia-compilers/NVHPC. And we also enabled loading the old NVHPC so that it can still be used by custom easyblocks or custom easyconfigs.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we cannot guarantee that a hook for the old NVHPC will continue to work with the new one at any level.

Then I'd rather not have compiler.NVHPC. With the current change the code above seems to work but is never triggered. That is even worse than before. Better always wrong, than sometimes silently wrong.

What I was aiming for was a "best effort".
I don't know what the differences between the 2 toolchain classes (easyblocks?) are now. My goal was that working with instances of either works the same as much as possible, both when the user had a custom derived easyblock and when the user got an instance of (now) the new one.

@Flamefire
Copy link
Contributor

We only care about downwards compatibility where deprecated modules can be loaded and provide their newer counterparts.

Do we? We have hooks checking if a toolchain is a CUDA toolchain by using isinstance.
That's why I took extra care that they behave as-if they are literally the same, i.e. "as-if NVHPC = NVidiaCompilers"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nvidia-compilers toolchain regression in develop since release of v5.2.0

4 participants