Skip to content

Commit

Permalink
Honor recent MPI command simplifications in upstream Khiops binary pa…
Browse files Browse the repository at this point in the history
…ckages

Thusly, functional parity is kept with the `khiops-env` script which is
part of the native Khiops binary packages.

Also honor the `KHIOPS_MPI_VERBOSE` environment variable which controls
the verbosity of the MPI command.

closes #192
  • Loading branch information
popescu-v committed Jun 24, 2024
1 parent 2ed2864 commit d9e542a
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 7 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/pip.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ jobs:
- name: Run tests
env:
KHIOPS_SAMPLES_DIR: ${{ github.workspace }}/khiops-samples
# Do not add `--quiet` to OpenMPI
KHIOPS_MPI_VERBOSE: true
# This is needed so that OpenMPI's mpiexec can be run as root
OMPI_ALLOW_RUN_AS_ROOT: 1
OMPI_ALLOW_RUN_AS_ROOT_CONFIRM: 1
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ jobs:
KHIOPS_DOCKER_RUNNER_URL: https://localhost:11000
KHIOPS_DOCKER_RUNNER_SHARED_DIR: /tmp/sandbox
KHIOPS_RUNNER_SERVICE_PATH: /scripts/run_service.sh
# Do not add `--quiet` to OpenMPI
KHIOPS_MPI_VERBOSE: true
# This is needed so that OpenMPI's mpiexec can be run as root
OMPI_ALLOW_RUN_AS_ROOT: 1
OMPI_ALLOW_RUN_AS_ROOT_CONFIRM: 1
Expand Down
4 changes: 4 additions & 0 deletions doc/notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ environment variables listed below. They can be split into three groups:
- ``KHIOPS_MPIEXEC_PATH``: path to the ``mpiexec`` command
- ``KHIOPS_MPI_LIB``: *Linux and MacOS only* path to the MPI library; added to
the beginning of ``LD_LIBRARY_PATH``
- ``KHIOPS_MPI_VERBOSE``: flag which controls the verbosity of the MPI command; "false" by
default; if different from "true", then add the `--quiet` argument to the MPI command when
the MPI implementation is OpenMPI; for other MPI implementations, this flag currently has no
effect



Expand Down
21 changes: 14 additions & 7 deletions khiops/core/internals/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,10 @@ class KhiopsLocalRunner(KhiopsRunner):
- ``KHIOPS_MPIEXEC_PATH``: path to the ``mpiexec`` command
- ``KHIOPS_MPI_LIB``: *Linux and MacOS only* path to the MPI library; added to
the beginning of ``LD_LIBRARY_PATH``
- ``KHIOPS_MPI_VERBOSE``: flag which controls the verbosity of the MPI command;
"false" by default; if different from "true", then add the `--quiet` argument
to the MPI command when the MPI implementation is OpenMPI; for other MPI
implementations, this flag currently has no effect
.. rubric:: Samples directory settings
Expand Down Expand Up @@ -1155,13 +1159,13 @@ def _initialize_mpi_command_args(self):
break
if mpiexec_path is not None:
self._set_mpi_command_args_with_mpiexec(
mpiexec_path
mpiexec_path, installation_method
)
break

# If MPI is found, then set the path to mpiexec accordingly
if mpiexec_path is not None:
self._set_mpi_command_args_with_mpiexec(mpiexec_path)
self._set_mpi_command_args_with_mpiexec(mpiexec_path, installation_method)
# If MPI is still not found, then do not use MPI and warn the user
else:
self.mpi_command_args = []
Expand All @@ -1171,7 +1175,7 @@ def _initialize_mpi_command_args(self):
"Go to https://khiops.org for more information."
)

def _set_mpi_command_args_with_mpiexec(self, mpiexec_path):
def _set_mpi_command_args_with_mpiexec(self, mpiexec_path, installation_method):
assert mpiexec_path is not None
# User-specified MPI command args take precendence over automatic setting
if "KHIOPS_MPI_COMMAND_ARGS" in os.environ:
Expand Down Expand Up @@ -1199,11 +1203,14 @@ def _set_mpi_command_args_with_mpiexec(self, mpiexec_path):
"1",
]
elif platform.system() == "Linux":
# For Linux native installations we use OpenMPI
# Also honor `KHIOPS_MPI_VERBOSE`
if (
installation_method == "binary+pip"
and os.environ.get("KHIOPS_MPI_VERBOSE") != "true"
):
self.mpi_command_args.append("--quiet")
self.mpi_command_args += [
"-bind-to",
"hwthread",
"-map-by",
"core",
"-n",
str(self.max_cores),
]
Expand Down
42 changes: 42 additions & 0 deletions tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -2661,6 +2661,48 @@ def test_khiops_environment_variables_basic(self):
else:
os.environ[fixture["variable"]] = old_value

def test_mpi_command_verbosity_control(self):
"""Test MPI command verbosity control via KHIOPS_MPI_VERBOSE"""
# Get previous MPI command verbosity status
khiops_mpi_verbose = os.environ.get("KHIOPS_MPI_VERBOSE")

# Set MPI command to non-verbose
os.environ["KHIOPS_MPI_VERBOSE"] = "false"

# Create a fresh runner and initialize its env
with MockedRunnerContext(create_mocked_raw_run(False, False, 0)) as runner:
pass

# Look-up installation method from the runner's status message
install_method = None
for status_line in runner._build_status_message()[0].splitlines():
if status_line.startswith("install type"):
install_method = status_line.split(":")[1].strip()
break
if install_method == "binary+pip":
# By default, "--quiet" is an OpenMPI command argument
try:
self.assertIn("--quiet", runner.mpi_command_args)
# Restore original MPI command verbosity
finally:
if khiops_mpi_verbose is not None:
os.environ["KHIOPS_MPI_VERBOSE"] = khiops_mpi_verbose

# Set MPI command to verbose
os.environ["KHIOPS_MPI_VERBOSE"] = "true"

# Create a fresh runner and initialize its env
with MockedRunnerContext(create_mocked_raw_run(False, False, 0)) as runner:
pass

# Now, "--quiet" is not an MPI command argument anymore
try:
self.assertNotIn("--quiet", runner.mpi_command_args)
# Restore original MPI command verbosity
finally:
if khiops_mpi_verbose is not None:
os.environ["KHIOPS_MPI_VERBOSE"] = khiops_mpi_verbose

def test_mpi_command_is_updated_on_max_cores_update(self):
"""Test MPI command is updated on max_cores update"""
# Create a fresh runner and initialize its env
Expand Down

0 comments on commit d9e542a

Please sign in to comment.