Use khiops-core and khiops-driver-* pip packages containing binaries only by tramora · Pull Request #582 · KhiopsML/khiops-python

tramora · 2026-05-26T16:22:12Z

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, rst 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

tramora · 2026-05-28T23:51:33Z

False sentiment of success : we simulate multiple Python environments using conda (#573). This is very unfortunate as we are not able yet to detect automatically an issue with the new PyPI packages regarding the remote drivers khiops-core:1041. In my opinion this one is a blocking issue.

popescu-v · 2026-05-29T09:37:34Z

False sentiment of success : we simulate multiple Python environments using conda (#573). This is very unfortunate as we are not able yet to detect automatically an issue with the new PyPI packages regarding the remote drivers khiops-core:1041. In my opinion this one is a blocking issue.

Hence, to properly test this, issue #573 must be addressed first. Right, @tramora ?

popescu-v · 2026-05-29T10:13:31Z

-          python -m pip install --user .
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          # Use of both 'index-url' and '--extra-index-url' options allows indexes prioritization (PEP 766)
+          python -m pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple --user .


I am not sure I see why just python -m pip install --extra-index-url https://test.pypi.org/simple does not suffice:

the latest version of khiops-core is taken from test.pypi.org;

all other dependencies are taken from pypi.org.

Why is this not enough (no need for drivers to build the API docs)?

It was must first change but ... when the package will be on both PyPI and Test PyPI (under the same version or not), as PyPI is the index in the list, it will be used first. That was the rationale of the reordering

I see. In this case, in order to make sure all other dependencies than khiops-*(core and drivers) are taken from pypi.org, IMHO we need to:

First install all dependencies except khiops-*-ones from pypi.org

Install only the khiops-* packages from test.pypi.org only (as the only --index-url, no --extra-index-url).

I will split into 2 distinct lines then.

There is a difficulty however in a few cases

when a pip install . is performed in the khiops-python folder. (in api-docs.yml)

when the khiops-python "tar.gz" artifact is installed (in pip.yml)

on Windows Powershell where the perl and grep tools are not available

It seems impossible to perform a on-the-fly filtering (unless doing complex tricks)

Refactored in splitting in two lines except for Windows (PowerShell) where I do not know yet the tools that would help me

popescu-v · 2026-05-29T10:16:41Z

-          echo "KHIOPSDEV_OS_CODENAME=$(echo '${{ matrix.khiopsdev-os }}' | tr -d '0-9.')" >> "$GITHUB_ENV"
+          KHIOPSDEV_OS_CODENAME=$(echo '${{ matrix.khiopsdev-os }}' | tr -d '0-9.')
+          echo "KHIOPSDEV_OS_CODENAME=${KHIOPSDEV_OS_CODENAME}" >> "$GITHUB_ENV"
+          if [[ "${KHIOPSDEV_OS_CODENAME}" == "ubuntu" || "${KHIOPSDEV_OS_CODENAME}" == "debian" ]]; then


I would use a Bash case here with an error message in the default case.

I changed the syntax, unless I'm wrong, there is no error message to show in the default case as then a specific Dockerfile is used ("Dockerfile.rocky")

I changed the syntax, unless I'm wrong, there is no error message to show in the default case as then a specific Dockerfile is used ("Dockerfile.rocky")

I thought about explicitly supporting debian, ubuntu (one case), and rocky (second case), with an error for any other codename.

Changed but in my opinion it adds a burden to the maintainer (compared to the situation before this PR).
The matrix of all the supported OSes is defined in the same file and the Dockerfile extension was evaluated using each value of the matrix (after suppressing the digits or .). It seems unnecessary to verify that every value is in the same matrix

popescu-v · 2026-05-29T10:17:29Z

-          pip install $(ls khiops*.tar.gz)
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          # Use of both 'index-url' and '--extra-index-url' options allows indexes prioritization (PEP 766)
+          pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple $(ls khiops*.tar.gz)


See comment above about the --extra-index-url.

Refactored in splitting in two lines except for Windows (PowerShell) where I do not know yet the tools that would help me

popescu-v · 2026-05-29T10:18:13Z

+
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          # Use of both 'index-url' and '--extra-index-url' options allows indexes prioritization (PEP 766)
+          pip-compile --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple -o requirements.txt


Idem as above.

Refactored in splitting in two lines except for Windows (PowerShell) where I do not know yet the tools that would help me

popescu-v · 2026-05-29T10:18:19Z

          # Install dev dependencies
-          pip install -r requirements.txt
+          # Same indexes prioritization as above
+          pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple -r requirements.txt


Idem as above.

Refactored in splitting in two lines except for Windows (PowerShell) where I do not know yet the tools that would help me

popescu-v · 2026-05-29T10:21:18Z

+          # Use of both 'index-url' and '--extra-index-url' options allows indexes prioritization (PEP 766)
          Get-Content .\requires.txt `
-            | ForEach-Object {python -m pip install $_.toString()}
+            | ForEach-Object {python -m pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple $_.toString()}


Same as above regarding --extra-index-url.

for Windows (PowerShell), I do not know yet the tools that would help me split into 2 lines

popescu-v · 2026-05-29T10:21:45Z

+          # Same indexes prioritization as above
          Get-Content .\requires.txt `
-            | ForEach-Object {khiops-windows-venv\Scripts\python -m pip install $_.toString()}
+            | ForEach-Object {khiops-windows-venv\Scripts\python -m pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple $_.toString()}


Cf. the --extra-index-url comments above.

for Windows (PowerShell), I do not know yet the tools that would help me split into 2 lines

popescu-v · 2026-05-29T10:21:59Z

-          pip install `cat requires.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          # Use of both 'index-url' and '--extra-index-url' options allows indexes prioritization (PEP 766)
+          pip install --index-url https://test.pypi.org/simple --extra-index-url https://pypi.org/simple `cat requires.txt`


Same comment as above.

Refactored in splitting in two lines except for Windows (PowerShell) where I do not know yet the tools that would help me

popescu-v · 2026-05-29T10:23:49Z

    return uri_info._replace(path=_child_path(uri_info.path, child_name))


+def _check_khiops_driver_library(khiops_drivers_path, remote_storage_type):


Have khiops_drivers_path as a kwarg set to None by default.

popescu-v · 2026-05-29T10:28:19Z

+        # We cannot guarantee where the dynamic library is located.
+        # It seems wiser to avoid raising a false error
+        return
+    extension = ".dll" if platform.system() == "Windows" else ".so"


I think the DLL extension on MacOS is .dylib. Cf. e.g. https://github.com/KhiopsML/khiopsdriver-gcs/blob/943fef618b689d8e1abb05e6feb70e1f8efd1db7/test/plugin_test.cpp#L32.

You are right, even if I read in different pages that the ".so" was supported first to avoid differences between the "*BSD/Darwin" world and the Linux... then that ".so" was intented only for "Shared Modules" not "Shared Libraries" that shoud be ".dylib" or ".framework". Nevertheless it is implemented as a ".dylib" in our case.

popescu-v · 2026-05-29T10:28:46Z

    return uri_info._replace(path=_child_path(uri_info.path, child_name))


+def _check_khiops_driver_library(khiops_drivers_path, remote_storage_type):


Add type and value checks for remote_storage_type (as asserts, because this is a "private" function by convention) - also see the comment below in this respect.

tramora · 2026-05-29T10:30:36Z

False sentiment of success : we simulate multiple Python environments using conda (#573). This is very unfortunate as we are not able yet to detect automatically an issue with the new PyPI packages regarding the remote drivers khiops-core:1041. In my opinion this one is a blocking issue.

Hence, to properly test this, issue #573 must be addressed first. Right, @tramora ?

I changed the $CONDA install -y -n env ... to $CONDA run -n env pip install ... in order to really use the PyPI packages.
But it is not perfect yet as the run will set the CONDA_PREFIX environment variable that is checked by khiops-python to determine if the installation is done in a 'conda' method or not. 'conda' method and 'pip' one are close but not totally identical

popescu-v · 2026-05-29T10:35:30Z

+    if khiops_drivers_path is None or len(khiops_drivers_path) == 0:
+        # We cannot guarantee where the dynamic library is located.
+        # It seems wiser to avoid raising a false error
+        return


I would ban None or the empty string (or bytes) as a khiops_drivers_path value: if this function is called, as it is "private" by convention, we can just assert that khiops_drivers_path is not None and it is of type str or bytes and non-empty at its very beginning: it is the programmer's responsibility to make sure khiops_drivers_path has proper type and value before calling the function IMHO.

popescu-v · 2026-05-29T10:37:01Z

+        return
+    extension = ".dll" if platform.system() == "Windows" else ".so"
+    absolute_path = os.path.join(
+        khiops_drivers_path,


I would write os.path.abspath(khiops_drivers_path) instead, to make sure the path is absolute.

popescu-v · 2026-05-29T10:37:44Z

+    extension = ".dll" if platform.system() == "Windows" else ".so"
+    absolute_path = os.path.join(
+        khiops_drivers_path,
+        "libkhiopsdriver_file_" f"{remote_storage_type}{extension}",


Put all this line inside a single f-string: f"libkhiopsdriver_file_{remote_storage_type}{extension}"

popescu-v · 2026-05-29T10:41:03Z


    def __init__(self, uri):
+
+        _check_khiops_driver_library(os.environ.get("KHIOPS_DRIVERS_PATH"), "gcs")


Only call this function if KHIOPS_DRIVERS_PATH is properly set (see comments above regarding this function). Also, note that KHIOPS_DRIVERS_PATH is not supposed to be changed by the end-user. A wrong value of it is an upstream bug.

popescu-v · 2026-05-29T10:41:21Z


    def __init__(self, uri):
+
+        _check_khiops_driver_library(os.environ.get("KHIOPS_DRIVERS_PATH"), "s3")


Same comment as above.

popescu-v · 2026-05-29T10:41:36Z

        Azure Storage Resource initializer common to Files and Blobs
        """

+        _check_khiops_driver_library(os.environ.get("KHIOPS_DRIVERS_PATH"), "azure")


Same comment as above.

popescu-v · 2026-05-29T10:43:59Z

-            # KHIOPS_HOME variable by default
-            if "KHIOPS_HOME" in os.environ:
+        if platform.system() == "Windows" and installation_method == "pip":
+            # On Windows native installations,


On Windows, Pip should also install khiops-core. Hence, I am not sure I understand why all this comment is still relevant. Windows native installation should be out-of-scope now, shouldn't it?

As I commented in the initial issue, on Linux at least, given the 3 levels "venv pip", "system-wide pip", "system-wide native", if the user removes the khiops-core package, the khiops in upper level is used instead (khiops_env is searched in the available PATH).

On Windows, the system-wide native is the equivalent of the "system-wide native" cited above. In my opinion forbidding its use (in ignoring the KHIOPS_HOME set by the native installer) will make a behavior difference between the 2 platforms

IMHO, we should not support fallback to native installation: if the user removes the khiops-core Pip package, then we should detect this and fail right away (I would say, directly in khiops/__init__.py): indeed, now the link between the Python library and the binaries is no longer (only) a matter of local runner, but a matter of packaging. Thus, in khiops/__init__.py, we could do something like this:

from importlib.metadata import distribution, PackageNotFoundError try: distribution("khiops-core") except PackageNotFoundError as exc: raise KhiopsEnvironmentError(f"The Khiops binaries are not installed properly: ${exc}") from exc

I think this should cover the case evoked here. In any case, IMHO the fallback to OS-native packages should not be authorized, because we would run into version compatibility checks etc, which would defeat the purpose of having a "full-pip" or "full-conda" Python experience with Khiops.

Agreed.

I then copied your snippet into khiops/__init.py but I had to comment it out immediately until we find a compatible fix.
Indeed in 'conda' and 'conda-based' installation methods where a Conda package is installed (not a PyPI package)
distribution("khiops-core") always raises a PackageNotFoundError

On Windows 'pip', I also modified the error message if khiops_env cannot be found : the user must check the installation of the 'Khiops Python library' not 'Khiops' any longer

popescu-v · 2026-05-29T10:45:19Z

+            # Miss : KHIOPS_HOME variable evaluation is the fallback
+            elif "KHIOPS_HOME" in os.environ:
                khiops_env_path = os.path.join(
                    os.environ["KHIOPS_HOME"], "bin", "khiops_env.cmd"
                )
-            # Raise error if KHIOPS_HOME is not set


Why do we still need this? Either khiops_env.cmd can be found as expected according to the current Python environment (virtualenv, user-site or system-wide - discouraged), or it cannot. Why rely on KHIOPS_HOME?

related to the comment #582 (comment)

See comment #582 (comment).

popescu-v · 2026-05-29T10:46:00Z

                raise KhiopsEnvironmentError(
-                    "No environment variable named 'KHIOPS_HOME' found. "
+                    "No 'khiops_env.cmd' found in current environment nor "
+                    "in 'KHIOPS_HOME' location. "


We should drop KHIOPS_HOME fallback IMHO (see comments above).

related to the comment #582 (comment)

See comment #582 (comment).

popescu-v · 2026-05-29T10:48:29Z

+                # must be appended to "PATH" otherwise Khiops wouldn't find it
+                # and fail immediately
+                elif var_name == "KHIOPS_MPI_DLL_PATH":
+                    os.environ["PATH"] = os.environ.get("PATH") + os.pathsep + var_value


I would write either os.pathsep.join([var_value, os.environ.get("PATH")]), or f"{var_value}{os.pathsep}{os.environ.get('PATH')}" and prefer the former (more readable IMHO).

But anyway, I would prepend var_value to the path, to make sure the correct MPI path takes precedence over whatever (potentially system-wide) MPI installation might preexist.

popescu-v · 2026-05-29T10:53:42Z

 - (`sklearn`) `keep_selected_variables_only` parameter to the predictors (`KhiopsClassifier` and `KhiopsRegressor`)
 - (General) Support for Azure storage
+### Changed
+- *Internals*:


I would place this change under (General) and rephrase it to something like:
"- (General) Full-Pip installation support: khiops now depends on the khiops-core and remote storage driver Pip packages"

popescu-v · 2026-05-29T10:55:58Z

 - (General) Support for Azure storage
+### Changed
+- *Internals*:
+  - Declaration of khiops-core and khiops drivers as dependencies (mandatory or optional)


I would also add an entry to the ### Removed section, along the lines of:
"- Support for OS-native Khiops Core and Pip Khiops Python library installations"

Line added even if I'm not 100% sure because of #582 (comment)

See comment #582 (comment).

popescu-v

See the comments.

…ries only - these packages become mandatory ("khiops-core") or optional ("khiops-driver-*") dependencies dragged during the installation process - these packages must be installed in the Conda environments used to simulate multiple environs (the Conda packages MUST not be used) - in order to avoid distorting usage statistics the test workflows will always use packages from TestPypi

tramora marked this pull request as draft May 26, 2026 16:23

tramora force-pushed the core-and-drivers-as-pip-packages branch 9 times, most recently from caa2f18 to fd7c550 Compare May 28, 2026 10:32

tramora marked this pull request as ready for review May 28, 2026 13:44

tramora requested a review from popescu-v May 28, 2026 13:44

tramora force-pushed the core-and-drivers-as-pip-packages branch 2 times, most recently from 394d474 to 5312426 Compare May 28, 2026 21:33

tramora force-pushed the core-and-drivers-as-pip-packages branch from 5312426 to b0223fa Compare May 29, 2026 00:37

tramora force-pushed the core-and-drivers-as-pip-packages branch from 95a1d21 to cfd1802 Compare May 29, 2026 09:43

popescu-v reviewed May 29, 2026

View reviewed changes

popescu-v requested changes May 29, 2026

View reviewed changes

tramora force-pushed the core-and-drivers-as-pip-packages branch 5 times, most recently from daeaf02 to 19e7ca7 Compare May 29, 2026 23:38

Thierry RAMORASOAVINA added 2 commits May 30, 2026 01:50

Change code formatting to match 'black' linter suggestion

e45f7dc

tramora force-pushed the core-and-drivers-as-pip-packages branch from 19e7ca7 to e45f7dc Compare May 29, 2026 23:50

		return uri_info._replace(path=_child_path(uri_info.path, child_name))


		def _check_khiops_driver_library(khiops_drivers_path, remote_storage_type):


		def __init__(self, uri):

		_check_khiops_driver_library(os.environ.get("KHIOPS_DRIVERS_PATH"), "gcs")

Conversation

tramora commented May 26, 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!

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

Uh oh!

Uh oh!

popescu-v commented May 29, 2026

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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popescu-v May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tramora commented May 26, 2026 •

edited

Loading

tramora commented May 28, 2026 •

edited

Loading

tramora May 29, 2026 •

edited

Loading

tramora May 29, 2026 •

edited

Loading

tramora May 29, 2026 •

edited

Loading

popescu-v May 29, 2026 •

edited

Loading

popescu-v May 29, 2026 •

edited

Loading

tramora commented May 29, 2026 •

edited

Loading

popescu-v May 29, 2026 •

edited

Loading