-
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
use mpiexec to run import test in PythonPackage easyblock if usempi is enabled #2001
Conversation
Successfully tested with:
|
Don't we already have a variable for this that should be used? @boegel ?? |
@akesandgren There's |
At least that is configurable though with |
exts_filter = (orig_exts_filter[0].replace('python', self.python_cmd), orig_exts_filter[1]) | ||
exts_filter = (exts_filter[0].replace('python', self.python_cmd), exts_filter[1]) | ||
|
||
if 'exts_filter' not in kwargs: | ||
kwargs.update({'exts_filter': exts_filter}) |
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.
indent is off by one level here
@@ -687,16 +687,18 @@ def sanity_check_step(self, *args, **kwargs): | |||
} | |||
|
|||
# make sure 'exts_filter' is defined, which is used for sanity check | |||
if self.multi_python: | |||
# when installing for multiple Python versions, we must use 'python', not a full-path 'python' command! |
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.
please retain this comment below, so we're not left wondering why replacing 'python'
with the full path is not done when using multi-Python
exts_filter = EXTS_FILTER_PYTHON_PACKAGES | ||
|
||
if self.toolchain.options.get('usempi', None): | ||
# packages using MPI have to execute the sanity checks with mpirun |
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.
not mpirun, mpiexec
Please open an issue in framework with some motivation, maybe we need to replace |
@boegel I though self.toolchain.mpi_cmd_for was configurable from the outside too? |
Sure I can do that but what I meant was that if we used True that it is not a very obvious though, name is a a bit off. |
I got feedback from Intel on the original issue that triggered this change (easybuilders/easybuild-easyconfigs#10213). They acknowledge the issue and it should be fixed at some point. Therefore, this PR is not the way to go. It should not be necessary to use In conclusion, I'm closing this PR. |
Python packages using MPI should run the sanity check command
python -c "import module_name"
withmpirun
to avoid failed initializations of MPI.Fixes: easybuilders/easybuild-easyconfigs#10213