Skip to content

Detect "unhappy" installation states #402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

tramora
Copy link
Collaborator

@tramora tramora commented Apr 25, 2025

  • Warn when the version tuple (major, minor, patch) of Khiops does not match the Khiops Python library one
  • Fix the types of the returned objects in _build_status_message
  • Detect when the library is installed by something else than conda in a conda environment
  • Detect when the conda execution environment does not match the installation one

TODO Before Asking for a Review

  • [ X] Rebase your branch to the latest version of dev (or main for release PRs)
  • 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 tramora linked an issue Apr 25, 2025 that may be closed by this pull request
@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch from 8528be2 to 48d012b Compare April 27, 2025 17:32
Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. The main point of these comments is that the unhappy installation paths are:

  • partial installations:
    • the Python lib is present, but the binaries are missing;
  • hybrid installations:
    • the binaries are installed in a Conda env (via the khiops-core package), but the Python lib is installed via Pip;
    • the binaries are installed system-wide, but the Python lib is installed in a Conda env (e.g. the khiops-core package has been uninstalled by the user);
    • the binaries are installed system-wide and the Python package is installed via Pip, but their versions are not compatible at the major.minor.patch level.
    • the binaries and the Python package are installed in a Conda env, but the binaries package got updated, alone, to a future, incompatible version (the Python lib package kept the initial version).

The following are not necessarily "unhappy" IMHO:

  • fully-compatible binary + pip installation, run from within a Conda environment (with no binaries or Python library installed in that Conda environment);
  • [with less certainty?] fully-compatible full Conda installation inside a Conda env, run from a virtualenv that is activated inside that Conda env (that is, in a shell where the Conda env is also activated), with no Khiops Python library installed in that virtualenv.

@popescu-v
Copy link
Collaborator

popescu-v commented Jun 16, 2025

Perhaps perform a 3-times check of "happy path":

  1. Check that Python executable path matches khiops __file__;
  2. If so, then check that khiops __file__ also matches (relevant part of) CONDA_PREFIX if defined;
  3. If so, then check that Khiops executable path matches (relevant part of) CONDA_PREFIX ?

Any else is an unhappy path, if CONDA_PREFIX is defined.

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch 3 times, most recently from 27d557c to 0361146 Compare June 19, 2025 15:36
@tramora
Copy link
Collaborator Author

tramora commented Jun 19, 2025

Perhaps perform a 3-times check of "happy path":

1. Check that Python executable path matches khiops `__file__`;

After a few attempts it seems that this check is pointless and a big hassle. Pointless because if a wrong python interpreter is used it wouldn't find the correct library (and digging programmatically from the python library for the python executable will lead to the sys.executable). A big hassle because of the OS specificities ('Scripts' folder for Windows instead of 'bin' for example)

2. If so, then check that khiops `__file__` also matches (relevant part of) `CONDA_PREFIX` if defined;

Indeed. This check is performed.

3. If so, then check that Khiops executable path matches (relevant part of) `CONDA_PREFIX` ?

Indeed. This check is performed.

Any else is an unhappy path, if CONDA_PREFIX is defined.

@popescu-v
Copy link
Collaborator

Perhaps perform a 3-times check of "happy path":

1. Check that Python executable path matches khiops `__file__`;

After a few attempts it seems that this check is pointless and a big hassle. Pointless because if a wrong python interpreter is used it wouldn't find the correct library (and digging programmatically from the python library for the python executable will lead to the sys.executable). A big hassle because of the OS specificities ('Scripts' folder for Windows instead of 'bin' for example)

Wouldn't this accommodate both Windows and Linux: khiops.__file__.startswith(str(pathlib.Path(sys.executable).parents[1]))?
Because otherwise, how do we make sure, if not in a Conda env, that we don't launch a Virtualenv-based Python which imports a system-wide installed Khiops?

2. If so, then check that khiops `__file__` also matches (relevant part of) `CONDA_PREFIX` if defined;

Indeed. This check is performed.

3. If so, then check that Khiops executable path matches (relevant part of) `CONDA_PREFIX` ?

Indeed. This check is performed.

Any else is an unhappy path, if CONDA_PREFIX is defined.

@popescu-v
Copy link
Collaborator

Add warning on Windows system-wide MPI precedence over Conda MPI: https://github.com/conda-forge/msmpi-feedstock/blob/32bc89b1b104dd158a12b161ae2384d5a8a5642c/recipe/activate.bat#L7

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch 4 times, most recently from c8915f8 to c5ed7d8 Compare June 30, 2025 00:52
@tramora
Copy link
Collaborator Author

tramora commented Jun 30, 2025

Add warning on Windows system-wide MPI precedence over Conda MPI: https://github.com/conda-forge/msmpi-feedstock/blob/32bc89b1b104dd158a12b161ae2384d5a8a5642c/recipe/activate.bat#L7

This case is addressed now

Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few pending changes (see comments), among which a needed fix.

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch from b91e295 to b4bb815 Compare July 31, 2025 14:26
@tramora tramora requested a review from popescu-v July 31, 2025 14:26
@popescu-v
Copy link
Collaborator

Also, add support for pip install --user (intermediate between global, system-wide install, and in-virtualenv install).

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch 2 times, most recently from 6569507 to 4392852 Compare August 9, 2025 20:55
@tramora
Copy link
Collaborator Author

tramora commented Aug 9, 2025

Also, add support for pip install --user (intermediate between global, system-wide install, and in-virtualenv install).

This "User site" installation is handled now (as an happy path). As a consequence, api docs build does not require any longer a virtual env. Thus, this specific modification in the api-docs.yml was reverted

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch from 4392852 to fc3b5c0 Compare August 11, 2025 14:21
@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch 2 times, most recently from 441c495 to 2785b62 Compare August 12, 2025 18:07
error_list.append(error)
# fetch the 'User site' site-packages
# the path is adapted for each OS (Windows, MacOS, Linux)
user_site_packages = site.getusersitepackages()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/user_site_packages/user_site_packages_dir/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable name was refactored

and thus the CONDA_PREFIX environment variable is undefined
and the path to the `bin` directory inside the conda environment is not in PATH
- 'binary+pip' installs the binaries and the shared libraries system-wide
but will keep the python libraries
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but will keep the Python libraries in the Python system folder or in the Python folder inside the home directory of the user, or in a virtual environment (if one is used)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring modified (for the pip install --user usage)

Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, LGTM.
See however a few minor comments (nice to have fixed IMHO).
Also update commit message line:
"- Detect installation incompatibilities in a system-wide pip installation" to
"- Detect installation incompatibilities in a system-wide or user-site pip installation"

Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add specific user warning if installation type is "unknown" because package metadata is missing (see https://github.com/KhiopsML/khiops-python/pull/402/files#r2284574608 ).

@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch from 2785b62 to 30682ca Compare August 19, 2025 10:59
- Warn when the version tuple (major, minor, patch) of Khiops does not match the Khiops Python library one
- Fix the types of the returned objects in `_build_status_message`
- Detect installation incompatibilities in an active conda environment
 - Detect installation incompatibilities in a conda-based environment or a virtual environment
 - Detect installation incompatibilities in a system-wide pip installation or user-site pip installation
@tramora tramora force-pushed the 301-detect-unhappy-installation-paths branch from 30682ca to 71ce8b0 Compare August 19, 2025 11:04
@tramora tramora requested a review from popescu-v August 19, 2025 11:04
@tramora
Copy link
Collaborator Author

tramora commented Aug 19, 2025

Add specific user warning if installation type is "unknown" because package metadata is missing (see https://github.com/KhiopsML/khiops-python/pull/402/files#r2284574608 ).

Warning added

@tramora
Copy link
Collaborator Author

tramora commented Aug 19, 2025

Technically, LGTM. See however a few minor comments (nice to have fixed IMHO). Also update commit message line: "- Detect installation incompatibilities in a system-wide pip installation" to "- Detect installation incompatibilities in a system-wide or user-site pip installation"

Commit message updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test "unhappy" installation paths
3 participants