Skip to content

Conversation

@tramora
Copy link
Collaborator

@tramora tramora commented Dec 12, 2025

  • in case of major version mismatch, raise immediately an error
  • in case of a mix between conda and something else (pip or a cloned source folder), we suggest deactivating the conda env or install the library inside this later
  • discourage using system-wide khiops binary and a library inside a conda env

Fixes #515


TODO Before Asking for a Review

  • 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 force-pushed the 515-improve-messages-unhappy-installation-paths branch from ca1a8df to f7a31d4 Compare December 12, 2025 17:56
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.

See the comment.
I also think that we should add a test that an exception is raised if the major versions are different between Core and the Python library, in test_khiops_integrations.py plus changes in the CI. Since this test is out of the scope of this PR, I would update issue #382 accordingly with a comment.

@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch 2 times, most recently from d2727e4 to ef51808 Compare December 14, 2025 23:10
@tramora
Copy link
Collaborator Author

tramora commented Dec 14, 2025

The users may be surprised that the major version mismatch raises an exception and cuts the running flow immediately.
I could be changed to delay this and show then a "clean" installation status report.
The major drawback of this alternative is that the users may never run the installation status check and run tasks against an incompatible version of Khiops. The implementation of this PR seems then the best way to raise this error. It the users complain we could think about.

@tramora
Copy link
Collaborator Author

tramora commented Dec 14, 2025

See the comment. I also think that we should add a test that an exception is raised if the major versions are different between Core and the Python library, in test_khiops_integrations.py plus changes in the CI. Since this test is out of the scope of this PR, I would update issue #382 accordingly with a comment.

In my opinion this very border-case of incompatible major version is not worth being implemented into test_khiops_integrations.py and if so it would technically more difficult to reach the dedicated conda environment I'm thinking putting a Khiops10 installation.

It seems simplier to me to stick to the setup into the docker dev images and test directly within the CI workflow (before running test_khiops_integrations.py)

@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch from ef51808 to 638324b Compare December 14, 2025 23:52
@tramora tramora requested a review from popescu-v December 14, 2025 23:57
- in case of major version mismatch, raise immediately an error
- in case of a mix between conda and something else (pip or a cloned source folder), we suggest deactivating the conda env or install the library inside this later
- discourage using system-wide khiops binary and a library inside a conda env
@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch from 638324b to 199a9d6 Compare December 15, 2025 00:05
@popescu-v
Copy link
Collaborator

The users may be surprised that the major version mismatch raises an exception and cuts the running flow immediately. I could be changed to delay this and show then a "clean" installation status report. The major drawback of this alternative is that the users may never run the installation status check and run tasks against an incompatible version of Khiops. The implementation of this PR seems then the best way to raise this error. It the users complain we could think about.

I think failing immediately if the major versions do not match is the correct behavior, because we are sure, by the convention of the major releases, that the binaries are incompatible with the Python library, hence it seems pointless to execute anything else beyond this check if the major versions do not match.

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.

See the comments.

@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch 4 times, most recently from 466d010 to 9759275 Compare December 15, 2025 13:37
@tramora tramora requested a review from popescu-v December 15, 2025 13:38
@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch from 9759275 to 9c4572e Compare December 15, 2025 13:51
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.

Just a few comments left.

@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch 2 times, most recently from 279074f to b0b6595 Compare December 15, 2025 14:28
@tramora tramora requested a review from popescu-v December 15, 2025 14:34
@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch from b0b6595 to 5729e03 Compare December 15, 2025 14:36
- Khiops 10 is installed in a dedicated conda environment for this purpose
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.

LGTM

@tramora tramora force-pushed the 515-improve-messages-unhappy-installation-paths branch from 5729e03 to 8ba7676 Compare December 15, 2025 15:42
@tramora tramora merged commit 1775b09 into dev Dec 15, 2025
22 checks passed
@tramora tramora deleted the 515-improve-messages-unhappy-installation-paths branch December 15, 2025 15:56
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.

Requested message improvements for the unhappy installation paths

3 participants