-
Notifications
You must be signed in to change notification settings - Fork 305
Make cdft lib compilation optional for Intel MKL, by detecting MPI availability #1393
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
Conversation
This Python is identical to Python-3.6.4-intel-2018a.eb except for mpi4py, that is no MPI support. Depends on easybuilders/easybuild-easyblocks#1393
easybuild/easyblocks/i/imkl.py
Outdated
| if LooseVersion(self.version) >= LooseVersion('10.3'): | ||
| cdftlibs.append('fftw3x_cdft') | ||
| # cdftlibs set in prepare_step | ||
| cdftlibs = self.cdftlibs |
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.
@bartoldeman There's no point in doing this, should use self.cdftlibs everywhere and just stop using cdftlibs?
easybuild/easyblocks/i/imkl.py
Outdated
| cdftlibs = [] | ||
| self.log.debug("Determined MPI specification based on loaded MPI module: %s" % mpi_spec) | ||
| self.cdftlibs = cdftlibs | ||
| self.mpi_spec = mpi_spec |
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.
@bartoldeman Please initialize both of these in the constructor of the easyblock (__init__)
easybuild/easyblocks/i/imkl.py
Outdated
| } | ||
| mpi_fam = self.toolchain.mpi_family() | ||
| mpi_spec = mpi_spec_by_fam.get(mpi_fam) | ||
| self.log.debug("Determined MPI specification based on MPI toolchain component: %s" % mpi_spec) |
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.
@bartoldeman While touching this, please change this to:
self.log.debug("...: %s", mpi_spec)(, rather than %; same below)
| super(EB_imkl, self).prepare_step(*args, **kwargs) | ||
| else: | ||
| super(EB_imkl, self).prepare_step(*args, **kwargs) | ||
| if self.cfg['interfaces']: |
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.
@bartoldeman One more minor style issues: add an empty line above (and maybe also a comment explaining what's going on in the block below)
easybuild/easyblocks/i/imkl.py
Outdated
| buildopts = [compopt] | ||
| if lib in fftw3libs: | ||
| buildopts.append('install_to=$INSTALL_DIR') | ||
| elif lib in cdftlibs: |
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.
should be self.cdftlibs, same above
|
Changes applied and tested with imkl easyconfigs with and without MPI toolchains. |
|
Ping? |
| self.mpi_spec = 'mpich2' | ||
| elif get_software_root('OpenMPI'): | ||
| self.mpi_spec = 'openmpi' | ||
| elif not get_software_root('impi'): |
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.
@bartoldeman At this point, the modules for the dependencies are not loaded yet (that's done in prepare_step), so each of these get_software_root calls will return None...
We should initialize self.cdftlibs and self.mpi_spec in __init__, and move this whole block to prepare_step, after the super(EB_imkl, self).prepare_step call
As suggested by @boegel during review, because only after prepare_step get_software_root works.
This allows installing MKL using a compiler-only toolchain with interfaces enabled.