-
Notifications
You must be signed in to change notification settings - Fork 2
Add the detection of a python virtual environment when searching for unhappy installation paths under Windows #514
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
…unhappy installation paths under Windows - python sys.base_prefix differs from sys.prefix in conda-based installation or pip installation within a virtual env
pyproject.toml
Outdated
| dependencies = [ | ||
| "pandas>=0.25.3", | ||
| "scikit-learn>=0.22.2", | ||
| # do not necessarily use the latest version, to avoid undesired breaking |
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.
Remove necessarily. Fit to one line.
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.
Fixed
.github/workflows/tests.yml
Outdated
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host "::error::Status error: improper setup, as expected: khiops-python has been cloned, not installed from a package" | ||
| } | ||
| } |
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.
Remove this whitespace.
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.
Fixed
| # Install the Python requirements inside a venv | ||
| # The venv python executable is used here | ||
| Get-Content .\requires.txt ` | ||
| | ForEach-Object {khiops-windows-venv\Scripts\python -m pip install $_.toString()} | ||
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.
Instead of a ForEach-Object loop simply use python -m pip install --requirements requires.txt
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.
It was a existing code, maybe it was to avoid issues manipulating multi-lines file under DOS (\r\n) ?
But OK to simplify this
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.
With the current command we are sure we get one dependency per line.
| # Print status | ||
| # The venv python executable is used here | ||
| Invoke-Expression -Command "Scripts\python -c 'import sys; import khiops.core as kh; return_code = kh.get_runner().print_status(); sys.exit(return_code)'" | ||
| # The installation status MUST not fail |
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.
Add a whiteline above to make a paragraph.
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.
Fixed
.github/workflows/tests.yml
Outdated
| # Install khiops-python in the python venv using the sources | ||
| Invoke-Expression -Command "khiops-windows-venv\Scripts\Activate.ps1" | ||
| # The venv python executable is used here | ||
| Invoke-Expression -Command "khiops-windows-venv\Scripts\python -m pip install ." |
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.
If you are using directly the venv python path I don't think it is necessary to activate it. So I'd remove the first command.
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.
Fixed
8281994 to
4dc2b18
Compare
4dc2b18 to
c240e99
Compare
TODO Before Asking for a Review
dev(ormainfor release PRs)Unreleasedsection ofCHANGELOG.md(no date)index.html