Skip to content

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

Conversation

fcharras
Copy link
Contributor

What does this implement/fix? Explain your changes.

Change the developers documentation to primarily showcase usage of pip with default flags rather than always with --no-build-isolation and --no-use-pep517. Only suggests those flags in side notes. Why:

So I'm proposing to re-write it a way that hints that usage of those flags are for more experienced developers that need to optimize their workflow, and suggests that the default usage should be preferred for simpler cases or bug reports (maybe this could be explicitly stated too ?)

@fcharras fcharras changed the title Advanced installation now display default pip install commands and only expands in the note Installation guidelines now display default pip install commands and only expands in the notes Dec 14, 2023
Copy link

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: ccfa6e6. Link to the linter CI: here

@@ -96,7 +96,7 @@ feature, code or documentation improvement).

.. prompt:: bash $

pip install -v --no-use-pep517 --no-build-isolation -e .
pip install -v -e .
Copy link
Member

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.

Copy link
Contributor Author

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 🤔

Copy link
Member

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:

  • control the version of library used to compiled since you are not creating an isolated environment
  • you will not recompile everything from scratch since you leverage the already compiled files since you are not isolated

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)

Copy link
Member

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 that pip 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 the build folder of the repository directly and it will not recompile if there is any change.

Copy link
Member

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 which pip 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.

Copy link
Contributor Author

@fcharras fcharras Dec 14, 2023

Choose a reason for hiding this comment

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

build isolation
For building packages using this interface, pip uses an isolated environment. That is, pip will install build-time Python dependencies in a temporary directory which will be added to sys.path for the build commands. This ensures that build requirements are handled independently of the user’s runtime environment.

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.

"what else would I want pip to do??"

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.

@lesteve
Copy link
Member

lesteve commented Dec 14, 2023

Preliminary investigation done with @lesteve suggests that this flag causes pytest bugs on windows (see #27806 ).

After further debugging, I am not that sure anymore about this, I can reproduce the original issue #27806 with and without --no-use-pep517, contrary to what we observed together ...

@fcharras fcharras marked this pull request as draft December 14, 2023 11:04
@fcharras
Copy link
Contributor Author

I confirm however that in my environment (windows 11 VM, miniforge prompt, mamba, separate environments, etc) not passing --no-use-pep517 makes the issue disappear.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

both those flags are required if we don't want contributors to wait for a complete build everytime they run the command. Otherwise their workflow will be very slow.

If we were comfortable recommending people to use main in then we'd be in an easier place.

During springs, almost always we end up having to install WSL for contributors on windows. So building under windows should be considered advanced and we can have windows specific commands in the advanced installation section. But by default we shouldn't change the flags which makes everybody else's workflow significantly slower.

I'm -1 on this.

@lesteve
Copy link
Member

lesteve commented Dec 14, 2023

FYI I opened #27806 which should fix the weird pytest issue on Windows.

@fcharras
Copy link
Contributor Author

fcharras commented Dec 15, 2023

Let's close this. I see the need of a practical guide for sprints with beginners. I hope it does not slow the adoption of good environment management skills. Anyway I don't see a way out to improve that right now.

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.

5 participants