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

rewrite GROMACS easyblock to build all four variations in the same build #1991

Conversation

akesandgren
Copy link
Contributor

@akesandgren akesandgren commented Mar 14, 2020

single precision without MPI, single precision with MPI,
double precision without MPI, double precision with MPI.

Change the default for double_precision from False to None
And a bunch of other changes.

This supersedes #1987

…ild.

single precision without MPI, single precision with MPI,
double precision without MPI, double precision with MPI.

Change the default for double_precision from False to None
And a bunch of other changes.
Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

General:

  • Needs rebase on master, especially as CMakeMake supports build_type and build_shared_libs
  • Always check for the longest possible string when checking for a config option. So -DFoo=bar not DFoo=bar just to avoid false positives, the extra dash doesn't hurt but makes it safer, even though only slightly
  • Check for uses of self.cfg['foo'] where foo is from extra_options. @boegel insisted that self.cfg.get(foo, default) is used so deriving classes not calling the base extra_options don't make this throw a KeyError. However as this EB is unlikely to be derived from, I'd say this is OK especially here (I personally prefer the KeyError in general). Check with @boegel
  • Replace self.dynamic by self.cfg['build_shared_libs'], see note
  • Handle CUDA vs double in 1 place, e.g. build_all_steps. BTW: Is this a limitation of GROMACS? CUDA generally can do double just fine. Maybe this doesn't apply anymore?

I'm not to confident on the multi-step builds, so comments are mostly general and CMake specific.

easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
@akesandgren
Copy link
Contributor Author

Attacking the comments from @Flamefire piece by piece...

@akesandgren
Copy link
Contributor Author

Reg "Replace self.dynamic by self.cfg['build_shared_libs']" perhaps later. The dynamic flag comes from the cray toolchain and self.dynamic been hijacked a bit and I need to be able to enforce it to be on for later versions.

I can look at that later.

@akesandgren
Copy link
Contributor Author

Reg "Handle CUDA vs double in 1 place, e.g. build_all_steps", haven't look at what i can do in that.

Can look again later, leaving it alone right now.

easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Outdated Show resolved Hide resolved
easybuild/easyblocks/g/gromacs.py Show resolved Hide resolved
@boegel boegel modified the milestones: 4.x, next release (4.2.1?) May 18, 2020
@boegel
Copy link
Member

boegel commented May 18, 2020

Retested extensively with a whole range of easyconfigs, old and new, works like a charm.

Thanks a lot @akesandgren!

@boegel boegel merged commit 2dd0be1 into easybuilders:develop May 18, 2020
@akesandgren akesandgren deleted the make_gromacs_build_all_variants_in_one_go branch May 19, 2020 09:57
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.

5 participants