-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Toward PEP 518 #4802
Comments
#4799 is where a lot of the debate is happening. It was mostly prompted by:
As I dug into trying to work out what @xoviat was saying in #4799, it became obvious that we had some issues around recursive building of build environments (X needs Y to build, and Y needs Z, ...) although I'm still unclear as to whether these are implementation bugs, deeper design problems, or nasty corner cases that we can defer without too much issue. Personally, I'm at the point where I'm out of my depth. I don't understand @xoviat's implementation to judge if it's needed, or if we still just need to merge #4764 and fix #4647 to be ready to go. Nor do I know how easy it'll be to fix #4647 (@xoviat seems to be saying that we need to merge #4799 to fix #4647, but that brings its own problems). I've run out of time and energy to take the discussion any further this weekend, so I'm going to drop out at this point (at least for a while). For me, the key point is that we want an acceptable level of PEP 518 support for pip 10. I'd like someone to give me a feel for whether we're nearly there, or whether we're weeks away, so that I can avoid getting people fired up that pip 10 is coming only to then say it won't be till the new year... |
Sure thing. |
@pradyunsg I know you don't have time for this. But you have been more successful at getting PRs approved than I have, so if you want, I'll be more than happy to walk you through the current implementation, how it works, and the potential problems that it has. I'll explain how I solved some (but not all) of these problems and my ideas for a complete fix (which again, I may do after PEP 517 if it's not done). I honestly don't care who does the work as long as it's done. |
Actually, you're AWOL, so let me write a summary:
I'll define the "scope" of an object as a sort of lifetime. It's the duration that an object exists. Right now, the scope of PEP 518 in So what's the problem with that? The problem is that the scope of the PEP 518 environment needs to be equal or greater than the scope of all calls to The first obvious decision that you will encounter is: what should have a reference to The next problem you may encounter is this: how do we install the Another problem is what shell call should we make? It's actually trickier than you might think, because getting the command-line parameters is frankly a PITA where you need to make that call. So you might have trouble passing the original parameters that the user passed to the child. The solution used by the original implementer involved using the Shelling out a child of yourself without some kind of manager class that can kill children when the user presses ctrl+C isn't just wrong, it's malicious, especially when you don't know how many processes you've spawned. I personally don't know whether the children die in the current implementation (this might be FUD), but if they don't, the current implementation IMHO is wrong (aside from the other concerns). Some possible solutions to that issue are the following:
|
@xoviat While it may not cover the case of deliberate malice, am I right in thinking that a build cache (which was used even when
|
Using the disk to avoid having to figure out OOP design problems is not a bad idea. That's like an intermediate option between implementing PEP 518 completely correctly and just hacking something together. |
It would also interact nicely with containerised environments and chroots in general, since we'd be able to use operating system level tools to keep different Python level builds isolated from each other, so pip would just need to figure out how to ensure its own subprocesses cooperate with each other. |
@xoviat thanks for the summary above. As I'd said, I'd reached the limit of my understanding of the code in this area, and your explanation has helped me enormously. |
I had not actually looked at #4144's code before. I just did and I really don't want that to be shipping.
Honestly, I think it comes down to this. Implementing PEP 518 completely and properly is a task which would/might delay pip 10 a (fair) bit if we go that way. I think a safe-ish middle ground here would be to require build-dependencies to be available as wheels. That way, we can avoid the recursion problem since the build-dependencies (and all of their dependencies) would not need to be built via this process. How does this sound? |
It's restricted but I think a restricted first implementation is better than a you-can-shoot-yourself-in-the-foot-if-you-are-not-careful first implementation. |
Thanks for confirming that - that was my fear. However, now I am confused, as I thought that at least partial PEP 518 was already in master. Specifically, as I understand it #4647 demonstrates a bug in the PEP 518 support in master - so clearly we have something already, as whatever it is isn't correct... So we have to do something, and it seems the options are:
As you say (3) means a long delay before pip 10 and we have other fixes that I'd really like to see released (Unicode fixes being one we're regularly getting issues over). So I'm not keen on that. You didn't mention (1), and I'm not sure whether that's because you thought we had no PEP 518 support in place, or whether you assumed backing it out wasn't an option. Personally, I don't like the idea - it's a backward step, and it sends a pretty negative message about the PEP itself, if it's that hard to implement correctly. But I think we should be explicit about rejecting it. Your proposal for (2), that we ship a version of PEP 518 that only supports wheels as build-dependencies (we'd still need to fix #4647, as the demonstration of that uses a build dependency that is a wheel) seems reasonable, in the sense that it's practical for us to implement. My main reservation is that I've no idea how problematic that restriction would be for people who want to use PEP 518. So I guess I feel like we're stuck whatever we do, but partial support covering only wheels as build dependencies is the best option out of a bad lot :-( The PEP authors (in #4799 (comment) and #4799 (comment) in response to my question in #4799 (comment)) were pretty definite that full PEP support required building any build dependencies, though, so it has to be only a stopgap. But it's not me that will be implementing this, so I'm happy to go with the judgement of whoever does. One thing I have done is create #4803 and mark it as a release blocker, as a reminder that we should document how we deviate from the spec, if we need to. (And off-topic for this issue, but can I suggest that we are careful not to make the same mistakes when we start implementing PEP 517? Let's make sure we understand all of the implementation implications before we get too deep into coding - my instinct is that PEP 517 is going to be an even more complex design problem than PEP 518...) |
I'm most familiar with distros when it comes to the "build everything from source" perspective, and we definitely separate the process of "bootstrapping the buildroot" from that of regular package builds. Full auto-bootstrapping from source is hard, since you end up having to do things like bootstrap your C compiler. So for pip, I think it's reasonable to say that build dependencies will always be installable from wheel files. The refinement you can introduce post 10.x is to have a build cache that's distinct from the regular wheel cache, such that users of the cache can be sure that all the wheels in that were built in a controlled environment, rather than downloaded from PyPI or another index server. |
I don't necessarily agree with that. It's hard to implement correctly for pip. But pip as stated in the PEP is one of the only front ends that people are going to use. I think it's our job as language implementers, and pip really is a language that people use to package their python projects, to make it as simple as possible to create a build system without having to think about all of these difficult issues. For them it should just work seamlessly because we've done the hard work. |
Actually that's exactly what #4799 does. If you want I can restore that branch and then you can fork it and submit it as a PR. |
Two things on the implementation side of things of (2) still stand though -- as @xoviat had pointed out above:
I'm not sure who would have the time to do these though.
It probably is not hard to implement correctly. It's just that with the way pip's codebase is today, it's non-trivial to implement in pip -- there's stuff that happens in weird places and I think if that is cleaned up, it would be fairly trivial.
I assumed backing out isn't an option. Now that I think about it -- how important is it to ship PEP 518 in pip 10? I feel if it can be deferred to the next major release, that would (other than being the easy way out of this situation) be straightforward and both 517 + 518 could land in one big release. This feels clean enough that I won't be the one saying this is not the way to go. @dstufft @xavfernandez thoughts? @ncoghlan's idea of a build-cache sounds like a good idea to me although I'm not sure I understand all the implications of it.
I probably won't have the time and even if I do I might not be reusing any existing commits. But restoring that branch can't hurt. :) |
We've had this exact discussion except that you probably didn't even know it. This situation is situation X. Calling egg_info before the build environment is set up is situation Y (#4799). |
I guess that means #4799 is back on the table then? As long as it passes all of the tests and does what it claims to do? |
Aargh, those X's and Y's come back to haunt me again 😉 Yes, I'm saying I don't have a feel for the relative likelihoods of the 2 cases. I understand you're saying that build requirements that aren't wheels are rare enough that we're OK to go with ignoring that case. So were one "it's OK" and one "don't know" vote between us, basically. I'm not trying to block this option, just saying where the limits of my intuition lie, is all. |
@xoviat I have a few of questions. It'd be awesome if you would answer them before you make a new PR. :)
|
I've worked with a lot of the scientific projects so it could be that I'm biased. But I can rattle off a list of projects with wheel build dependencies and I genuinely cannot think of one project with source dependencies. Maybe I'm wrong. @rgommers would you be okay with PEP 518 only supporting build dependencies available as wheels if it was in the next pip? |
Subprocess gets the requirements list exactly as specified. That way it goes through the resolver.
Yes that's in fact why the test was xfailed. The build dependency in the test is not a wheel. |
What's our feel for whether we'll get anyone with the time to do PEPs 517 and 518 for pip 11? I'm not optimistic. It seems to me that they are both big chunks of work, and we also have the resolver work ongoing as well. While I'm in not favour of holding up pip 10 longer than necessary, I'm equally uncomfortable with letting all of our major feature plans drift while we release a series of essentially minor releases. To put it another way, saying "let's go for a pip 10 release" prompted a resurgence of activity on the PEP 518 work. If we remove that from pip 10, I for one will focus on getting things ready for the release, and I suspect it's likely that PEP 518 loses momentum again. What's to kick things into activity again? @xoviat's been working on implementation, but he's had problems trying to get any of the rest of us to understand the issues he's been struggling with till now. I don't want to leave him working with no feedback again. What we could do is release a "pip 9.1" with just the incremental fixes that we have ready, and reserve the version number "pip 10" for implementation of (at least one of) the 3 big ticket features that are in the pipeline. But if we do that, I'd like to try[1] to commit to a pip 10 release in the first quarter of 2018. I'd be OK with that as an approach. But does anyone have a feel for what would be involved in backing out the partial support we currently have in master? Or in documenting what we have and what its limitations are (so that people don't try to use it assuming it's complete, hit bugs and raise issues that we have to respond to with "this feature's not yet complete, sorry but wait for pip 10")? Are we just exchanging one big chunk of work for a different one? [1] To the extent that we can commit to anything with extremely limited volunteer resource as all we have available. |
Thanks, it's hard sometimes to know people's backgrounds. If you're familiar with the scientific projects, that alleviates my concerns a lot. |
I really like this. +1 to a pip 9.1.0 instead of pip 10.0.0
I'd had a very interesting brainwave -- pip turns 10 years old on 12 Oct 2018. That would be the perfect date to do a pip 10.0.0 release. It's a completely different timeline. I am not saying that we should delay the white-whale features until then but some part of me really wants this version number and age thing to coincide too.
I'll do what I can to make sure it doesn't. Hopefully @xoviat is willing to as well. :)
I won't mind taking a look at this tomorrow. Since @dstufft was the one reviewed on #4144, I think his input on this would valuable. |
Note - I wouldn't want to do anything as drastic as backing things out without agreement from @dstufft and @xavfernandez - so let's see what they have to say, too. |
@dstufft doesn't have enough time in the day. He also has to make sure that warehouse doesn't go down. |
Yes please. :) |
Is there anyone that can summarise where we stand with PEP 517 and PEP 518 in relation to a pip 10 release? Specifically:
My feeling is that we're waiting on the release blockers, but we're not close enough to PEP 517/518 support to block pip 10 on them. But I don't think anyone is working on #4647 except as part of implementing PEP 518. One alternative option would be to document the limitations of current PEP 518 support, and downgrade #4647 from being a release blocker. I don't know enough about the use cases to know if that's viable. |
I only just saw this discussion - apologies for the half-baked initial implementation that's liable to act as a fork bomb, and thanks to all of you who've taken the time to understand it and come up with better ideas. FWIW, I think restricting it to installing wheels for build requirements would be an acceptable compromise for the first version. This also reminds me that I should probably fix matters so that flit is not a build requirement of itself. |
No apology necessary. As I said you cannot predict these issues on the first try. Even knowing what we know now, delaying the PR would have just deferred these discussions. |
IIUC, it can fork-bomb a system as of now; is that correct? If so, I think it's not.
I think, the easy (short term) solution would be to just restrict to wheels for build dependencies. I'll try to take a stab at it sometime next week. If that doesn't materialise, I'll be fine if we remove the current PEP 518 support from master and cut a 10.0.0 from that.
I think you mean, complete implementation of PEP 518 with source build dependencies allowed? |
I think that restricting it to wheels should be as simple as changing this line: pip/src/pip/_internal/wheel.py Line 696 in fc6b2c1
to: finder.format_control = FormatControl(set(), set([':all:'])) The second field there is a set of packages to find only as binaries, with a special case for |
Yes. But that alone wouldn’t address #4647. Also, none of this code goes
through a resolver.
…On Sat, 2 Dec 2017 at 01:23 Thomas Kluyver ***@***.***> wrote:
I think that restricting it to wheels should be as simple as changing this
line:
https://github.com/pypa/pip/blob/fc6b2c192088737f81259b6446f627f20ce46443/src/pip/_internal/wheel.py#L696
to:
finder.format_control = FormatControl(set(), set([':all:']))
The second field there is a set of packages to find only as binaries, with
a special case for ':all:' to mean all packages.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4802 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADH7SUi0QMS3rr5Iba90XWZmFweGmqeBks5s8FlEgaJpZM4QBdSg>
.
|
Right, there's a bunch of other stuff wrong with it. But that should prevent it from fork-bombing, which I think is the most pressing concern. |
If someone wants to take over my original PR ("fix the PEP 518 problems"),
it shouldn't be to difficult to alter it so that build isolation isn't
enabled without a pyproject.toml. The original reason that it wasn't merged
was that it dropped support for installing dependencies from source with
PEP 518. However, now that folks are beginning to realize that PEP 518 may
not be in pip 10 at all, they may be more amenable to accepting that PR. I
personally don't have time to champion it, but that shouldn't stop others
from taking it forward, as only a few lines of changes will be requires
(aside from xfailing the PEP 518 test).
|
Actually, against my better judgement, I am willing to implement both PEP 517 and 518 shortly if the pip developers will agree with my conditions:
|
I have no issues with 1; I have no preference either way for 2.
…On Sun, 3 Dec 2017 at 02:36 xoviat ***@***.***> wrote:
Actually, against my better judgement, I am willing to implement both PEP
517 and 518 shortly if the pip developers will agree with my conditions:
1. Dependencies will be only from wheels initially
2. pip will have an internal build backend initially, even though it
will eventually be removed
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4802 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADH7ST6riptZkYMap5Z5SstRf-VmE7eAks5s8bu5gaJpZM4QBdSg>
.
|
FYI the conditions are not arbitrary but are there to make initial implementation possible. Is @pfmoore okay with this? |
I'm, not particularly comfortable with the tone of the offer "against my better judgement ... agree with my conditions". I'm not going to object if the other pip developers are happy to take up this offer, but personally I don't have the time at the moment to restart the whole debate on implementation details. Basically, I'll defer to @dstufft and @xavfernandez on this (@pradyunsg has already given his views). |
The tone of the offer is the way that it is because methods of implementation were closed off due to fundamental disagreements about what an initial implementation would look like. I'd rather agree on the principles now than devolve into another implementation debate. |
I'll just state that I am also not super comfortable with the tone, it's
just, I don't have the time or energy to go into why that tone was used
etc. Same reason for the terse reply from my end.
Maybe also worth stating, I'm cool with binary only *build* dependencies in
the first implementation, not for a longer term. This doesn't apply to
runtime dependencies (since that has also somehow come up in another
discussion).
…On Sun, 3 Dec 2017, 23:37 xoviat, ***@***.***> wrote:
The tone of the offer is the way that it is because methods of
implementation were closed off due to fundamental disagreements about what
an initial implementation would look like. I'd rather agree on the
principles now than devolve into another implementation debate.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4802 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADH7ScUh-BveonoTxZ5FkkeSynFvoLb8ks5s8uNRgaJpZM4QBdSg>
.
|
Honestly, it's probably not an effective use of anyone's time to discuss the tone of the post further, as it's not relevant to including these features in pip 10. However, I need an assurance that my conditions are acceptable to merge from either @pfmoore (who has indicated that he cannot make such an assurance, which is acceptable given that no one is paid for their time here), @dstufft, or @xavfernandez. Again, the conditions are not my personal opinion, but are implementation-driven. If I relax these conditions, then I cannot promise an implementation, so there's no point in spending time on preparing a PR, and then people reading the diff and asking "oh, why is this line here?" and then "oh, so this isn't mergable?" because there was a miscommunication about what exactly the purpose of the PR was. |
Agreed re the tone. My point is simply that I won't be merging the change[1], so my view is not really that important here. [1] Obviously, I'm saying that without having seen the implementation, so I hope it's clear that my reasons are not to do with concerns about the code quality - it's about not having the time to ensure I understand the code well enough to be willing to merge, basically. |
@xoviat What are your implementation plans with respect to the iterative vs. recursive approach we discussed above? Also, to clarify, are you saying that you will first be completing a partial implementation of PEP 517 and 518 that can be merged, followed by a full implementation that can be merged? Or are you saying that you will only be doing the partial implementation, or are you saying that you will be doing a full implementation proceeding through earlier stages that won't necessarily be mergeable? (I'm partly trying to get a better sense of what you mean by "initially" and "eventually" in your comment.) |
The first condition eliminates the entire recursion issue.
What I am saying is that I will be completing a partial implementation this will work for 95% of use cases; specifically in the case where dependencies have wheels (very common now) and you're on a manylinux/windows/OSX platform (vast majority of users). It's not a full implementation. But the way you get un-mergable PRs is trying to do the "all or nothing" approach where you either comply with the standard or you don't. Keep in mind that a full implementation will require sorting through some fairly nasty issues that will each need a separate PR (as having a PR with 100+ comments usually means that the code isn't well-reviewed). [1] |
I'll close this issue now -- we have preliminary support for PEP 518 in pip 10 which only supports wheels as build dependencies. I'll open a new issue for discussing about complete support and another for PEP 517 support (quoting from here as relevant). Thanks @ncoghlan @rgommers @cjerdonek for your insights and help here. Thanks @takluyver for the initial implementation of PEP 518. Thanks @xoviat (who's @ghost now) for all the help with implementing these changes. Thanks @benoit-pierre for your help with improving the current support. PS: 100th comment! 🎉 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I'm AWOL this weekend but as I understand it, there needs to be a discussion toward PEP 518 and implementation of it in pip.
I've opened this issue, because I couldn't locate where the discussion was happening, if it is. Plus, having it in one place that's not pypa-dev/distutils-sig would be nice-ish?
The text was updated successfully, but these errors were encountered: