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 ?

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-06-01T15:43:45Z

                    f"'{installation_method}' installation method. Make sure "
-                    "you have installed khiops >= 10.2.3. "
+                    "you have installed properly the Khiops Python library "
+                    "and Khiops (if this later was not installed automatically). "


s/this later/the latter/

s/this latter/the latter/

popescu-v · 2026-06-01T15:44:34Z

        installation_method = _infer_khiops_installation_method()

        # Fail immediately if the major versions differ
+        # It can still occur if the installation method is 'conda' or 'conda-based',


Why? In the Conda case, the dependency is also specified, in meta.yml, right?

popescu-v · 2026-06-01T15:45:17Z


 __version__ = "11.0.0.3"

+# The current Khiops Python library (when packaged as a PyPI package)


I would move this snippet to the runner, in the context of the "pip" installation method.

Moved into the KhiopsLocalRunner::_detect_library_installation_incompatibilities...

If the khiops-core PyPI package was removed, an error is raised far before in KhiopsLocalRunner::_initialize_khiops_environment except in a very edge case I faced during my tests : khiops-core PyPI package was installed system-wide. In this case KhiopsLocalRunner::_initialize_khiops_environment runs successfully but the current detection raises an error

I think the environment should not initialize successfully in this case. See comments https://github.com/KhiopsML/khiops-python/pull/582/changes#r3373317679 and https://github.com/KhiopsML/khiops-python/pull/582/changes#r3373326847.

popescu-v

See pending 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 khiops packages from TestPypi

tramora · 2026-06-05T10:20:07Z

An issue remains while executing the "expensive tests" and thus running the integration tests for remote files access.
It is tracked on the drivers repository : KhiopsML/khiopsdriver-shared#2

popescu-v · 2026-06-08T12:43:38Z

+          python -m pip install tomli
+          # First, install all dependencies except khiops-core and khiops-drivers-*
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --exclude-khiops-family > requires-no-khiops.txt          
+          python -m pip install --user `cat requires-no-khiops.txt`


Why not -r requires-no-khiops.txt rather than cat requires-no-khiops.txt ?

popescu-v · 2026-06-08T12:44:37Z

+          python -m pip install --user `cat requires-no-khiops.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --khiops-family-only > requires-khiops.txt
+          python -m pip install --user --index-url https://test.pypi.org/simple `cat requires-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:46:03Z

+          pip install `cat requires-no-khiops.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --khiops-family-only > requires-khiops.txt
+          pip install --index-url https://test.pypi.org/simple `cat requires-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:46:30Z

+          pip install tomli
+          # First, install all dependencies except khiops-core and khiops-drivers-*
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --exclude-khiops-family > requires-no-khiops.txt
+          pip install `cat requires-no-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:46:43Z

+          pip install tomli
+          # First, install all dependencies except khiops-core and khiops-drivers-*
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --exclude-khiops-family > requires-no-khiops.txt
+          pip install `cat requires-no-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:46:50Z

+          pip install `cat requires-no-khiops.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --khiops-family-only > requires-khiops.txt
+          pip install --index-url https://test.pypi.org/simple `cat requires-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:47:16Z

+          $CONDA install -y -n "$CONDA_ENV" pip
+          # First, install all dependencies except khiops-core and khiops-drivers-*
+          $CONDA run --no-capture-output -n "$CONDA_ENV" python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --exclude-khiops-family > requires-no-khiops.txt
+          $CONDA run -n "$CONDA_ENV" pip install `cat requires-no-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:47:28Z

+          $CONDA run -n "$CONDA_ENV" pip install `cat requires-no-khiops.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          $CONDA run --no-capture-output -n "$CONDA_ENV" python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --khiops-family-only > requires-khiops.txt
+          $CONDA run -n "$CONDA_ENV" pip install --index-url https://test.pypi.org/simple `cat requires-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:48:02Z

-          rm -f requires.txt
+          # First, install all dependencies except khiops-core and khiops-drivers-*
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --exclude-khiops-family > requires-no-khiops.txt
+          pip install `cat requires-no-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:48:13Z

+          pip install `cat requires-no-khiops.txt`
+          # khiops-core and khiops-drivers-* must always be installed from TestPyPI in order to avoid distorting usage statistics
+          python scripts/extract_dependencies_from_pyproject_toml.py -f "pyproject.toml" --khiops-family-only > requires-khiops.txt
+          pip install --index-url https://test.pypi.org/simple `cat requires-khiops.txt`


Why not -r rather than cat?

popescu-v · 2026-06-08T12:52:18Z

-        # variable set by the Khiops Desktop Application installer
+        # Search for the `khiops_env` script location
        installation_method = _infer_khiops_installation_method()
-        if platform.system() == "Windows" and installation_method == "binary+pip":


Why not put the khiops-core package check in case of Pip installation here?

popescu-v · 2026-06-08T12:53:50Z

+            try:
+                distribution("khiops-core")
+            except PackageNotFoundError as exc:
+                error = (
+                    f"The Khiops binaries are not installed properly: {exc}. "
+                    "Re-install the Khiops Python library to automatically install "
+                    "Khiops. Go to https://khiops.org for more information.\n"
+                )
+                error_list.append(error)


I would put this check and fail (raise exception) far earlier, when the attempt is made to initialize the Khiops environment, just after the installation method has been detected (see the other comment in this respect, above).

popescu-v · 2026-06-08T13:01:39Z

+        an absolute path is inferred using the current folder which will probably
+        cause an error eventually.
+
+    this method raises a `RuntimeError` if the dynamic library is not found


Reformat this line to:

Raises ------- `RuntimeError` The dynamic library of the driver is not found.

popescu-v · 2026-06-08T13:02:48Z

+
+    this method raises a `RuntimeError` if the dynamic library is not found
+    """
+    assert type(remote_storage_type) == str and len(remote_storage_type) > 0


The second assert implies the first one AFAIU. Hence, I would remove the first assert.

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