Implement is_unsatisfiable on SpecifierSet using ranges#1119
Implement is_unsatisfiable on SpecifierSet using ranges#1119notatallshaw merged 22 commits intopypa:mainfrom
is_unsatisfiable on SpecifierSet using ranges#1119Conversation
5df83c0 to
8aabbca
Compare
b67de8d to
7991a09
Compare
|
I think this is ready for review, this attempts to be correct and implemented in an intuitive and as simple as possible way. This does not intended to be performant, while not slow, there are a lot of opportunities for performance, but usually at the cost of adding complexity or making less intuitive . |
4b067f1 to
a810998
Compare
a810998 to
5f72426
Compare
c7f43ce to
aa4b0ee
Compare
This comment was marked as outdated.
This comment was marked as outdated.
aa4b0ee to
95694d5
Compare
aad8702 to
37c6e22
Compare
|
Okay, this is ready for review/approval again, apologies for having to make it draft after the last approval, but I've done the following:
|
37c6e22 to
5c11f46
Compare
5c11f46 to
94fa3b4
Compare
is_unsatisfiable on SpecifierSet using intervalsis_unsatisfiable on SpecifierSet using ranges
henryiii
left a comment
There was a problem hiding this comment.
Not at a computer, I'll try to do a final review tonight. Triggered copilot just to see what it says.
There was a problem hiding this comment.
Pull request overview
This pull request adds a new SpecifierSet.is_unsatisfiable() API implemented via interval/range reasoning (an internal re-implementation of key PEP 440 ordering/containment rules), aimed at letting resolvers quickly detect contradictions without enumerating candidate versions.
Changes:
- Implement interval-based range conversion for
Specifierand range intersection logic forSpecifierSet, exposingSpecifierSet.is_unsatisfiable()with caching. - Add extensive unit tests for satisfiable/unsatisfiable specifier sets (including prerelease-mode variants) plus basic cache-behavior tests.
- Add new Hypothesis property tests asserting soundness/monotonicity properties of
is_unsatisfiable().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/packaging/specifiers.py |
Adds internal boundary/range types, Specifier._to_ranges(), and SpecifierSet.is_unsatisfiable() based on intersected ranges + special casing for === and prerelease mode. |
tests/test_specifiers.py |
Adds a large suite of concrete satisfiable/unsatisfiable cases (including prereleases=False scenarios) and caching assertions. |
tests/property/test_specifier_extended.py |
Adds property-based tests for is_unsatisfiable() soundness and intersection monotonicity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return self.version == other.version and self._kind == other._kind | ||
| return NotImplemented | ||
|
|
||
| def __lt__(self, other: object) -> bool: |
There was a problem hiding this comment.
Not that important for an internal class, but __lt__ doesn't need to be object like __eq__ does, it can be more specific and then type checkers and IDEs can flag invalid comparisons. I'm guessing this is supposed to only support _BoundaryVersion | Version, given self._is_family(other), for example?
There was a problem hiding this comment.
What about the ones below?
There was a problem hiding this comment.
Ah yeah, also fixed.
There was a problem hiding this comment.
If this was supposed to stay, I'd probably recommend putting all the new infrastructure into a new private file. But if a lot of it is going away in the next version, keeping it here is fine.
There was a problem hiding this comment.
Agreed this needs cleaning up, I'm committing to clean up all these over the next couple of PRs that will add internal usages and then a public API.
8e9d514 to
37dfc96
Compare
Fixes #940
Fixes #306
This uses intervals that re-implement PEP 440 logic, the plan is to move the entire specifier machinery to use intervals, but that will be in a follow up PR. This approach focuses on relative simplicity, given that choice, and is not micro-optimized.
There are some edge cases around
===that need to still be decided on, as this PR implements something closer to the specification than packaging quite implements (see #978 and #766), I will create a follow up PR to make that choice directly.PR still needs a little clean up, so I will leave in draft for now and comment when ready for review.