Set the `KHIOPS_PROC_NUMBER` env var for each training run using the value of `max_cores` by tramora · Pull Request #623 · KhiopsML/khiops-python

tramora · 2026-09-20T22:43:44Z

the allocated number of CPU cores can never be greater than the value of max_cores

Fixes #585

Implementation details :

the changes to the runner added by @lucaurelien in PR Enable System Parameters Globally #622 are reported because they are needed in this one and will be useful when the API to set globally the system parameters is implemented

TODO Before Asking for a Review

Rebase your branch to the latest version of main (or main-v10)
Make sure all CI workflows are green
When adding a public feature/fix: Update the Unreleased section of CHANGELOG.md (no date)
Self-Review: Review "Files Changed" tab and fix any problems you find
API Docs (only if there are changes in docstrings, markdown files or samples):
- Check the docs build without warning: see the log of the API Docs workflow
- Check that your changes render well in HTML: download the API Docs artifact and open index.html
- If there are any problems it is faster to iterate by building locally the API Docs

popescu-v

See the comments.

popescu-v · 2026-10-09T09:50:15Z

+
+
+def get_default_samples_dir(environment=None):
    """Returns the default samples directory


Document the environment parameter.

popescu-v · 2026-10-09T11:18:23Z

@@ -946,8 +968,18 @@ class KhiopsLocalRunner(KhiopsRunner):



I would document the environment in the class docstring, then the __init__ should only have "See class docstring" as its docstring (cf. AnalysisResults and other classes in khiops.core).

popescu-v · 2026-10-09T11:18:39Z

+    def __init__(self, environment=None):
+        """Initialize a local runner.
+
+        Parameters


Move to the class docstring (see the comment above).

popescu-v · 2026-10-09T11:46:33Z

+        # Ensure the previous runner is a KhiopsLocalRunner
+        # to avoid overwriting a mocked runner


Mocked runners are test-specific objects. IMHO they should not be mentioned in test code. I would just state "Ensure KHIOPS_PROC_NUMBER is set on KhiopsLocalRunner instances". And, in order to make sure we are dealing with "KhiopsLocalRunner" and not its subclasses, we could do if type(get_runner()) == KhiopsLocalRunner IMHO.

popescu-v · 2026-10-09T11:47:01Z

+        # to avoid overwriting a mocked runner
+        if isinstance(get_runner(), KhiopsLocalRunner):
+            khiops_env = os.environ.copy()
+            # The max pre-allocated cpu cores must have the same value


Insert empty line before this comment.

popescu-v · 2026-10-09T11:48:26Z

+        # Ensure the previous runner is a KhiopsLocalRunner
+        # to avoid overwriting a mocked runner
+        if isinstance(get_runner(), KhiopsLocalRunner):
+            khiops_env = os.environ.copy()


I would rename khiops_env to something like task_environment to avoid confusion with the khiops_env script.

popescu-v · 2026-10-09T11:55:29Z

+            "kh_samples", "train_predictor_file_paths", "AnalysisResults.khj"
+        )
+        # The existing MockedRunnerContext class is not used here
+        # as it creates a new KhiopsLocalRunner instance that does not fit our needs


Add comment here, like: "# Patch KhiopsLocalRunner so that Khiops is not run"

popescu-v · 2026-10-09T11:57:15Z

+                target_variable="class",
+                analysis_report_file_path=report_file_path,
+                trace=True,
+                max_cores=17,


Set to variable, like expected_max_cores rather than hard-coding it, so that the test is more readable (as it would reuse this variable).

popescu-v

A few comments.

…value of `max_cores` - the pre-allocated number of CPU cores can never be greater than the value of `max_cores` - the KhiopsLocalRunner accepts a 'private' local environment so that the `max_cores` value is valid only for the run

tramora force-pushed the max-cores-allocated branch from b2f355a to e532ce2 Compare September 22, 2026 09:53

tramora requested a review from popescu-v September 22, 2026 09:54

popescu-v reviewed Sep 24, 2026

View reviewed changes

Comment thread khiops/core/internals/runner.py Outdated

popescu-v reviewed Sep 24, 2026

View reviewed changes

Comment thread tests/test_khiops_integrations.py Outdated

popescu-v requested changes Sep 24, 2026

View reviewed changes

tramora force-pushed the max-cores-allocated branch 2 times, most recently from e39c91e to 7e13e1b Compare September 28, 2026 09:49

tramora requested a review from popescu-v September 28, 2026 09:57

tramora mentioned this pull request Oct 1, 2026

Add max_cores to the sklearn estimators to limit the number of CPU cores pre-allocated for the training #626

Open

4 of 6 tasks

popescu-v reviewed Oct 9, 2026

View reviewed changes

popescu-v requested changes Oct 9, 2026

View reviewed changes

tramora force-pushed the max-cores-allocated branch from 7e13e1b to 20aab02 Compare October 9, 2026 14:41

tramora requested a review from popescu-v October 9, 2026 14:42



		def get_default_samples_dir(environment=None):
		"""Returns the default samples directory

		# Ensure the previous runner is a KhiopsLocalRunner
		# to avoid overwriting a mocked runner

Conversation

tramora commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO Before Asking for a Review

Uh oh!

Uh oh!

Uh oh!

popescu-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popescu-v left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tramora commented Sep 20, 2026 •

edited

Loading