Skip to content
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

Fix for handling of version ranges #645

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dimbleby
Copy link
Contributor

don't include first prerelease in the allowed max for an exclusive range.

Fixes python-poetry/poetry#8475, python-poetry/poetry#8405, python-poetry/poetry#8202

(actually 8202 seems to have been resolved by changes in the available dependencies on pypi and so I can't reproduce it now: but I'm pretty sure these will all be the same)

Absurdly I can't find a simple testcase that takes two versions or version ranges and compares or intersects them or something to demonstrate the point. So I confess that I don't fully understand what was going wrong before this fix.

However I do understand that those issues are

  • something to do with wildcard constraints (eg =2.*) which poetry interprets as having a lower bound that is the first dev-release (eg 2.dev0)
  • and something to do with how they interact with regular but adjacent constraints like <2

so I've made this fix somewhat on gut: <2 certainly shouldn't allow 2.dev0 and so it's weird that allowed_max would return that value.

What can I say? I'm confused, but this looks like a good fix and it does resolve those issues...

don't include first prerelease in the allowed max for an exclusive
range.
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dimbleby
Copy link
Contributor Author

dimbleby commented Oct 1, 2023

I now think that the real problem - or anyway among the real problems - is ambiguity in poetry-core's treatment of the max in a strict inequality range.

  • when dealing with a parsed value such as <2 the max is 2 but the allowed_max should be 2.dev0, because pre-releases are excluded by <
  • however when dealing with a value calculated by operations between other versions or ranges, the max should be considered to be correct as it is, not to be revised by allowed_max
    • eg consider (>1,<3).difference(2). This should come out as >1,<2 || >2,<3: except that in the first part of that prereleases should be allowed and so subsequent calculations that treat this as though it were >1,<2.dev0 are wrong

A possible approach is to handle this at parsing and abandon allowed_max altogether. ie parse_single_constraint("<2") should construct the object with max=2.dev0 all along. Then never use allowed_max at all.

This also solves those issues reported in poetry - but causes carnage in the test scripts: the main (but probably not only) problem being that parsing and then printing <2 gives <2.dev0 - which should indeed be equivalent, but is not so pretty...

@dimbleby dimbleby marked this pull request as draft October 2, 2023 08:41
@radoering
Copy link
Member

A possible approach is to handle this at parsing and abandon allowed_max altogether. ie parse_single_constraint("<2") should construct the object with max=2.dev0 all along. Then never use allowed_max at all.

This also solves those issues reported in poetry - but causes carnage in the test scripts: the main (but probably not only) problem being that parsing and then printing <2 gives <2.dev0 - which should indeed be equivalent, but is not so pretty...

If we went down this path, maybe we should also change __str__ so that <2 and <2.dev0 would always be printed as <2? I suppose explicit constraints like <2.dev0 are rare so that complaints that <2.dev0 gives <2 are neglectable.

@dimbleby
Copy link
Contributor Author

dimbleby commented Oct 3, 2023

that seems plausible and I have a branch in which I tried it. Among the chaos in the unit tests I see at least these wrinkles

  • lots of failures where the unit tests compare a parsed value to an explicitly constructed Version and now get a different answer: to be expected, but will be tedious to work through
  • there's a distinction between version ranges as used in version requirements, and versions as used in markers / python versions. So as not to break the world it's probably unwise to make changes for the latter two
  • the good news is that printing <2.dev0 as just <2 is I think fine in the PEP440 context; because the rules about prereleases give these strings the same meaning
  • this approach both fixes and introduces bugs when dealing with != specifiers
    • eg !=2 is represented internally as <2 || >2. Today, the allowed_max business re-interprets that as <2.dev0 || >2 ie it wrongly disallows the prereleases
    • eg after the change, (>1).union(!=2) for some reason becomes >1,<2 || >2. Without allowed_max that's correct as an internal representation, but becomes wrong once re-parsed - it really does want to be written eg in lockfiles as >1,!=2

probably there's more that I haven't yet spotted

This feels like the right direction conceptually but I've no idea when or whether I'm likely to find the time or energy to do anything about it

@eickr
Copy link

eickr commented Aug 7, 2024

I now think that the real problem - or anyway among the real problems - is ambiguity in poetry-core's treatment of the max in a strict inequality range.

  • when dealing with a parsed value such as <2 the max is 2 but the allowed_max should be 2.dev0, because pre-releases are excluded by <

Pre-releases must be excluded by < unless the specified version is itself a pre-release, yes. But development releases are pre-releases for any release, including pre-/post-releases.
The versions 2aN.devN, 2aN, 2bN.devN, 2bN, 2rcN.devN, 2rcN are all less than 2.dev0.
If you wanted a pre-release allowed exclusive upper bound for the latest version starting with 1 or less then the exclusive ordered comparison equivalent syntax should be <2a0.dev0.

@dimbleby
Copy link
Contributor Author

dimbleby commented Aug 7, 2024

@eickr
Copy link

eickr commented Aug 7, 2024

Ah, I see. Apologies for wasting your time. In hindsight everything makes much more sense now.

I interpreted this part of the PEP incorrectly, "Developmental releases are ordered by their numerical component, immediately before the corresponding release (and before any pre-releases with the same release segment), and following any previous release (including any post-releases)." I had incorrectly taken the parenthetical parts to be solely about how developmental releases for pre-/post-releases are ordered before their respective pre-/post-release. And then I somehow managed to repeatedly miss the full example list at the end of the section you linked.

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

Successfully merging this pull request may close these issues.

poetry never resolves package combination: google-cloud-aiplatform kfp
3 participants