-
-
Notifications
You must be signed in to change notification settings - Fork 26k
Installation guidelines now display default pip install commands and only expands in the notes #27960
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
Closed
fcharras
wants to merge
2
commits into
scikit-learn:main
from
fcharras:FIX/recommend_default_pip_flags
Closed
Installation guidelines now display default pip install commands and only expands in the notes #27960
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
But the
--no-build-isolation
seems important, isn't it?Otherwise, you are going to build with other version of library that are isolated and not use the one from your current environment.
If we go this way, we never need to request Cython for instance and only NumPy and SciPy to be installed for the runtime.
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.
I don't know either how build isolation works exactly. But if that's true maybe the doc shouldn't hint that
--no-build-isolation
is for saving time only then 🤔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.
Saving time for me it just a side effect of what is happening. The parameter allows you to:
Perfectly, we should not have to explain this in our documentation but have the proper explanation on the
pip
documentation.I assume that the part about the PEP517 is something that we never really documented: #26334 (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.
Running twice
pip install -e . --no-build-isolation
, I can see thatpip
will create a tmp folder where to have the build and the compilation will happen at each call. Adding the--no-use-pep-517
force to build into thebuild
folder of the repository directly and it will not recompile if there is any change.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.
The way I understand
--no-build-isolation
is as "use the libraries available in the environment in whichpip
is being run". It seems odd to me that you need to specify this, as an old person I think "what else would I want pip to do??", but times move on and I assume there is a good reason why the default isn't--no-build-isolation
.tl;dr: I think we should keep it, and we (as developers) understand exactly why it is there in the docs.
Until I read Guillaume's last comment here I was under the impression that no one fully understood what
--no-use-pep517
does. But from his comment it seems like he does. The naming is a bit weird, especially if you don't know your PEPs off by heart, but it seems it does something useful and we can explain it.Should we explain it? I don't know, maybe link to the pip docs?
As long as we don't know of any negative side-effects for the unsuspecting developer, I think we should keep the command as is (and feel sad that the names are a bit abstract). The windows build issue might be a reason to remove
--no-use-pep517
.Uh oh!
There was an error while loading. Please reload this page.
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.
Isn't there to understand that build dependencies are temporarily fetched into a separate, temporary file tree if requirements are not already met by the user runtime environment ? I don't find any documentation of it actually having to do with compiled extensions and such.
I'd say that in general using different environments for different purposes is good, avoid a lot of troubles and unexpected interactions caused by unsuspected incompatible dependency trees and such, having the build system alter the dependencies of the environment that will be used at runtime is one of those instances of mixing things ~ especially for packages that can have build time dependencies that are not also runtime dependencies.