From dc8b9c5e3c7aca81531487e539ecd97b93a70c4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luc-Aur=C3=A9lien?= <133773992+lucaurelien@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:17:52 +0200 Subject: [PATCH 1/3] Enable System Parameters Globally #622 Update api.py --- .gitignore | 8 ++ doc/site/notes.md | 27 +++++ khiops/core/api.py | 171 +++++++++++++++++++++++++++++- khiops/core/internals/runner.py | 118 ++++++++++++++------- tests/test_core.py | 134 +++++++++++++++++++++++ tests/test_khiops_integrations.py | 93 ++++++++++++++++ 6 files changed, 513 insertions(+), 38 deletions(-) diff --git a/.gitignore b/.gitignore index 2b5d9c6a..5d56899c 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,13 @@ # Mac/OSX .DS_Store +# OpenSpec artifacts +openspec/ +.github/agents/openspec.agent.md +.github/prompts/opsx-*.prompt.md +.github/skills/openspec-*/ +.github/workflows/copilot-setup-steps.yml + # Source for the following rules: https://raw.githubusercontent.com/github/gitignore/master/Python.gitignore # Byte-compiled / optimized / DLL files __pycache__/ @@ -44,6 +51,7 @@ wheels/ tests/resources/*/output_reports tests/resources/*/output_kdic tests/resources/dictionary/copy_output_kdic +tests/resources/scenario_generation/system_settings/output tests/resources/scenario_generation/api/*/output tests/resources/scenario_generation/data_path_deprecation/*/output tests/resources/scenario_generation/general_options/output diff --git a/doc/site/notes.md b/doc/site/notes.md index 533009ae..fbb82f93 100644 --- a/doc/site/notes.md +++ b/doc/site/notes.md @@ -61,6 +61,33 @@ The functions in the [khiops.core.api][] have the following common parameters. : If `True` the internal scenario generated by Khiops will force characters such as accentuated ones to be decoded with the UTF8->ANSI khiops transformation. +#### Process-wide System-parameter Defaults { #core-api-global-system-params } + +The Core API provides process-wide defaults for subsequent local Khiops +executions. Each setter rebuilds the local runner with an isolated copy of the +current environment; it never changes the caller's complete `os.environ` +mapping. Existing per-call system parameters continue to apply to the generated +execution scenario. + +- [set_default_max_cores][khiops.core.api.set_default_max_cores] and + [get_default_max_cores][khiops.core.api.get_default_max_cores] configure and query + `KHIOPS_PROC_NUMBER`. +- [set_default_memory_limit_mb][khiops.core.api.set_default_memory_limit_mb] and + [get_default_memory_limit_mb][khiops.core.api.get_default_memory_limit_mb] + configure and query `KHIOPS_MEMORY_LIMIT`. +- [set_default_temp_dir][khiops.core.api.set_default_temp_dir] and + [get_default_temp_dir][khiops.core.api.get_default_temp_dir] configure and query + `KHIOPS_TMP_DIR`. + +The numeric setters accept `None` or a positive `int`. The temporary-directory +setter accepts `None` or a non-empty `str`; invalid types and values raise +`TypeError` or `ValueError`, and a rejected update leaves the previous value +unchanged. Calling a setter with `None` disables only that default and restores +the corresponding value from the environment captured when the Khiops package +initialized, or removes the variable if it was absent. Later changes to +`os.environ` do not change this reset target. Getters return typed numeric +values, a non-empty path, or `None` when the effective variable is absent. + ### Input Types { #core-api-input-types } The types accepted in most methods and classes of [khiops.core][] are flexible: diff --git a/khiops/core/api.py b/khiops/core/api.py index 3774c4c0..bafa9ce9 100644 --- a/khiops/core/api.py +++ b/khiops/core/api.py @@ -17,7 +17,9 @@ """ import io import os +import threading as _threading import warnings +from types import MappingProxyType as _MappingProxyType import khiops.core.internals.filesystems as fs from khiops.core.dictionary import DictionaryDomain @@ -29,9 +31,176 @@ is_string_like, type_error_message, ) -from khiops.core.internals.runner import get_runner +from khiops.core.internals.runner import KhiopsLocalRunner as _KhiopsLocalRunner +from khiops.core.internals.runner import get_runner, set_runner from khiops.core.internals.task import get_task_registry +# Capture the process environment before any runner initialization can occur. +_inherited_environment = _MappingProxyType(os.environ.copy()) +_current_environment = dict(_inherited_environment) +_default_system_parameters_lock = _threading.RLock() + + +def _validate_default_system_parameter(parameter_name, value, expected_type): + """Validate a global system-parameter value.""" + if value is None: + return + + if expected_type is int: + if isinstance(value, bool) or not isinstance(value, int): + raise TypeError(type_error_message(parameter_name, value, int)) + if value <= 0: + raise ValueError(f"{parameter_name} must be positive (it is {value})") + elif not isinstance(value, str): + raise TypeError(type_error_message(parameter_name, value, str)) + elif not value: + raise ValueError(f"{parameter_name} must be non-empty") + + +def _set_default_system_parameter( + parameter_name, environment_variable, value, expected_type +): + """Set one global system parameter and install its runner.""" + global _current_environment + + _validate_default_system_parameter(parameter_name, value, expected_type) + with _default_system_parameters_lock: + updated_environment = _current_environment.copy() + if value is None: + inherited_value = _inherited_environment.get(environment_variable) + if inherited_value is None: + updated_environment.pop(environment_variable, None) + else: + updated_environment[environment_variable] = inherited_value + else: + updated_environment[environment_variable] = str(value) + + runner = _KhiopsLocalRunner(env=updated_environment.copy()) + _current_environment = updated_environment + set_runner(runner) + + +def _get_default_system_parameter(environment_variable, expected_type): + """Get one global system parameter from the current environment.""" + with _default_system_parameters_lock: + value = _current_environment.get(environment_variable) + if value is None: + return None + if expected_type is int: + return int(value) + return value + + +def set_default_max_cores(max_cores=None): + """Set the process-wide default maximum number of Khiops cores. + + Parameters + ---------- + max_cores : int, optional + Positive maximum number of cores. If `None`, restore the value of + `KHIOPS_PROC_NUMBER` captured when the Khiops package initialized. + + Notes + ----- + The value is applied to subsequent local executions through an isolated + runner environment and does not modify `os.environ`. + + Raises + ------ + TypeError + If `max_cores` is not an `int` or `None`. + ValueError + If `max_cores` is not positive. + """ + _set_default_system_parameter("max_cores", "KHIOPS_PROC_NUMBER", max_cores, int) + + +def get_default_max_cores(): + """Return the process-wide default maximum number of Khiops cores. + + Returns + ------- + int or None + The effective positive value of `KHIOPS_PROC_NUMBER`, or `None` when + the variable is absent. + """ + return _get_default_system_parameter("KHIOPS_PROC_NUMBER", int) + + +def set_default_memory_limit_mb(memory_limit_mb=None): + """Set the process-wide default Khiops memory limit. + + Parameters + ---------- + memory_limit_mb : int, optional + Positive memory limit in megabytes. If `None`, restore the value of + `KHIOPS_MEMORY_LIMIT` captured when the Khiops package initialized. + + Notes + ----- + The value is applied to subsequent local executions through an isolated + runner environment and does not modify `os.environ`. + + Raises + ------ + TypeError + If `memory_limit_mb` is not an `int` or `None`. + ValueError + If `memory_limit_mb` is not positive. + """ + _set_default_system_parameter( + "memory_limit_mb", "KHIOPS_MEMORY_LIMIT", memory_limit_mb, int + ) + + +def get_default_memory_limit_mb(): + """Return the process-wide default Khiops memory limit. + + Returns + ------- + int or None + The effective positive value of `KHIOPS_MEMORY_LIMIT`, or `None` when + the variable is absent. + """ + return _get_default_system_parameter("KHIOPS_MEMORY_LIMIT", int) + + +def set_default_temp_dir(temp_dir=None): + """Set the process-wide default temporary directory for Khiops. + + Parameters + ---------- + temp_dir : str, optional + Non-empty temporary-directory path. If `None`, restore the value of + `KHIOPS_TMP_DIR` captured when the Khiops package initialized. + + Notes + ----- + The value is applied to subsequent local executions through an isolated + runner environment and does not modify `os.environ`. + + Raises + ------ + TypeError + If `temp_dir` is not a `str` or `None`. + ValueError + If `temp_dir` is empty. + """ + _set_default_system_parameter("temp_dir", "KHIOPS_TMP_DIR", temp_dir, str) + + +def get_default_temp_dir(): + """Return the process-wide default temporary directory for Khiops. + + Returns + ------- + str or None + The effective non-empty value of `KHIOPS_TMP_DIR`, or `None` when the + variable is absent. + """ + return _get_default_system_parameter("KHIOPS_TMP_DIR", str) + + # Construction rules DEFAULT_CONSTRUCTION_RULES = [ "GetValue", diff --git a/khiops/core/internals/runner.py b/khiops/core/internals/runner.py index e210a3c5..2c7f0c10 100644 --- a/khiops/core/internals/runner.py +++ b/khiops/core/internals/runner.py @@ -50,7 +50,7 @@ def _isdir_without_all_perms(dir_path): ) -def get_default_samples_dir(): +def get_default_samples_dir(environment=None): """Returns the default samples directory The default samples directory is computed according to the following priorities: @@ -60,15 +60,17 @@ def get_default_samples_dir(): - `%USERPROFILE%\\khiops_data\\samples` otherwise - Linux/macOS: `$HOME/khiops_data/samples` """ - if "KHIOPS_SAMPLES_DIR" in os.environ and os.environ["KHIOPS_SAMPLES_DIR"]: - samples_dir = os.environ["KHIOPS_SAMPLES_DIR"] - elif platform.system() == "Windows" and "PUBLIC" in os.environ: - samples_dir = os.path.join(os.environ["PUBLIC"], "khiops_data", "samples") + environment = os.environ if environment is None else environment + if "KHIOPS_SAMPLES_DIR" in environment and environment["KHIOPS_SAMPLES_DIR"]: + samples_dir = environment["KHIOPS_SAMPLES_DIR"] + elif platform.system() == "Windows" and "PUBLIC" in environment: + samples_dir = os.path.join(environment["PUBLIC"], "khiops_data", "samples") else: # The filesystem abstract layer is used here # as the path can be either local or remote + home_dir = environment.get("HOME", environment.get("KHIOPS_MPI_HOME", "")) samples_dir = fs.get_child_path( - fs.get_child_path(os.environ["HOME"], "khiops_data"), "samples" + fs.get_child_path(home_dir, "khiops_data"), "samples" ) return samples_dir @@ -237,7 +239,7 @@ def _check_conda_env_bin_dir(conda_env_bin_dir): return is_conda_env_bin_dir -def _infer_khiops_installation_method(trace=False): +def _infer_khiops_installation_method(trace=False, environment=None): """Returns the Khiops installation method Definitions : @@ -255,6 +257,8 @@ def _infer_khiops_installation_method(trace=False): - or in a classical virtual environment (highly encouraged) """ + environment = os.environ if environment is None else environment + # We are in a Conda environment if # - the CONDA_PREFIX environment variable exists and, # - the khiops_env script exists within: @@ -263,8 +267,8 @@ def _infer_khiops_installation_method(trace=False): # Note: The check that the Khiops binaries are actually executable is done # afterwards by the initializations method. installation_method = "unknown" - if "CONDA_PREFIX" in os.environ: - conda_env_dir = os.environ["CONDA_PREFIX"] + if "CONDA_PREFIX" in environment: + conda_env_dir = environment["CONDA_PREFIX"] if platform.system() == "Windows": conda_binary_dir = os.path.join(conda_env_dir, "Library", "bin") else: @@ -335,13 +339,16 @@ def _get_current_library_installer(): return "unknown" -def _build_khiops_process_environment(): +def _build_khiops_process_environment(environment=None): """Build a specific environment used for the execution of khiops in a process This environment can be modified freely without interfering with the global one. """ - khiops_env = os.environ.copy() + if environment is None: + khiops_env = os.environ.copy() + else: + khiops_env = environment.copy() # Ensure HOME is always set for OpenMPI 5+ # (using KHIOPS_MPI_HOME if it exists) @@ -946,8 +953,17 @@ class KhiopsLocalRunner(KhiopsRunner): """ - def __init__(self): + def __init__(self, env=None): + """Initialize a local runner. + + Parameters + ---------- + env : dict, optional + Environment owned by this runner. If omitted, initialization keeps + using the process environment for compatibility. + """ # Define specific attributes + self._environment = env.copy() if env is not None else None self._mpi_command_args = None self._khiops_path = None self._khiops_coclustering_path = None @@ -962,7 +978,12 @@ def __init__(self): self._initialize_khiops_environment() def _initialize_khiops_environment(self): - installation_method = _infer_khiops_installation_method() + runner_environment = ( + os.environ if self._environment is None else self._environment + ) + installation_method = _infer_khiops_installation_method( + environment=runner_environment + ) match installation_method: # In conda-based environments, khiops_env is not in PATH; # its location must be inferred from the conda env directory. @@ -974,7 +995,9 @@ def _initialize_khiops_environment(self): khiops_env_path += ".cmd" # In an activated conda environment, khiops_env is in PATH. case "conda": - khiops_env_path = self._infer_khiops_env_from_path(installation_method) + khiops_env_path = self._infer_khiops_env_from_path( + installation_method, runner_environment + ) case "pip": # Ensure the binary dependency is still installed. try: @@ -1011,18 +1034,23 @@ def _initialize_khiops_environment(self): # which is in PATH. else: khiops_env_path = self._infer_khiops_env_from_path( - installation_method + installation_method, runner_environment ) case _: raise KhiopsEnvironmentError( f"Unknown installation method '{installation_method}'." ) + khiops_env_process_options = { + "stdout": subprocess.PIPE, + "stderr": subprocess.PIPE, + "universal_newlines": True, + } + if self._environment is not None: + khiops_env_process_options["env"] = runner_environment.copy() + with subprocess.Popen( - [khiops_env_path, "--env"], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True, + [khiops_env_path, "--env"], **khiops_env_process_options ) as khiops_env_process: stdout, stderr = khiops_env_process.communicate() if khiops_env_process.returncode != 0: @@ -1054,32 +1082,37 @@ def _initialize_khiops_environment(self): # to prepend to or to set to HOME for OpenMPI 5+ # when running khiops core if var_name == "KHIOPS_MPI_HOME": - os.environ["KHIOPS_MPI_HOME"] = var_value + runner_environment["KHIOPS_MPI_HOME"] = var_value # Set paths to Khiops binaries elif var_name == "KHIOPS_PATH": self.khiops_path = var_value - os.environ["KHIOPS_PATH"] = var_value + runner_environment["KHIOPS_PATH"] = var_value elif var_name == "KHIOPS_COCLUSTERING_PATH": self.khiops_coclustering_path = var_value - os.environ["KHIOPS_COCLUSTERING_PATH"] = var_value + runner_environment["KHIOPS_COCLUSTERING_PATH"] = var_value # Set MPI command elif var_name == "KHIOPS_MPI_COMMAND": self._mpi_command_args = shlex.split(var_value) - os.environ["KHIOPS_MPI_COMMAND"] = var_value + runner_environment["KHIOPS_MPI_COMMAND"] = var_value # On Windows, in 'pip' installations, # "KHIOPS_MPI_DLL_PATH" (containing the Intel MPI Library) # must be added to "PATH" otherwise Khiops wouldn't find it # and fail immediately elif installation_method == "pip" and var_name == "KHIOPS_MPI_DLL_PATH": - os.environ["PATH"] = os.pathsep.join( - [var_value, os.environ.get("PATH")] + current_path = runner_environment.get("PATH") + runner_environment["PATH"] = ( + os.pathsep.join([var_value, current_path]) + if current_path + else var_value ) + if self._environment is not None: + runner_environment[var_name] = var_value # Propagate all the other environment variables to Khiops binaries else: - os.environ[var_name] = var_value + runner_environment[var_name] = var_value - # Set KHIOPS_API_MODE to `true` - os.environ["KHIOPS_API_MODE"] = "true" + # Set KHIOPS_API_MODE to `true` + runner_environment["KHIOPS_API_MODE"] = "true" # Check the tools exist and are executable self._check_tools() @@ -1087,8 +1120,9 @@ def _initialize_khiops_environment(self): # Initialize the default samples dir self._initialize_default_samples_dir() - def _infer_khiops_env_from_path(self, installation_method): - khiops_env_path = shutil.which("khiops_env") + def _infer_khiops_env_from_path(self, installation_method, environment=None): + search_path = None if environment is None else environment.get("PATH", "") + khiops_env_path = shutil.which("khiops_env", path=search_path) if khiops_env_path is None: raise KhiopsEnvironmentError( "The 'khiops_env' script not found for the current " @@ -1100,7 +1134,7 @@ def _infer_khiops_env_from_path(self, installation_method): def _initialize_default_samples_dir(self): """See class docstring""" - samples_dir = get_default_samples_dir() + samples_dir = get_default_samples_dir(self._environment) _check_samples_dir(samples_dir) self._samples_dir = samples_dir assert self._samples_dir is not None @@ -1162,7 +1196,12 @@ def _detect_library_installation_incompatibilities(self, library_root_dir_path): error_list = [] warning_list = [] - installation_method = _infer_khiops_installation_method() + runner_environment = ( + os.environ if self._environment is None else self._environment + ) + installation_method = _infer_khiops_installation_method( + environment=runner_environment + ) # activated 'conda' installation if installation_method == "conda": @@ -1182,14 +1221,14 @@ def _detect_library_installation_incompatibilities(self, library_root_dir_path): ) warning_list.append(warning) - conda_prefix_path = Path(os.environ["CONDA_PREFIX"]) + conda_prefix_path = Path(runner_environment["CONDA_PREFIX"]) # the conda environment must match the library installation if not library_root_dir_path.is_relative_to(conda_prefix_path): error = ( "Khiops Python library installation " f"path '{library_root_dir_path}' " "does not match the current Conda environment " - f"'{os.environ['CONDA_PREFIX']}'. " + f"'{runner_environment['CONDA_PREFIX']}'. " "Either deactivate the current Conda environment " "or use the Khiops Python library " "belonging to the current Conda environment. " @@ -1203,7 +1242,7 @@ def _detect_library_installation_incompatibilities(self, library_root_dir_path): error = ( f"Khiops binary path '{self.khiops_path}' " "does not match the current Conda environment " - f"'{os.environ['CONDA_PREFIX']}'. " + f"'{runner_environment['CONDA_PREFIX']}'. " "We recommend installing the Khiops binary " "in the current Conda environment. " "Go to https://khiops.org for instructions.\n" @@ -1335,7 +1374,12 @@ def _build_status_message(self): ) # Build the messages for install type and mpi - install_type_msg = _infer_khiops_installation_method() + runner_environment = ( + os.environ if self._environment is None else self._environment + ) + install_type_msg = _infer_khiops_installation_method( + environment=runner_environment + ) if self._mpi_command_args: mpi_command_args_msg = " ".join(self._mpi_command_args) else: @@ -1523,7 +1567,7 @@ def raw_run(self, tool_name, command_line_args=None, use_mpi=True, trace=False): # Build custom Khiops process environment # which makes sure HOME is defined and set # according to khiops_env's KHIOPS_MPI_HOME - khiops_env = _build_khiops_process_environment() + khiops_env = _build_khiops_process_environment(self._environment) # Execute the process with subprocess.Popen( diff --git a/tests/test_core.py b/tests/test_core.py index facdc667..9b8b67c9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -19,10 +19,12 @@ from copy import copy from difflib import unified_diff from pathlib import Path +from types import MappingProxyType from unittest import mock import khiops import khiops.core as kh +import khiops.core.api as core_api import khiops.core.internals.filesystems as fs from khiops.core import KhiopsRuntimeError from khiops.core.api import _deprecate_legacy_data_path @@ -1020,6 +1022,138 @@ def mocked_raw_run(*_, **__): return mocked_raw_run +class KhiopsGlobalSystemParameterTests(unittest.TestCase): + """Test process-wide Core API system-parameter defaults""" + + def setUp(self): + self._initial_inherited_environment = core_api._inherited_environment + self._initial_current_environment = core_api._current_environment + + launch_environment = os.environ.copy() + launch_environment.update( + { + "KHIOPS_PROC_NUMBER": "2", + "KHIOPS_MEMORY_LIMIT": "128", + "KHIOPS_TMP_DIR": "/launch/tmp", + } + ) + core_api._inherited_environment = MappingProxyType(launch_environment) + core_api._current_environment = launch_environment.copy() + + self._runner_constructor_patch = mock.patch.object( + core_api, "_KhiopsLocalRunner" + ) + self.runner_constructor = self._runner_constructor_patch.start() + self._set_runner_patch = mock.patch.object(core_api, "set_runner") + self.set_runner = self._set_runner_patch.start() + + def tearDown(self): + self._set_runner_patch.stop() + self._runner_constructor_patch.stop() + core_api._inherited_environment = self._initial_inherited_environment + core_api._current_environment = self._initial_current_environment + + def test_accessors_are_public_and_restore_launch_values(self): + """Test accessors expose typed defaults and inherited reset values""" + accessor_names = [ + "set_default_max_cores", + "get_default_max_cores", + "set_default_memory_limit_mb", + "get_default_memory_limit_mb", + "set_default_temp_dir", + "get_default_temp_dir", + ] + for accessor_name in accessor_names: + with self.subTest(accessor_name=accessor_name): + self.assertTrue(hasattr(kh, accessor_name)) + + self.assertEqual(kh.get_default_max_cores(), 2) + self.assertEqual(kh.get_default_memory_limit_mb(), 128) + self.assertEqual(kh.get_default_temp_dir(), "/launch/tmp") + + kh.set_default_max_cores(8) + kh.set_default_memory_limit_mb(512) + kh.set_default_temp_dir("/configured/tmp") + self.assertEqual(kh.get_default_max_cores(), 8) + self.assertEqual(kh.get_default_memory_limit_mb(), 512) + self.assertEqual(kh.get_default_temp_dir(), "/configured/tmp") + + kh.set_default_max_cores() + self.assertEqual(kh.get_default_max_cores(), 2) + self.assertEqual(kh.get_default_memory_limit_mb(), 512) + self.assertEqual(kh.get_default_temp_dir(), "/configured/tmp") + + def test_defaults_use_isolated_complete_runner_environment(self): + """Test accepted updates preserve the process environment""" + initial_process_environment = os.environ.copy() + + kh.set_default_max_cores(8) + + self.assertEqual(os.environ, initial_process_environment) + self.assertEqual(self.runner_constructor.call_count, 1) + runner_environment = self.runner_constructor.call_args.kwargs["env"] + self.assertEqual(runner_environment["KHIOPS_PROC_NUMBER"], "8") + self.assertEqual(runner_environment["KHIOPS_MEMORY_LIMIT"], "128") + self.assertEqual(runner_environment["KHIOPS_TMP_DIR"], "/launch/tmp") + self.assertEqual(set(runner_environment), set(core_api._current_environment)) + self.assertIsNot(runner_environment, core_api._current_environment) + + def test_later_environment_changes_do_not_change_reset_targets(self): + """Test reset values remain tied to the launch snapshot""" + with mock.patch.dict(os.environ, {"KHIOPS_PROC_NUMBER": "99"}): + kh.set_default_max_cores(8) + kh.set_default_max_cores(None) + + self.assertEqual(kh.get_default_max_cores(), 2) + + def test_absent_inherited_values_are_removed_on_reset(self): + """Test resetting an absent inherited value removes the variable""" + launch_environment = dict(core_api._inherited_environment) + for environment_variable in ( + "KHIOPS_PROC_NUMBER", + "KHIOPS_MEMORY_LIMIT", + "KHIOPS_TMP_DIR", + ): + launch_environment.pop(environment_variable) + core_api._inherited_environment = MappingProxyType(launch_environment) + core_api._current_environment = launch_environment.copy() + + kh.set_default_max_cores(8) + kh.set_default_memory_limit_mb(512) + kh.set_default_temp_dir("/configured/tmp") + kh.set_default_max_cores() + kh.set_default_memory_limit_mb() + kh.set_default_temp_dir() + + self.assertIsNone(kh.get_default_max_cores()) + self.assertIsNone(kh.get_default_memory_limit_mb()) + self.assertIsNone(kh.get_default_temp_dir()) + runner_environment = self.runner_constructor.call_args.kwargs["env"] + self.assertNotIn("KHIOPS_PROC_NUMBER", runner_environment) + self.assertNotIn("KHIOPS_MEMORY_LIMIT", runner_environment) + self.assertNotIn("KHIOPS_TMP_DIR", runner_environment) + + def test_invalid_updates_leave_values_unchanged(self): + """Test invalid values are rejected without changing effective defaults""" + invalid_updates = [ + (kh.set_default_max_cores, 0, kh.get_default_max_cores), + (kh.set_default_max_cores, "8", kh.get_default_max_cores), + (kh.set_default_memory_limit_mb, 0, kh.get_default_memory_limit_mb), + (kh.set_default_memory_limit_mb, "512", kh.get_default_memory_limit_mb), + (kh.set_default_temp_dir, "", kh.get_default_temp_dir), + (kh.set_default_temp_dir, 42, kh.get_default_temp_dir), + ] + expected_values = [2, 2, 128, 128, "/launch/tmp", "/launch/tmp"] + + for (setter, invalid_value, getter), expected_value in zip( + invalid_updates, expected_values + ): + with self.subTest(invalid_value=invalid_value): + with self.assertRaises((TypeError, ValueError)): + setter(invalid_value) + self.assertEqual(getter(), expected_value) + + class KhiopsCoreServicesTests(unittest.TestCase): """Test the services of the core module classes diff --git a/tests/test_khiops_integrations.py b/tests/test_khiops_integrations.py index ee3132db..37fabe99 100644 --- a/tests/test_khiops_integrations.py +++ b/tests/test_khiops_integrations.py @@ -17,6 +17,7 @@ import khiops.core as kh import khiops.core.internals.filesystems as fs +import khiops.core.internals.runner as runner_module from khiops import tools from khiops.core.exceptions import KhiopsEnvironmentError from khiops.core.internals.runner import ( @@ -346,6 +347,98 @@ def test_runner_environment_initialization(self): self.assertEqual(env_khiops_api_mode, "true") + def test_isolated_runner_environment_is_not_written_to_process(self): + """Test explicit runner environments reach both subprocess boundaries""" + initial_process_environment = os.environ.copy() + isolated_environment = initial_process_environment.copy() + isolated_environment.update( + { + "KHIOPS_PROC_NUMBER": "7", + "KHIOPS_MEMORY_LIMIT": "256", + "KHIOPS_TMP_DIR": "/isolated/tmp", + "HOME": "/isolated/home", + } + ) + tool_path = os.environ.get("SHELL", "/bin/sh") + + mock_popen = MagicMock() + mock_popen.return_value.__enter__.return_value.communicate.return_value = ( + "KHIOPS_MPI_HOME /isolated/mpi\n" + f"KHIOPS_PATH {tool_path}\n" + f"KHIOPS_COCLUSTERING_PATH {tool_path}\n" + "KHIOPS_MPI_COMMAND mpiexec\n" + "KHIOPS_API_MODE false\n" + "KHIOPS_RETURNED yes\n", + "", + ) + mock_popen.return_value.__enter__.return_value.returncode = 0 + + with patch.object( + runner_module, "_infer_khiops_installation_method", return_value="conda" + ): + with patch.object( + KhiopsLocalRunner, + "_infer_khiops_env_from_path", + return_value="/bin/echo", + ): + with patch.object(KhiopsLocalRunner, "_check_tools"): + with patch.object( + KhiopsLocalRunner, "_initialize_default_samples_dir" + ): + with patch.object( + runner_module.subprocess, "Popen", mock_popen + ): + runner = KhiopsLocalRunner(env=isolated_environment) + runner.raw_run("khiops", [], use_mpi=False) + + self.assertEqual(os.environ, initial_process_environment) + self.assertEqual(runner._environment["KHIOPS_RETURNED"], "yes") + self.assertEqual(runner._environment["KHIOPS_API_MODE"], "true") + self.assertEqual( + mock_popen.call_args_list[0].kwargs["env"], isolated_environment + ) + final_environment = mock_popen.call_args_list[1].kwargs["env"] + self.assertEqual(final_environment["KHIOPS_PROC_NUMBER"], "7") + self.assertEqual(final_environment["KHIOPS_MEMORY_LIMIT"], "256") + self.assertEqual(final_environment["KHIOPS_TMP_DIR"], "/isolated/tmp") + self.assertEqual(final_environment["KHIOPS_RETURNED"], "yes") + self.assertEqual(final_environment["HOME"], "/isolated/home") + + def test_default_runner_does_not_export_mpi_dll_path(self): + """Test the default runner keeps MPI DLL details out of os.environ""" + mock_popen = MagicMock() + mock_popen.return_value.__enter__.return_value.communicate.return_value = ( + "KHIOPS_PATH /bin/echo\n" + "KHIOPS_COCLUSTERING_PATH /bin/echo\n" + "KHIOPS_MPI_DLL_PATH /isolated/mpi\n", + "", + ) + mock_popen.return_value.__enter__.return_value.returncode = 0 + + with patch.dict(os.environ, {}, clear=True): + with patch.object( + runner_module, "_infer_khiops_installation_method", return_value="pip" + ): + with patch.object( + KhiopsLocalRunner, + "_infer_khiops_env_from_path", + return_value="/bin/echo", + ): + with patch.object(KhiopsLocalRunner, "_check_tools"): + with patch.object( + KhiopsLocalRunner, "_initialize_default_samples_dir" + ): + with patch.object( + runner_module.subprocess, "Popen", mock_popen + ): + runner = KhiopsLocalRunner() + runner.raw_run("khiops", [], use_mpi=False) + + self.assertNotIn("KHIOPS_MPI_DLL_PATH", os.environ) + final_environment = mock_popen.call_args_list[1].kwargs["env"] + self.assertEqual(final_environment["PATH"], "/isolated/mpi") + self.assertNotIn("KHIOPS_MPI_DLL_PATH", final_environment) + def test_khiops_and_khiops_coclustering_are_run_with_mpi(self): """Test that MODL and MODL_Coclustering are run with MPI""" From 33592a33aca6c13384774f736112061f54c93743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luc-Aur=C3=A9lien?= <133773992+lucaurelien@users.noreply.github.com> Date: Wed, 23 Sep 2026 17:05:03 +0200 Subject: [PATCH 2/3] fix: address review comments for global system parameters lint --- doc/site/notes.md | 12 ++-- khiops/core/api.py | 76 +++++++++++------------ khiops/core/internals/runner.py | 61 +++++++++--------- tests/test_core.py | 100 +++++++++++++++++++++--------- tests/test_khiops_integrations.py | 37 +---------- 5 files changed, 145 insertions(+), 141 deletions(-) diff --git a/doc/site/notes.md b/doc/site/notes.md index fbb82f93..159c65f2 100644 --- a/doc/site/notes.md +++ b/doc/site/notes.md @@ -71,18 +71,18 @@ execution scenario. - [set_default_max_cores][khiops.core.api.set_default_max_cores] and [get_default_max_cores][khiops.core.api.get_default_max_cores] configure and query - `KHIOPS_PROC_NUMBER`. + the maximum number of CPU cores. - [set_default_memory_limit_mb][khiops.core.api.set_default_memory_limit_mb] and [get_default_memory_limit_mb][khiops.core.api.get_default_memory_limit_mb] - configure and query `KHIOPS_MEMORY_LIMIT`. + configure and query the maximum amount of memory in megabytes. - [set_default_temp_dir][khiops.core.api.set_default_temp_dir] and [get_default_temp_dir][khiops.core.api.get_default_temp_dir] configure and query - `KHIOPS_TMP_DIR`. + the Khiops temporary directory. -The numeric setters accept `None` or a positive `int`. The temporary-directory -setter accepts `None` or a non-empty `str`; invalid types and values raise +The numeric setters accept an optional positive `int`. The temporary-directory +setter accepts an optional non-empty `str`; invalid types and values raise `TypeError` or `ValueError`, and a rejected update leaves the previous value -unchanged. Calling a setter with `None` disables only that default and restores +unchanged. Calling a setter with no arguments disables only that default and restores the corresponding value from the environment captured when the Khiops package initialized, or removes the variable if it was absent. Later changes to `os.environ` do not change this reset target. Getters return typed numeric diff --git a/khiops/core/api.py b/khiops/core/api.py index bafa9ce9..2c51feda 100644 --- a/khiops/core/api.py +++ b/khiops/core/api.py @@ -17,9 +17,9 @@ """ import io import os -import threading as _threading +import threading import warnings -from types import MappingProxyType as _MappingProxyType +from types import MappingProxyType import khiops.core.internals.filesystems as fs from khiops.core.dictionary import DictionaryDomain @@ -31,14 +31,15 @@ is_string_like, type_error_message, ) -from khiops.core.internals.runner import KhiopsLocalRunner as _KhiopsLocalRunner -from khiops.core.internals.runner import get_runner, set_runner +from khiops.core.internals.runner import KhiopsLocalRunner, get_runner, set_runner from khiops.core.internals.task import get_task_registry # Capture the process environment before any runner initialization can occur. -_inherited_environment = _MappingProxyType(os.environ.copy()) -_current_environment = dict(_inherited_environment) -_default_system_parameters_lock = _threading.RLock() +# Avoid race condition on os.environ (via MappingProxyType) and make sure updates +# to it are not propagated to the current environment (via .copy()). +_INHERITED_ENVIRONMENT = MappingProxyType(os.environ.copy()) +_CURRENT_ENVIRONMENT = dict(_INHERITED_ENVIRONMENT) +_DEFAULT_SYSTEM_PARAMETERS_LOCK = threading.RLock() def _validate_default_system_parameter(parameter_name, value, expected_type): @@ -46,44 +47,43 @@ def _validate_default_system_parameter(parameter_name, value, expected_type): if value is None: return - if expected_type is int: - if isinstance(value, bool) or not isinstance(value, int): - raise TypeError(type_error_message(parameter_name, value, int)) - if value <= 0: - raise ValueError(f"{parameter_name} must be positive (it is {value})") - elif not isinstance(value, str): - raise TypeError(type_error_message(parameter_name, value, str)) - elif not value: + if not isinstance(value, expected_type) or ( + expected_type is int and isinstance(value, bool) + ): + raise TypeError(type_error_message(parameter_name, value, expected_type)) + if not value: raise ValueError(f"{parameter_name} must be non-empty") + if isinstance(value, int) and value < 0: + raise ValueError(f"{parameter_name} must be positive; it is {value}") def _set_default_system_parameter( parameter_name, environment_variable, value, expected_type ): """Set one global system parameter and install its runner.""" - global _current_environment + global _CURRENT_ENVIRONMENT _validate_default_system_parameter(parameter_name, value, expected_type) - with _default_system_parameters_lock: - updated_environment = _current_environment.copy() + with _DEFAULT_SYSTEM_PARAMETERS_LOCK: + updated_environment = _CURRENT_ENVIRONMENT.copy() if value is None: - inherited_value = _inherited_environment.get(environment_variable) - if inherited_value is None: - updated_environment.pop(environment_variable, None) - else: + inherited_value = _INHERITED_ENVIRONMENT.get(environment_variable) + if inherited_value is None and environment_variable in updated_environment: + del updated_environment[environment_variable] + elif inherited_value is not None: updated_environment[environment_variable] = inherited_value else: updated_environment[environment_variable] = str(value) - runner = _KhiopsLocalRunner(env=updated_environment.copy()) - _current_environment = updated_environment + runner = KhiopsLocalRunner(environment=updated_environment.copy()) + _CURRENT_ENVIRONMENT = updated_environment set_runner(runner) def _get_default_system_parameter(environment_variable, expected_type): """Get one global system parameter from the current environment.""" - with _default_system_parameters_lock: - value = _current_environment.get(environment_variable) + with _DEFAULT_SYSTEM_PARAMETERS_LOCK: + value = _CURRENT_ENVIRONMENT.get(environment_variable) if value is None: return None if expected_type is int: @@ -97,8 +97,8 @@ def set_default_max_cores(max_cores=None): Parameters ---------- max_cores : int, optional - Positive maximum number of cores. If `None`, restore the value of - `KHIOPS_PROC_NUMBER` captured when the Khiops package initialized. + Positive maximum number of cores. If not specified, restore the value + captured when the Khiops package initialized. Notes ----- @@ -121,8 +121,8 @@ def get_default_max_cores(): Returns ------- int or None - The effective positive value of `KHIOPS_PROC_NUMBER`, or `None` when - the variable is absent. + The effective positive maximum number of cores, or `None` when no + default is configured. """ return _get_default_system_parameter("KHIOPS_PROC_NUMBER", int) @@ -133,8 +133,8 @@ def set_default_memory_limit_mb(memory_limit_mb=None): Parameters ---------- memory_limit_mb : int, optional - Positive memory limit in megabytes. If `None`, restore the value of - `KHIOPS_MEMORY_LIMIT` captured when the Khiops package initialized. + Positive memory limit in megabytes. If not specified, restore the value + captured when the Khiops package initialized. Notes ----- @@ -159,8 +159,8 @@ def get_default_memory_limit_mb(): Returns ------- int or None - The effective positive value of `KHIOPS_MEMORY_LIMIT`, or `None` when - the variable is absent. + The effective positive memory limit in megabytes, or `None` when no + default is configured. """ return _get_default_system_parameter("KHIOPS_MEMORY_LIMIT", int) @@ -171,8 +171,8 @@ def set_default_temp_dir(temp_dir=None): Parameters ---------- temp_dir : str, optional - Non-empty temporary-directory path. If `None`, restore the value of - `KHIOPS_TMP_DIR` captured when the Khiops package initialized. + Non-empty temporary-directory path. If not specified, restore the value + captured when the Khiops package initialized. Notes ----- @@ -195,8 +195,8 @@ def get_default_temp_dir(): Returns ------- str or None - The effective non-empty value of `KHIOPS_TMP_DIR`, or `None` when the - variable is absent. + The effective non-empty temporary-directory path, or `None` when no + default is configured. """ return _get_default_system_parameter("KHIOPS_TMP_DIR", str) diff --git a/khiops/core/internals/runner.py b/khiops/core/internals/runner.py index 2c7f0c10..1e80807d 100644 --- a/khiops/core/internals/runner.py +++ b/khiops/core/internals/runner.py @@ -50,6 +50,13 @@ def _isdir_without_all_perms(dir_path): ) +def _current_environment(environment=None): + """Returns the provided environment or the process environment.""" + if environment is not None: + return environment + return os.environ + + def get_default_samples_dir(environment=None): """Returns the default samples directory @@ -60,7 +67,7 @@ def get_default_samples_dir(environment=None): - `%USERPROFILE%\\khiops_data\\samples` otherwise - Linux/macOS: `$HOME/khiops_data/samples` """ - environment = os.environ if environment is None else environment + environment = _current_environment(environment) if "KHIOPS_SAMPLES_DIR" in environment and environment["KHIOPS_SAMPLES_DIR"]: samples_dir = environment["KHIOPS_SAMPLES_DIR"] elif platform.system() == "Windows" and "PUBLIC" in environment: @@ -68,10 +75,12 @@ def get_default_samples_dir(environment=None): else: # The filesystem abstract layer is used here # as the path can be either local or remote - home_dir = environment.get("HOME", environment.get("KHIOPS_MPI_HOME", "")) - samples_dir = fs.get_child_path( - fs.get_child_path(home_dir, "khiops_data"), "samples" - ) + if "HOME" in environment: + samples_dir = fs.get_child_path( + fs.get_child_path(environment["HOME"], "khiops_data"), "samples" + ) + else: + raise KeyError("HOME") return samples_dir @@ -239,7 +248,7 @@ def _check_conda_env_bin_dir(conda_env_bin_dir): return is_conda_env_bin_dir -def _infer_khiops_installation_method(trace=False, environment=None): +def _infer_khiops_installation_method(environment=None, trace=False): """Returns the Khiops installation method Definitions : @@ -257,7 +266,7 @@ def _infer_khiops_installation_method(trace=False, environment=None): - or in a classical virtual environment (highly encouraged) """ - environment = os.environ if environment is None else environment + environment = _current_environment(environment) # We are in a Conda environment if # - the CONDA_PREFIX environment variable exists and, @@ -345,10 +354,10 @@ def _build_khiops_process_environment(environment=None): This environment can be modified freely without interfering with the global one. """ - if environment is None: - khiops_env = os.environ.copy() - else: + if environment is not None: khiops_env = environment.copy() + else: + khiops_env = os.environ.copy() # Ensure HOME is always set for OpenMPI 5+ # (using KHIOPS_MPI_HOME if it exists) @@ -953,17 +962,17 @@ class KhiopsLocalRunner(KhiopsRunner): """ - def __init__(self, env=None): + def __init__(self, environment=None): """Initialize a local runner. Parameters ---------- - env : dict, optional + environment : dict, optional Environment owned by this runner. If omitted, initialization keeps using the process environment for compatibility. """ # Define specific attributes - self._environment = env.copy() if env is not None else None + self._environment = environment.copy() if environment is not None else None self._mpi_command_args = None self._khiops_path = None self._khiops_coclustering_path = None @@ -978,9 +987,7 @@ def __init__(self, env=None): self._initialize_khiops_environment() def _initialize_khiops_environment(self): - runner_environment = ( - os.environ if self._environment is None else self._environment - ) + runner_environment = _current_environment(self._environment) installation_method = _infer_khiops_installation_method( environment=runner_environment ) @@ -1099,14 +1106,10 @@ def _initialize_khiops_environment(self): # must be added to "PATH" otherwise Khiops wouldn't find it # and fail immediately elif installation_method == "pip" and var_name == "KHIOPS_MPI_DLL_PATH": - current_path = runner_environment.get("PATH") - runner_environment["PATH"] = ( - os.pathsep.join([var_value, current_path]) - if current_path - else var_value + runner_environment["PATH"] = os.pathsep.join( + [var_value, runner_environment.get("PATH")] ) - if self._environment is not None: - runner_environment[var_name] = var_value + runner_environment[var_name] = var_value # Propagate all the other environment variables to Khiops binaries else: runner_environment[var_name] = var_value @@ -1121,7 +1124,9 @@ def _initialize_khiops_environment(self): self._initialize_default_samples_dir() def _infer_khiops_env_from_path(self, installation_method, environment=None): - search_path = None if environment is None else environment.get("PATH", "") + # Fall back to searching `khiops_env` according to the `PATH` set in + # `os.environ`. + search_path = environment.get("PATH") if environment is not None else None khiops_env_path = shutil.which("khiops_env", path=search_path) if khiops_env_path is None: raise KhiopsEnvironmentError( @@ -1196,9 +1201,7 @@ def _detect_library_installation_incompatibilities(self, library_root_dir_path): error_list = [] warning_list = [] - runner_environment = ( - os.environ if self._environment is None else self._environment - ) + runner_environment = _current_environment(self._environment) installation_method = _infer_khiops_installation_method( environment=runner_environment ) @@ -1374,9 +1377,7 @@ def _build_status_message(self): ) # Build the messages for install type and mpi - runner_environment = ( - os.environ if self._environment is None else self._environment - ) + runner_environment = _current_environment(self._environment) install_type_msg = _infer_khiops_installation_method( environment=runner_environment ) diff --git a/tests/test_core.py b/tests/test_core.py index 9b8b67c9..2ed1e664 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -24,9 +24,8 @@ import khiops import khiops.core as kh -import khiops.core.api as core_api import khiops.core.internals.filesystems as fs -from khiops.core import KhiopsRuntimeError +from khiops.core import KhiopsRuntimeError, api from khiops.core.api import _deprecate_legacy_data_path from khiops.core.internals.io import KhiopsOutputWriter from khiops.core.internals.runner import KhiopsLocalRunner, KhiopsRunner @@ -1026,8 +1025,8 @@ class KhiopsGlobalSystemParameterTests(unittest.TestCase): """Test process-wide Core API system-parameter defaults""" def setUp(self): - self._initial_inherited_environment = core_api._inherited_environment - self._initial_current_environment = core_api._current_environment + self._initial_inherited_environment = api._INHERITED_ENVIRONMENT + self._initial_current_environment = api._CURRENT_ENVIRONMENT launch_environment = os.environ.copy() launch_environment.update( @@ -1037,21 +1036,19 @@ def setUp(self): "KHIOPS_TMP_DIR": "/launch/tmp", } ) - core_api._inherited_environment = MappingProxyType(launch_environment) - core_api._current_environment = launch_environment.copy() + api._INHERITED_ENVIRONMENT = MappingProxyType(launch_environment) + api._CURRENT_ENVIRONMENT = launch_environment.copy() - self._runner_constructor_patch = mock.patch.object( - core_api, "_KhiopsLocalRunner" - ) + self._runner_constructor_patch = mock.patch.object(api, "KhiopsLocalRunner") self.runner_constructor = self._runner_constructor_patch.start() - self._set_runner_patch = mock.patch.object(core_api, "set_runner") + self._set_runner_patch = mock.patch.object(api, "set_runner") self.set_runner = self._set_runner_patch.start() def tearDown(self): self._set_runner_patch.stop() self._runner_constructor_patch.stop() - core_api._inherited_environment = self._initial_inherited_environment - core_api._current_environment = self._initial_current_environment + api._INHERITED_ENVIRONMENT = self._initial_inherited_environment + api._CURRENT_ENVIRONMENT = self._initial_current_environment def test_accessors_are_public_and_restore_launch_values(self): """Test accessors expose typed defaults and inherited reset values""" @@ -1091,12 +1088,12 @@ def test_defaults_use_isolated_complete_runner_environment(self): self.assertEqual(os.environ, initial_process_environment) self.assertEqual(self.runner_constructor.call_count, 1) - runner_environment = self.runner_constructor.call_args.kwargs["env"] + runner_environment = self.runner_constructor.call_args.kwargs["environment"] self.assertEqual(runner_environment["KHIOPS_PROC_NUMBER"], "8") self.assertEqual(runner_environment["KHIOPS_MEMORY_LIMIT"], "128") self.assertEqual(runner_environment["KHIOPS_TMP_DIR"], "/launch/tmp") - self.assertEqual(set(runner_environment), set(core_api._current_environment)) - self.assertIsNot(runner_environment, core_api._current_environment) + self.assertEqual(set(runner_environment), set(api._CURRENT_ENVIRONMENT)) + self.assertIsNot(runner_environment, api._CURRENT_ENVIRONMENT) def test_later_environment_changes_do_not_change_reset_targets(self): """Test reset values remain tied to the launch snapshot""" @@ -1108,15 +1105,15 @@ def test_later_environment_changes_do_not_change_reset_targets(self): def test_absent_inherited_values_are_removed_on_reset(self): """Test resetting an absent inherited value removes the variable""" - launch_environment = dict(core_api._inherited_environment) + launch_environment = dict(api._INHERITED_ENVIRONMENT) for environment_variable in ( "KHIOPS_PROC_NUMBER", "KHIOPS_MEMORY_LIMIT", "KHIOPS_TMP_DIR", ): launch_environment.pop(environment_variable) - core_api._inherited_environment = MappingProxyType(launch_environment) - core_api._current_environment = launch_environment.copy() + api._INHERITED_ENVIRONMENT = MappingProxyType(launch_environment) + api._CURRENT_ENVIRONMENT = launch_environment.copy() kh.set_default_max_cores(8) kh.set_default_memory_limit_mb(512) @@ -1128,7 +1125,7 @@ def test_absent_inherited_values_are_removed_on_reset(self): self.assertIsNone(kh.get_default_max_cores()) self.assertIsNone(kh.get_default_memory_limit_mb()) self.assertIsNone(kh.get_default_temp_dir()) - runner_environment = self.runner_constructor.call_args.kwargs["env"] + runner_environment = self.runner_constructor.call_args.kwargs["environment"] self.assertNotIn("KHIOPS_PROC_NUMBER", runner_environment) self.assertNotIn("KHIOPS_MEMORY_LIMIT", runner_environment) self.assertNotIn("KHIOPS_TMP_DIR", runner_environment) @@ -1136,22 +1133,63 @@ def test_absent_inherited_values_are_removed_on_reset(self): def test_invalid_updates_leave_values_unchanged(self): """Test invalid values are rejected without changing effective defaults""" invalid_updates = [ - (kh.set_default_max_cores, 0, kh.get_default_max_cores), - (kh.set_default_max_cores, "8", kh.get_default_max_cores), - (kh.set_default_memory_limit_mb, 0, kh.get_default_memory_limit_mb), - (kh.set_default_memory_limit_mb, "512", kh.get_default_memory_limit_mb), - (kh.set_default_temp_dir, "", kh.get_default_temp_dir), - (kh.set_default_temp_dir, 42, kh.get_default_temp_dir), + ( + kh.set_default_max_cores, + 0, + kh.get_default_max_cores, + "max_cores must be non-empty", + ), + ( + kh.set_default_max_cores, + -1, + kh.get_default_max_cores, + "max_cores must be positive; it is -1", + ), + ( + kh.set_default_max_cores, + "8", + kh.get_default_max_cores, + "'max_cores' type must be 'int', not 'str'", + ), + ( + kh.set_default_memory_limit_mb, + 0, + kh.get_default_memory_limit_mb, + "memory_limit_mb must be non-empty", + ), + ( + kh.set_default_memory_limit_mb, + "512", + kh.get_default_memory_limit_mb, + "'memory_limit_mb' type must be 'int', not 'str'", + ), + ( + kh.set_default_temp_dir, + "", + kh.get_default_temp_dir, + "temp_dir must be non-empty", + ), + ( + kh.set_default_temp_dir, + 42, + kh.get_default_temp_dir, + "'temp_dir' type must be 'str', not 'int'", + ), ] - expected_values = [2, 2, 128, 128, "/launch/tmp", "/launch/tmp"] - for (setter, invalid_value, getter), expected_value in zip( - invalid_updates, expected_values - ): + for setter, invalid_value, getter, expected_message in invalid_updates: with self.subTest(invalid_value=invalid_value): - with self.assertRaises((TypeError, ValueError)): + with self.assertRaises((TypeError, ValueError)) as context: setter(invalid_value) - self.assertEqual(getter(), expected_value) + self.assertEqual(str(context.exception), expected_message) + self.assertEqual( + getter(), + { + kh.get_default_max_cores: 2, + kh.get_default_memory_limit_mb: 128, + kh.get_default_temp_dir: "/launch/tmp", + }[getter], + ) class KhiopsCoreServicesTests(unittest.TestCase): diff --git a/tests/test_khiops_integrations.py b/tests/test_khiops_integrations.py index 37fabe99..e7b14a03 100644 --- a/tests/test_khiops_integrations.py +++ b/tests/test_khiops_integrations.py @@ -388,7 +388,7 @@ def test_isolated_runner_environment_is_not_written_to_process(self): with patch.object( runner_module.subprocess, "Popen", mock_popen ): - runner = KhiopsLocalRunner(env=isolated_environment) + runner = KhiopsLocalRunner(environment=isolated_environment) runner.raw_run("khiops", [], use_mpi=False) self.assertEqual(os.environ, initial_process_environment) @@ -404,41 +404,6 @@ def test_isolated_runner_environment_is_not_written_to_process(self): self.assertEqual(final_environment["KHIOPS_RETURNED"], "yes") self.assertEqual(final_environment["HOME"], "/isolated/home") - def test_default_runner_does_not_export_mpi_dll_path(self): - """Test the default runner keeps MPI DLL details out of os.environ""" - mock_popen = MagicMock() - mock_popen.return_value.__enter__.return_value.communicate.return_value = ( - "KHIOPS_PATH /bin/echo\n" - "KHIOPS_COCLUSTERING_PATH /bin/echo\n" - "KHIOPS_MPI_DLL_PATH /isolated/mpi\n", - "", - ) - mock_popen.return_value.__enter__.return_value.returncode = 0 - - with patch.dict(os.environ, {}, clear=True): - with patch.object( - runner_module, "_infer_khiops_installation_method", return_value="pip" - ): - with patch.object( - KhiopsLocalRunner, - "_infer_khiops_env_from_path", - return_value="/bin/echo", - ): - with patch.object(KhiopsLocalRunner, "_check_tools"): - with patch.object( - KhiopsLocalRunner, "_initialize_default_samples_dir" - ): - with patch.object( - runner_module.subprocess, "Popen", mock_popen - ): - runner = KhiopsLocalRunner() - runner.raw_run("khiops", [], use_mpi=False) - - self.assertNotIn("KHIOPS_MPI_DLL_PATH", os.environ) - final_environment = mock_popen.call_args_list[1].kwargs["env"] - self.assertEqual(final_environment["PATH"], "/isolated/mpi") - self.assertNotIn("KHIOPS_MPI_DLL_PATH", final_environment) - def test_khiops_and_khiops_coclustering_are_run_with_mpi(self): """Test that MODL and MODL_Coclustering are run with MPI""" From fb5d762657eaebef46438bd7c76dff3aa5e6d6e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luc-Aur=C3=A9lien?= <133773992+lucaurelien@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:36:58 +0200 Subject: [PATCH 3/3] fix --- .gitignore | 7 ------- khiops/core/internals/runner.py | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 5d56899c..82f217a9 100644 --- a/.gitignore +++ b/.gitignore @@ -6,13 +6,6 @@ # Mac/OSX .DS_Store -# OpenSpec artifacts -openspec/ -.github/agents/openspec.agent.md -.github/prompts/opsx-*.prompt.md -.github/skills/openspec-*/ -.github/workflows/copilot-setup-steps.yml - # Source for the following rules: https://raw.githubusercontent.com/github/gitignore/master/Python.gitignore # Byte-compiled / optimized / DLL files __pycache__/ diff --git a/khiops/core/internals/runner.py b/khiops/core/internals/runner.py index 1e80807d..e6553366 100644 --- a/khiops/core/internals/runner.py +++ b/khiops/core/internals/runner.py @@ -1109,7 +1109,7 @@ def _initialize_khiops_environment(self): runner_environment["PATH"] = os.pathsep.join( [var_value, runner_environment.get("PATH")] ) - runner_environment[var_name] = var_value + runner_environment["KHIOPS_MPI_DLL_PATH"] = var_value # Propagate all the other environment variables to Khiops binaries else: runner_environment[var_name] = var_value