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

update CP2K easyblock for compatibility with CP2K v2023.1 release (GCC only) + fix GPU support #2918

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

osbama
Copy link

@osbama osbama commented Apr 14, 2023

CP2K has had a recent release v2023.1. This release has significant improvements on the GPU side.

The current cp2k easyblock does not produce an executable using foss chain for this version. I have contacted CP2K developers, and Alfio Lazzaro fixed the arch file generated by cp2k see the thread here

I have implemented the new gpuarch and tried to replicate the arch file fixed by Alfio Lazzarro as close as possible. I have created a new extra_option called v2023 to contain the changes. The reason I did not do this by checking the version is that, the GPU compilation for the previous release also does not work with the current easyblock, whereas this arch file seems to work.

I have tested this on a Epyc/A100 based system, and the binary seems fine.

Please notice that I am an absolute newbie to easybuild. Please check the modifications carefully.

I will also submit a pull request for the easyconfig file I have used.

Thanks,
Baris

@boegel
Copy link
Member

boegel commented Apr 15, 2023

@osbama Thanks a lot for your contribution!

Before we take a detailed look at the proposed changes, can you look into fixing the code style problems that the Hound bot is reporting?

Some high-level info is available at https://docs.easybuild.io/code-style.

Do let us know if you have any questions (either in here, or via the EasyBuild Slack).

@Micket
Copy link
Contributor

Micket commented Apr 20, 2023

I find the need to specify the gpuver stuff painful. I think we'd very much like to use out cuda-compute-capabilities that we already specify.

I had a look through the CP2K code to see where is actually needs to specify 1 single type, and why the heck it needs to specify the product name instead of just the compute capabilities.

I think most of the CP2K code would just work for any (multiple even) cuda compute capabilities, if only they got rid of their unnecessary stuff where they check for the product names (ugh, painful).
One could probably circumvent that rather easily, perhaps with just CMake flags or a very simple patch to remove all the stuff they added to their cmakelists, so that one can just specify the flags directly.

Digging deeper, i did find that there was one place where was truly unfortunately limited to just building with only 1 compute capability;
https://github.com/cp2k/cp2k/blob/04dd581a73886059b5d5fb12035b1998b2b62f5a/exts/build_dbcsr/Makefile#L78
since it actually hardcodes some tuning parameters for the GPU model.

Ugh. OK, i guess due to this i suppose this is one of those softwares that are specific to one CCC.

A few options we could have;

  1. Use this GPUVER and specify versionsuffixes for each easyconfig to differentiate them. I have clusters with T4's and V100, as well as A40 and A100, so I guess there would be very different performance across these. Advantage: Could build the best performance for each GPU type. Disadvantage: More to build i guess, and of course, having to selection at compile time.
  2. Map the lowest CCC to a GPUVER. Possible just as the default, if nothing else is chosen. Upside is to have an easyconfig that will, at least, build everywhere for EB users who have all probably specified their favourite CCCs. Downside is that despite that CCC might not have to much negative impact, the fact that it also impacts parameters for matrix multiplication probably sucks.

@boegel boegel changed the title CP2K v2023.1 release compatibility (gcc only) (fixes GPU). update CP2K easyblock for compatibility wiht CP2K v2023.1 release (GCC only) + fix GPU support May 10, 2023
@boegel boegel modified the milestones: 4.x, release after 4.7.2 May 10, 2023
@boegel boegel modified the milestones: 4.7.3, release after 4.7.3 Jul 5, 2023
@boegel boegel changed the title update CP2K easyblock for compatibility wiht CP2K v2023.1 release (GCC only) + fix GPU support update CP2K easyblock for compatibility with CP2K v2023.1 release (GCC only) + fix GPU support Aug 14, 2023
@boegel
Copy link
Member

boegel commented Aug 14, 2023

@osbama Thanks a lot for the contribution!

I think this will need some work before we can merge it though, not only because of @Micket's feedback, but also because of the v2023 custom easyconfig parameter you added.
You briefly mentioned why you did that instead of doing a version check in the easyblock, but I don't fully understand the motivation here... Can you elaborate?

@Micket W.r.t. gpuver: how about we auto-detect which GPU is being used, and automatically determine the value to pass to GPUVER? I guess that should be possible based on the output of nvidia-smi, and perhaps that should be a function provided by the EasyBuild framework even (but we can start in the CP2K easyblock).
Mapping specified CUDA compute capability to the GPU type name is another option, but significantly messier imho, see the approach taken in the LAMMPS easyblock...

@osbama
Copy link
Author

osbama commented Aug 15, 2023

Hi @boegel,
No worries, happy to be of assistance.

Please check out the linked discussion in CP2K. The method CP2K developer suggested is quite different than the currently implemented one.

CP2K is a modular architecture that can get quite challenging to compile, furthermore sometimes older versions do some things better than newer versions, and to make things even more complicated, some modules may refuse to get compiled in the aformentioned method, so I decided not to radically alter the build procedure, but add a new one based on the suggestion of CP2K developer.

If one insists, or skips some capability like GPU for the sake of some other packages/optimizations, I am sure the previous method can be made to compile the newer version, although I didn't try it myself.

Maybe the name can be changed to "alternate_compilation_mode" or something similar?

BTW, gpuver makes the life of people with heterogenous systems and restrictive compilation conditions very easy, I will be sad if it is removed.

@boegel
Copy link
Member

boegel commented Aug 15, 2023

@osbama The problem is not so much that you're switching to a new installation method, that should be fine as long as we opt-in to it consistently starting with a particular CP2K version (and keep using the old method for older CP2K versions). Unless there's a reason that doesn't make sense which I'm overlooking here?

The main problem with gpuver as it's used now is that you're hardcoding a particular value (A100) in your easyconfig PR (easybuilders/easybuild-easyconfigs#17718), which doesn't work well in a shared easyconfigs repository.

By default, when gpuver is not specified, the easyblock should (ideally) do some auto-detect mechanism.
We can keep gpuver as is so you can hard specify the GPU you want to build for (and skip the auto-detection), but the easyconfig file in the central repository should use gpuver = None (which implies relying on auto-detection).

@osbama
Copy link
Author

osbama commented Aug 15, 2023

@boegel
Let's not call it "new" and "old". Let's call it "alternative 1" and "alternative 2"
"alternative 1" did not work with the new GPU module. I can not guarantee "alternative 2" will work with all the modules. Both alternatives can be tailored to compile "2023" version. My LAMMPS experience suggests it will be a good idea to keep both for different user needs/scenarios.

gpuver is hardcoded because: My compile environment is strictly cross compile for GPUs in a heterogeneous GPU environment. In my experience CUDA checks only if there is a NVIDIA card in the system (copyright?), does not complain if the card is not capable of that calculation mode. If autodetection is forced, it will compile for an irrelevant GPU. it will really make admins of heterogenous systems like mine very upset. Even if compute capability to CP2K naming scheme shall be implemented, in my opinion it should not be forced. I think it is better to set it to a minimum compute accepted, newer cards will run that code, and those who want to tune the compilation for performance, will edit the eb file anyhow.

My humble opinion: It is nice to have everything automated, but automation works acceptably only if everything adheres to a particular and strict standard. Standards vary wildly especially in the developing world, where the rule "something is better than nothing" dominates. If automation will make the life of some admins harder, it is much more efficient to make it strictly optional.

@@ -103,14 +105,16 @@ def extra_options():
'plumed': [None, "Enable PLUMED support", CUSTOM],
'type': ['popt', "Type of build ('popt' or 'psmp')", CUSTOM],
'typeopt': [True, "Enable optimization", CUSTOM],
'v2023': [False, "2023 version of CP2K", CUSTOM],
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be a config option, use LooseVersion(get_software_version('CP2K')) to figure out if it is 2023 or newer instead.

'FPIC': self.fpic,
'DFLAGS': ' -D__parallel -D__BLACS -D__SCALAPACK -D__FFTSG %s' % self.cfg['extradflags'],
'INCS': '',
'CFLAGS': '-O3 -fopenmp -ftree-vectorize -march=native -fno-math-errno -fopenmp -std=c11 $(FPIC) $(DEBUG) '
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the way to do it, do not hardcode compiler flags like this.
-march=native only works for GCC for instance.
The old code was correct apart from the missing INCS and DFLAGS.

'FREE': '-ffree-form -ffree-line-length-none -std=f2008',
})
options[
'FCFLAGSOPT'] = '-O3 -ftree-vectorize -march=native -fno-math-errno -fopenmp -fPIC $(FREE) $(DFLAGS) ' \
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, don't hardcode compiler options

Comment on lines +821 to +828
'f90': os.getenv('F90'),
'base': os.path.dirname(os.path.normpath(self.cfg['start_dir'])),
'cp2k_version': self.cfg['type'],
'triplet': self.typearch,
'cp2k_dir': os.path.basename(os.path.normpath(self.cfg['start_dir'])),
'maxtasks': self.cfg['maxtasks'],
'mpicmd_prefix': self.toolchain.mpi_cmd_for('', test_core_cnt),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unintended shift-right

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

Successfully merging this pull request may close these issues.

None yet

4 participants