Skip to content

DOC update documentation on re-building Cython extensions #26334

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

marenwestermann
Copy link
Member

Reference Issues/PRs

towards #25985

What does this implement/fix? Explain your changes.

Updates the documentation on re-building Cython extensions to provide better guidance for new contributors. I also updated the pip install -v --no-build-isolation -e . command to pip install -v --no-use-pep517 --no-build-isolation -e . in the documentation to prevent potential errors during the installation process.

Any other comments?

@betatim
Copy link
Member

betatim commented May 9, 2023

Thanks for figuring this out. While reading the diff I realised that I have no idea what this PEP517 is about, and why I might want to (not) use it. Is there a short explanation of what the PEP does? (I've read the abstract of the PEP but left none the wiser) Similarly, why do we need this new flag to pip? Is it because newer versions of pip behave differently from older ones? Asked differently: why do I not seem to need this flag?

Do we need to add the -v flag to the commands where it wasn't already present? I don't know what the rationale is for some of the commands having it and some not, but for me pip already prints "two or three walls of text", which results in me mostly not reading what it tells me (I know ...). Which makes me wonder if turning on verbose mode helps people who are trying to debug something or if it adds yet another wall of text?

The pedant in me would prefer if we spelled out all the command line flags (so -e -> --editable). But some of the commands are becoming pretty long (see above for "wall of text"). What is your thinking?

And finally, some times we explain what --no-build-isolation does, but we don't explain any of the other flags. Back when it was basically the only flag it made sense, but now maybe we should just not explain any of them? Or all of them?

@adrinjalali
Copy link
Member

adrinjalali commented May 9, 2023

I have no idea what pep517 does, and I don't really care (it's given me headaches). But w/o it, the --no-build-isolation basically has no effect in build time and the command builds the whole package from scratch, which is not what we want. We want it to build only changed modules. I personally use make in instead, some people use python setup.py develop instead, but I guess we want to recommend the pip command here.

The -v makes it show what's being compiled, w/o it, it's just a very slow command that you don't know what it's doing. So I'm a strong +1 for both these changes.

I don't mind -e or --editable, I guess the diff here is just to make the consistent. We can also merge this PR and then figure out if we want to explain what pep517 does in a separate PR. But that doesn't really have an effect on our contributors. Right now, our docs don't give the right tools to first time contributors.

Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

I agree that this is an immediate and welcome improvement to an unfortunately common headache. I would also like to see in separate PRs:

  1. A brief explanation of PEP 517 -- perhaps as a note or an aside.
  2. Making the commands uniform (e.g. sometimes we use -e and sometimes --editable, as well as different orders of flags).

Personally I think using the shortened -e is preferable since readers can easily look up the specific flag if they are curious about it, and it meaningfully shortens these already-long commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants