[MRG] Fix MPIBackend platform logic #876
Conversation
…able assignment logic in _get_mpi_env
hnn_core/parallel_backends.py
Outdated
| # For Linux systems | ||
| if sys.platform not in ['win32', 'darwin']: |
There was a problem hiding this comment.
I'm not sure if the original intent of this line was to just target Linux systems, but that was what it was doing using the 'win' not in logic. This has the same functionality but is more explicit of the platforms it avoids.
The old logic would have also excluded the 'cygwin' platform, a posix-translator layer for windows... but I doubt anyone will be trying to use hnn-core on that.
There was a problem hiding this comment.
You'll have to look at Blake's old commits to understand why this flag was added. Likely a Windows-specific thing not related to Mac.
There was a problem hiding this comment.
I'm guessing this is intended only for unix systems which support open-mpi (vs. ms-mpi), in which case we should only be avoiding windows here.
There was a problem hiding this comment.
Also, setting the OMPI_MCA_btl_base_warn_component_unused environment variable to 0 is only used for suppressing warning messages. Regardless of Blake's original intention here, I suggest that we either implement this consistently for all references to an open-mpi build (i.e., across Mac and Linux systems) or not at all.
There was a problem hiding this comment.
To be clear, I'm suggesting we remove 'darwin' from the list above or remove L87-89 altogether. Aside from that and @jasmainak's comments, this looks good to go by me.
|
what's the motivation for this fix? Does it break something for you? If it ain't broken, don't fix it ... we don't have the means to test it properly |
I found the incorrect logic on lines 655 while working on a refactor of the MPIBackend class wrt discussion in #871. I figured it'd be better to give it a separate PR instead of bundled with the refactor. The logic is incorrect but you're correct, it's not breaking. I looked into why it's not breaking for Linux and discussed that in #875. That said, as a developer I had to spend time trying to understand bad logic while working on codebase. Clarity and readability is important for a maintainable codebase. |
Co-authored-by: Mainak Jas <[email protected]>
gtdang
force-pushed
the
fix-mpibackend-platform-logic
branch
from
e266a9b to
898dc58
Compare
|
Thanks @gtdang! |
Updated the logic for MPIBackend and get_mpi_env function to not use the obtuse 'win' string search.
closes #875