-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add automatic CUDA support for ELPA #2673
base: develop
Are you sure you want to change the base?
Changes from all commits
0c8ae77
f97d6d5
b97b1c7
015bda4
a9b2daa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
|
||
@author: Micael Oliveira (MPSD-Hamburg) | ||
@author: Kenneth Hoste (Ghent University) | ||
@author: Damian Alvarez (Forschungszentrum Juelich GmbH) | ||
""" | ||
import os | ||
|
||
|
@@ -52,6 +53,7 @@ def extra_options(): | |
"""Custom easyconfig parameters for ELPA.""" | ||
extra_vars = { | ||
'auto_detect_cpu_features': [True, "Auto-detect available CPU features, and configure accordingly", CUSTOM], | ||
'cuda': [None, "Enable CUDA build if CUDA is among the dependencies", CUSTOM], | ||
'with_shared': [True, "Enable building of shared ELPA libraries", CUSTOM], | ||
'with_single': [True, "Enable building of single precision ELPA functions", CUSTOM], | ||
'with_generic_kernel': [True, "Enable building of ELPA generic kernels", CUSTOM], | ||
|
@@ -159,6 +161,24 @@ def run_all_steps(self, *args, **kwargs): | |
self.cfg.update('configopts', '--with-mpi=no') | ||
self.cfg.update('configopts', 'LIBS="$LIBLAPACK"') | ||
|
||
# Add CUDA features | ||
cuda_is_dep = 'CUDA' in [i['name'] for i in self.cfg.dependencies()] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this, including the "cuda" extra_var, can be simplified to That way you can drop the elif statements and the cuda extra_var. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's actually not fully true... If the So this is actually a better approach... We should have proper support for this in framework though, so you can just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say that if you do that (load CUDA manually before building) then you're doing it wrong and almost all our easyconfigs are at risk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For JSC, CUDA is a dep of MPI by default (and that MPI works regardless of whether or not there actually is a GPU through the use of UCX variables). So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ocaisa if CUDA is a dep of MPI regardless of whether or not there actually is a GPU, don’t you want to trigger a GPU build also regardless of whether or not there actually is a GPU? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smoors No, because then that GPU-enabled build will (typically) not work on a system without a GPU (whereas OpenMPI will work just fine on that system as long as you set the appropriate btl). So at JSC, the GPU supported build is usually explicit |
||
if cuda_is_dep and (self.cfg['cuda'] is None or self.cfg['cuda']): | ||
self.cfg.update('configopts', '--enable-nvidia-gpu') | ||
cuda_cc_space_sep = self.cfg.template_values['cuda_cc_space_sep'].replace('.', '').split() | ||
# Just one is supported, so pick the highest one (but prioritize sm_80) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Picking the highest one is likely a bad idea since then the code won't run on lower end GPU's in case there sites default settings lists all their models. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or pick first listed (or minimum rather than max) and print a clear warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The LAMMPS easyblock selects the largest: https://github.com/easybuilders/easybuild-easyblocks/blob/develop/easybuild/easyblocks/l/lammps.py#L307 Having a method, with a clear rational on what is picked, would be good for cases where the software supports building for one, and not all of the, CUDA CC. |
||
selected_cc = "0" | ||
for cc in cuda_cc_space_sep: | ||
if int(cc) > int(selected_cc) and int(selected_cc) != 80: | ||
selected_cc = cc | ||
self.cfg.update('configopts', '--with-NVIDIA-GPU-compute-capability=sm_%s' % selected_cc) | ||
if selected_cc == "80": | ||
self.cfg.update('configopts', '--enable-nvidia-sm80-gpu') | ||
elif not self.cfg['cuda']: | ||
self.log.warning("CUDA is disabled") | ||
elif not cuda_is_dep and self.cfg['cuda']: | ||
raise EasyBuildError("CUDA is not a dependency, but support for CUDA is enabled.") | ||
|
||
# make all builds verbose | ||
self.cfg.update('buildopts', 'V=1') | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not set this to
True
by default? That would simplify the logic below a bit to:if cuda_is_dep and self.cfg['cuda']:
...
elif not self.cfg['cuda']:
...
else:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually make sense: don't make a decision by default, let it depend on whether
CUDA
is included as a dependency or not, but allow explicitely opting in (True
) or out (False
).@damianam is just using the same approach as in #2666 here...