-
Notifications
You must be signed in to change notification settings - Fork 2
Improve messages and behaviour when detecting unhappy installation paths #516
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
Conversation
ca1a8df to
f7a31d4
Compare
There was a problem hiding this 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.
d2727e4 to
ef51808
Compare
|
The users may be surprised that the major version mismatch raises an exception and cuts the running flow immediately. |
In my opinion this very border-case of incompatible major version is not worth being implemented into It seems simplier to me to stick to the setup into the docker dev images and test directly within the CI workflow (before running |
ef51808 to
638324b
Compare
- 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
638324b to
199a9d6
Compare
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. |
popescu-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments.
466d010 to
9759275
Compare
9759275 to
9c4572e
Compare
popescu-v
left a comment
There was a problem hiding this 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.
279074f to
b0b6595
Compare
b0b6595 to
5729e03
Compare
- Khiops 10 is installed in a dedicated conda environment for this purpose
popescu-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5729e03 to
8ba7676
Compare
Fixes #515
TODO Before Asking for a Review
dev(ormainfor release PRs)Unreleasedsection ofCHANGELOG.md(no date)index.html