-
Notifications
You must be signed in to change notification settings - Fork 22
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
rip does not follow spec for handling of pre-releases #118
Comments
FYI, the discussion on the thread questions the behavior I have described for Pip: https://discuss.python.org/t/handling-of-pre-releases-when-backtracking/40505/4 I will try and reproduce, but I will do this seperarelty from rip and file a Pip bug if I find the behavior does not match the comment I linked in all cases. |
Yes we should fix this definitely we just have not come around to it yet! |
Some updates on the Python discussion board: https://discuss.python.org/t/handling-of-pre-releases-when-backtracking/40505/10 It's clear to me now that what Pip does here "selects a pre-release if all releases are pre-releases" and what the spec says are different. @wolfv if any RIP maintainers would like to provide any feedback there on any changes they would like to see here (Pip documentation on what they do, update spec to Pip or something different, change in Pip behavior to the spec, etc.) I'm sure input would be appreciated. |
Thanks @notatallshaw for pointing this out. To be honest, what The |
Sounds good to me, and sounds like the spec should be updated. Although, when I get a chance, I will check what Poetry does. I would still strongly prefer there be an option to exclude all pre-releases. I would really like to not be surprised in prod that a pre-release slipped in. But maybe this use case is too niche. |
We have that as an option in the /// Defines how to pre-releases are handled during package resolution.
#[derive(Default, Debug, Clone, Copy, Eq, PartialOrd, PartialEq)]
pub enum PreReleaseResolution {
/// Don't allow pre-releases to be selected during resolution
Disallow,
/// Allow pre-releases to be selected during resolution but only if there are no other versions
/// available (default)
#[default]
AllowIfNoOtherVersions,
/// Allow pre-releases to be selected during resolution
Allow,
} It might be the better default ... although, as you noted, will prevent some packages from resolving. |
Hey @notatallshaw ! Thanks for pitching and for the link! I would also be very interested in what Poetry does! |
As best as I can tell, Poetry appears to follow the spec for direct dependencies, but struggles when resolving transative dependencies: https://discuss.python.org/t/handling-of-pre-releases-when-backtracking/40505/11 |
So if we want to follow pip's rules, which rules should we follow? I'm getting a bit lost in the details here. I initially wrote down: #4 but not sure where I got that idea from anymore. Then there's @pradyun comment:
Which seem quite reasonable. But @notatallshaw you've also noted in: https://discuss.python.org/t/handling-of-pre-releases-when-backtracking/40505/11 that pip either does or does not use pre-releases when backtracking? Which kind-of comes back to #4. But also does not make a whole lot of sense to me. Although poetry does not consider transitive pre-release dependencies? Maybe @wolfv @notatallshaw could clearly lay out the rules that pip follows in this thread or in the linked one so that we have a written down reference to follow, instead of just code :) |
I would like to follow what Pradyun laid out.
Although there will be some trickery involved in implementing the first rule while also implementing the second rule, so let's see (since we cannot just filter out the pre-releases anymore). |
So what we discussed what we could do is to change the Note that the the let pre = pep440_rs::Version::from_str("1.0.0a1").unwrap();
let set = pep440_rs::VersionSpecifier::from_str(">=0.9").unwrap();
assert!(set.contains(&pre)); // assertion will pass So take care when implementing this :) E.g if you have 2 packages
If we first process If you have a lot of problems with this you can always implement |
@notatallshaw The final point of unclarity for me remains in the comment in the python discourse thread w.r.t backtracking and transitive dependencies, including the poetry comment. Hoping you can clarify :)! So in my head there are two cases:
But I suppose in both cases you should just look at which candidates would match the That is if the direct dependency However, if I read your posts correctly it seems that:
Am I correct in my reading of this? |
I'm a bit confused by the different questions, let me outline the scenario I was thinking about when I originally started that thread:
I thought this case was special for Pip when I opened the thread because I thought Pip followed the spec. But now I think (but haven't tested) that this case is not special for Pip because for Pip this is no different than normal, Pip see's C as having non-prerelease versions and none of the requirements involve a pre-release specifier so Pip will not choose I think Poetry also doesn't choose I still don't think Pip is consistent in how it handles this, the real world motivation for me initially thinking about this was a user reported an issue to Pip that they were gets a Finally, and I think your questions are getting at this (?), I have not even thought about the scenario where one or both of the specifiers include a pre-release:
I would suspect Pip's behavior is the same as to whether it is a transitive dependency or not and depends on the order it is checking the specifiers.
And which specifier operators are being used:
Off to report more issues to the Pip tracker. |
Wow, I would expect that in both the cases Also, the second example with the specifier caught me off-guard as well. |
I thought both of them should have a solution, but either way I would prefer consistency.
Some further discussion on Pip's state of affairs on one of the issues I raised: pypa/pip#12471 (comment) IMO Pip's situation is the following:
But, as per the discussion I link to, this is all likely because the logic was implemented before there was a spec, and there hasn't been a significant interest in working on this aspect of requirement resolution. Fow now I think RIP is going to need to make it's own choices, document it's own intended behavior, and take a pragmatical approach of what works in the Python ecosystem. |
Pip maintainer here - I disagree with this explanation. IMO, pip isn't following the spec, but it should be. That's a bug in pip. That's all. It's not a bug we'll be able to fix quickly (so if rip follows the spec, it will behave differently than pip in some edge cases, probably for some time) but it is "just" a bug. Talk of pip's "intended behaviour" basically comes down to a couple of pip maintainers (one of whom was me) having mentioned what they thought pip did as part of a discussion. It was a statement of what we think the code does, not a claim that it's intentional (much less that it's intentionally different to the spec!) |
Apologies, it seems I misinterpreted some statements.
I have been confused on this point over the course of this conversation, because of you saying that the spec says SHOULD and MAY that Pip could be interpreted as not violating the spec (which, in some sense, I am taking to have an equivelence as "following the spec"). I think this, with an outlining of behavior of what the code in Pip does, and the highlighting that Pip may have implemented this behavior before there was a spec, is what led to my mistaken assumption of Pip's "intended" behavior. |
Nice to see a fruitful discussion coming out of this. Also both @notatallshaw and @pfmoore, thank you for all the hard work you are doing, I've really learned a lot about the pypa/pypi packaging space these last month, and all of your help has been invaluable. 🙏 😃 I think for now we will be going with an implementation that is close to pip, and disregards some of the spec, of course we would rather follow it, but I have the feeling it might change in the coming time. @wolfv is working on an implementation already: #141 |
Currently rip does not accept a pre-release if there are no other packages, but the documentation states the follow: https://packaging.python.org/en/latest/specifications/version-specifiers/#handling-of-pre-releases
But rip excludes all pre-releases even if nothing else is available:
e.g.
cargo r -- opentelemetry-exporter-prometheus
rusults in:In my opinion there should be an option to force exclude pre release packages and keep this behavior, but this does come up in the Python ecosystem naturally: #74 (comment)
By the way, my understanding is Pip's behavior is to do this when collecting packages, but not when backtracking transitive dependencies, I have a question on the Python board if this is really following the spec: https://discuss.python.org/t/handeling-of-pre-releases-when-backtracking/40505
The text was updated successfully, but these errors were encountered: