Make > operator exclude post and local releases #2471
Conversation
charliermarsh
force-pushed
the
charlie/max-version
branch
from
135ed18
to
532935d
Compare
crates/pep440-rs/src/version.rs
Outdated
| let post = if version.is_max() { | ||
| // But don't convert a `dev` version to a `post` version. `1.0.dev0` should _not_ be greater | ||
| // than `1.0.post0.dev0`, since the latter means "a dev release of a post release of 1.0", | ||
| // not "a post release of a dev release of 1.0". |
There was a problem hiding this comment.
Mildly concerned with how all of this intersects with post-releases, but I did add tests.
There was a problem hiding this comment.
I ended up removing dev support, we never create a version with both max and dev so not sure it's worth accommodating it here.
There was a problem hiding this comment.
I added debug asserts to the constructors for min and max.
charliermarsh
force-pushed
the
charlie/max-version
branch
from
532935d
to
78abd28
Compare
| ] | ||
| expected["explanation"] = ( | ||
| "We do not have correct behavior for local version identifiers yet" | ||
| ) |
There was a problem hiding this comment.
This is the payoff, we can remove a bunch of incorrectly-succeeding scenarios.
|
This is related to #2430, but not really relevant for the core use-case of local versions (PyTorch) -- it's just a correctness thing. I do think we would've received a bug report that |
charliermarsh
force-pushed
the
charlie/max-version
branch
2 times, most recently
from
2858de8
to
4818abf
Compare
| Some(u64::MAX) | ||
| } else { | ||
| version.post() | ||
| }; |
There was a problem hiding this comment.
We could also consider incorporating max into the match below... This felt easier to get right.
| ╰─▶ Because only package-a==1.0.0 is available and package-a==1.0.0 depends on package-b>2.0.0, we can conclude that all versions of package-a depend on package-b>2.0.0. | ||
| And because you require package-a and you require package-b==2.0.0+foo, we can conclude that the requirements are unsatisfiable. |
There was a problem hiding this comment.
Not for here, but I wonder if we should add a "hint" explaining why package-b==2.0.0+foo does not satisfy package-b>2.0.0
| Installed 1 package in [TIME] | ||
| + package-a==1.2.3.post1 | ||
| × No solution found when resolving dependencies: | ||
| ╰─▶ Because only package-a<=1.2.3 is available and you require package-a>1.2.3, we can conclude that the requirements are unsatisfiable. |
There was a problem hiding this comment.
Similarly, I wonder if we should add a hint that 1.2.3.post1 is available. I think it's probably unnecessary here since post releases feel equivalent to the version they follow and are generally transparent to users?
There was a problem hiding this comment.
Makes sense to me!
It might be nice to have scenarios for:
- The user requests a post version but there are no post versions available
- The user requests a greater than post version the excludes all available post versions
To ensure we display these correctly in our unsat messages
There was a problem hiding this comment.
This LGTM, but I think you might want to bump the cache version because of the representational change to Version. Actually, I kind of wonder if you maybe don't need to do it for this change, since I don't believe any existing versions have their meaning changed or become invalidated. Unlike the min change, this change doesn't impact the existing suffix tags. Unless we're 100% sure, we should bump it, because if we're wrong it can cause subtle and implicit bugs.
charliermarsh
force-pushed
the
charlie/max-version
branch
from
4818abf
to
3e59e02
Compare
|
Bumped cache! |
charliermarsh
force-pushed
the
charlie/max-version
branch
from
3e59e02
to
27b2263
Compare
Summary
This PR attempts to use a similar trick to that we added in #1878, but for post-releases.
In #1878, we added a fake "minimum" version to enable us to treat
< 1.0.0as excluding pre-releases of 1.0.0.Today, on
main, we accept post-releases and local versions in> 1.0.0. But per PEP 440, that should exclude post-releases and local versions, unless the specifier is itself a pre-release, in which case, pre-releases are allowed (e.g.,> 1.0.0.post0should allow> 1.0.0.post1).To support this, we add a fake "maximum" version that's greater than all the post and local releases for a given version. This leverages our last remaining free bit in the compact representation.