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

unstable sort in case where versions only differ in their build #132

Closed
keshav-space opened this issue May 18, 2022 · 12 comments · Fixed by #133
Closed

unstable sort in case where versions only differ in their build #132

keshav-space opened this issue May 18, 2022 · 12 comments · Fixed by #133
Labels
Status:CodeWelcome This feature design has been approved; next step: write the code! Topic:API Improvements to the library API Topic:SemVer Issues related to SemVer implementation Type:Enhancement

Comments

@keshav-space
Copy link

We're observing unstable sort in cases where versions only differ in their build.

>>> from semantic_version import Version
>>> v1 = Version("2.0.12+123")
>>> v2 = Version("2.0.12+1")
>>> v3 = Version("2.0.12+321")
>>> v4 = Version("2.0.12+22")
>>> sorted([v1,v2,v3,v4])
[Version('2.0.12+123'), Version('2.0.12+1'), Version('2.0.12+321'), Version('2.0.12+22')]
>>> sorted([v4,v2,v1,v3])
[Version('2.0.12+22'), Version('2.0.12+1'), Version('2.0.12+123'), Version('2.0.12+321')]
>>> sorted([v1,v2,v3,v4])
[Version('2.0.12+123'), Version('2.0.12+1'), Version('2.0.12+321'), Version('2.0.12+22')]
>>>

This is due to Semver mandating that the versions differing only in their build must have the same precedence.
Nevertheless, we have an implementation in node which does consider build to yield a stable sorting.
Can we have something similar here?

@rbarrois
Copy link
Owner

That's indeed an interesting issue ;)

I concur that one would expect the following to be true:

def ensure_stable_sort(seq):
    """Ensure sort stability.

    Once sorted, all permutations of a sequence should be equal;
    i.e objects at position i are always equal -- though they may not have the same "id".
    """
    shuffled = random.sample(seq, k=len(seq))  # See note in https://docs.python.org/3/library/random.html#random.shuffle
    for orig, shuf in zip(sorted(seq), sorted(shuffled)):
        assert orig == shuf

Here, as you've shown, that property doesn't hold.

My idea would be to have a mechanism for comparing versions that doesn't apply any formalized semantics on the build version.
For instance, one could use some hash of the build component as a precedence key: it's deterministic, stable, yet opaque from the user. The only issue would be that the hash will be different across runs or processes (see the note about PYTHONHASHSEED in python(1)).

What do you think? Is this consistent with the behaviour you would expect?

@rbarrois rbarrois added Type:Enhancement Topic:SemVer Issues related to SemVer implementation Topic:API Improvements to the library API labels May 18, 2022
@keshav-space
Copy link
Author

Hash would work for a particular run.
But we're expecting consistent sorting across runs/devices. Which IMO isn't achievable using hash.
We might need some alphanumeric comparison of the build itself.

What do you say @pombredanne?

@keshav-space
Copy link
Author

Or maybe we can instead use something like SHA-256 which is guaranteed to be same across runs/devices.

@pombredanne
Copy link
Contributor

pombredanne commented May 19, 2022

The only issue would be that the hash will be different across runs or processes

@rbarrois this would IMHO break the stability property. Sort stability is expected to apply across Python versions and implementations.

The thing it is hard to get this right:

  1. It is quite nice to use the standard rich comparison operators such that version are comparable with >, < and so on. This is the basis of this implementation (and the basis of most if all version handling libraries I know of... we looked at quite a few ;) for univers)
  2. semver is peculiar is that the "build" segment is not significant for determining precedence.
  3. this library implements a correct hash() that includes the build segment correctly
  1. this library and the other libraries both ignore the build segment in sorting.
  2. this library uses the all segments in equality as it should

With this said, here is my suggestion:

  • the semver spec that the build does not matter for precedence. Therefore it does not matter too that this is used. Since two builds will be equivalent which one is picked over another one does not matter and is still compliant with the spec. So the build can be included in the precedence and no harm is done
  • you could add a new method to the version that returns True if two versions are identical ignoring build say is_equivalent(self, other)
  • when you do matching you can also ignore the build segment: tow version can be equivalent, but neither equal nor identical (same hash). But two versions that are identical hash-wise should be equal IMHO.

@pombredanne
Copy link
Contributor

pombredanne commented May 19, 2022

Just to restate my key point that may not be obvious: since the build does not matter in semver ordering, using here a stable order including build is still compliant because order does not matter .

@rbarrois
Copy link
Owner

@pombredanne Thanks for the details. I agree that any ordering is valid.
However, I'm afraid that, were I to order build numbers lexically, users would come to rely on that behaviour. The suggestion by @keshav-space to use a SHA hash has the benefit of using an opaque hash, alleviating that concern.

Hence, I think I'll move forward with a SHA-like hash of the build for the precedence key (maybe opting for a faster-to-compute one for performance concerns).
Would that match your expectations?

@pombredanne
Copy link
Contributor

Would that match your expectations?

Not really. 😇

Using a checksum to hide the ordering will not hide anything IMHO. Unless you take the library closed source ;) you are not hiding anything from anyone. So I could read the code and come to rely on a certain stable order based on whatever hashing you picked.
IMHO KISS, and in earnest I would find it super surprising if the build ordering was not based the plain sort of the strings.
Say you have 1.2.3+1 and 1.2.3+2 ... it begs to be sorted by 1 then 2. Anything else will be a (bad) surprise and a head scratcher IMHO.

And -- this is key here-- this is also the observed behaviour of node-semver See https://github.com/npm/node-semver/blob/4907647d169948a53156502867ed679268063a9f/test/functions/sort.js#L4 and https://github.com/npm/node-semver/blob/f1e4e293de1ad73fa250336bfcfb9b4142d21d1b/classes/semver.js#L152

@rbarrois
Copy link
Owner

I see your point; however, I don't want to have the discussion of *Should 1.0.0+1.1 sort before or after 1.0.0+11.1?

Until that point is discussed and settled in the semver spec, I'd rather opt for an opaque ordering (not obfuscated, just not lexical).

As a side note, this was somewhat sidestepped in the SemVer repo: semver/semver#89.

@pombredanne
Copy link
Contributor

@rbarrois re:

Until that point is discussed and settled in the semver spec, I'd rather opt for an opaque ordering (not obfuscated, just not lexical).

This is your call of course, but this means that this library will not match the node-semver behaviour and we will need to either subclass or fork it as this is a primary use case for univers. For instance, it will not pass the same tests as node-semver.

I am in earnest less interested in the semver spec per-se than its prominent implementations because the spec is mostly interpreted loosely by its key users. So much so that at this stage I very much doubt that there is any "true-to-spec" implementation of semver based on our experience with the univers library and vulnerable code: there is IMHO roughly one slightly different take on semver in each implementation.

@rbarrois
Copy link
Owner

@pombredanne After giving this some more thought, I think that using the same ordering rules as for components of the "prerelease" bits (i.e according to the rules in section 11.4 of Semver) will be the least-surprising way to go.

I'll have to add some mentions on that topic in the docs, though :)

@rbarrois rbarrois added the Status:CodeWelcome This feature design has been approved; next step: write the code! label May 19, 2022
@pombredanne
Copy link
Contributor

@rbarrois Thank you ++

keshav-space added a commit to keshav-space/univers that referenced this issue May 25, 2022
    - Revert this once this is fixed in upstream
    - rbarrois/python-semanticversion#132

Signed-off-by: Keshav Priyadarshi <[email protected]>
rbarrois added a commit that referenced this issue May 26, 2022
Sorting any permutation of Version objects should always yield the same
result, even if those hold some build metadata.

To that end, the "precedence_key" is now used exclusively for sorting;
direct comparisons between Version objects still ignores the "build"
metadata, using a different precedence key.

For performance improvements, both precedence keys are cached.

Closes: #132
rbarrois added a commit that referenced this issue May 26, 2022
Sorting any permutation of Version objects should always yield the same
result, even if those hold some build metadata.

To that end, the "precedence_key" is now used exclusively for sorting;
direct comparisons between Version objects still ignores the "build"
metadata, using a different precedence key.

For performance improvements, both precedence keys are cached.

Closes: #132
@rbarrois
Copy link
Owner

Release in v2.10.0 :)

mtremer referenced this issue in ipfire/ipfire-2.x Nov 11, 2022
…thon-3.10.8

- Updated from version 2.9.0 to 2.10.0
- Update of rootfile
- Changelog
    2.10.0 (2022-05-26)
	*New:*
	    * `132 <https://github.com/rbarrois/python-semanticversion/issues/132>`_:
	      Ensure sorting a collection of versions is always stable, even with
	      build metadata.

Tested-by: Adolf Belka <[email protected]>
Signed-off-by: Adolf Belka <[email protected]>
Bento007 referenced this issue in chanzuckerberg/cellxgene-ontology-guide Mar 7, 2024
…ology-builder (#97)

Bumps
[semantic-version](https://github.com/rbarrois/python-semanticversion)
from 2.8.5 to 2.10.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/rbarrois/python-semanticversion/blob/master/ChangeLog">semantic-version's
changelog</a>.</em></p>
<blockquote>
<h2>2.10.0 (2022-05-26)</h2>
<p><em>New:</em></p>
<pre><code>* `132
&lt;https://github.com/rbarrois/python-semanticversion/issues/132&gt;`_:
  Ensure sorting a collection of versions is always stable, even with
  build metadata.
</code></pre>
<h2>2.9.0 (2022-02-06)</h2>
<p><em>New:</em></p>
<pre><code>* Add support for Django 3.1, 3.2, 4.0
* Add support for Python 3.7 / 3.8 / 3.9 / 3.10
</code></pre>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/e49b5b065b845cd7798c0219e0fa8986c75f6a4a"><code>e49b5b0</code></a>
Preparing release 2.10.0</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/57c78e7307792879dce33734c11e7774383b9d36"><code>57c78e7</code></a>
Guarantee a stable ordering with build metadata</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/7dcc42d2a828adbbeb6f8a23cdca40a3c61782bc"><code>7dcc42d</code></a>
Fix pip install name in README</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/47be07eb4a632850b28cc584b4caa54ed02cd924"><code>47be07e</code></a>
Update the Version.parse() to match the code</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/8a7162fc01b33964688a5be41df3865bceb875c3"><code>8a7162f</code></a>
docs: Update reference to NPM range specification</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/c61278bd35f2059ad3c7fc196a4b06eae34f3b7e"><code>c61278b</code></a>
Back to development: 2.9.1</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/5f2bf3539ea877540f52ac47d5317ee1b17ba761"><code>5f2bf35</code></a>
Preparing release 2.9.0</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/2713cf34f37214f2b3acfa79e8909d0d03dcabac"><code>2713cf3</code></a>
doc: Remove remaining Sphinx markup from README</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/4fcc1475e0161bdf9fa3737023068afae15c62f1"><code>4fcc147</code></a>
docs: Update CI location</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/7e59a4b2e82abe4338e307b9fe49b072c9537a15"><code>7e59a4b</code></a>
Improve documentation</li>
<li>Additional commits viewable in <a
href="https://github.com/rbarrois/python-semanticversion/compare/2.8.5...2.10.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=semantic-version&package-manager=pip&previous-version=2.8.5&new-version=2.10.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bento007 referenced this issue in chanzuckerberg/cellxgene-ontology-guide Mar 7, 2024
…ython (#98)

Bumps
[semantic-version](https://github.com/rbarrois/python-semanticversion)
from 2.8.5 to 2.10.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/rbarrois/python-semanticversion/blob/master/ChangeLog">semantic-version's
changelog</a>.</em></p>
<blockquote>
<h2>2.10.0 (2022-05-26)</h2>
<p><em>New:</em></p>
<pre><code>* `132
&lt;https://github.com/rbarrois/python-semanticversion/issues/132&gt;`_:
  Ensure sorting a collection of versions is always stable, even with
  build metadata.
</code></pre>
<h2>2.9.0 (2022-02-06)</h2>
<p><em>New:</em></p>
<pre><code>* Add support for Django 3.1, 3.2, 4.0
* Add support for Python 3.7 / 3.8 / 3.9 / 3.10
</code></pre>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/e49b5b065b845cd7798c0219e0fa8986c75f6a4a"><code>e49b5b0</code></a>
Preparing release 2.10.0</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/57c78e7307792879dce33734c11e7774383b9d36"><code>57c78e7</code></a>
Guarantee a stable ordering with build metadata</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/7dcc42d2a828adbbeb6f8a23cdca40a3c61782bc"><code>7dcc42d</code></a>
Fix pip install name in README</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/47be07eb4a632850b28cc584b4caa54ed02cd924"><code>47be07e</code></a>
Update the Version.parse() to match the code</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/8a7162fc01b33964688a5be41df3865bceb875c3"><code>8a7162f</code></a>
docs: Update reference to NPM range specification</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/c61278bd35f2059ad3c7fc196a4b06eae34f3b7e"><code>c61278b</code></a>
Back to development: 2.9.1</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/5f2bf3539ea877540f52ac47d5317ee1b17ba761"><code>5f2bf35</code></a>
Preparing release 2.9.0</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/2713cf34f37214f2b3acfa79e8909d0d03dcabac"><code>2713cf3</code></a>
doc: Remove remaining Sphinx markup from README</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/4fcc1475e0161bdf9fa3737023068afae15c62f1"><code>4fcc147</code></a>
docs: Update CI location</li>
<li><a
href="https://github.com/rbarrois/python-semanticversion/commit/7e59a4b2e82abe4338e307b9fe49b072c9537a15"><code>7e59a4b</code></a>
Improve documentation</li>
<li>Additional commits viewable in <a
href="https://github.com/rbarrois/python-semanticversion/compare/2.8.5...2.10.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=semantic-version&package-manager=pip&previous-version=2.8.5&new-version=2.10.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:CodeWelcome This feature design has been approved; next step: write the code! Topic:API Improvements to the library API Topic:SemVer Issues related to SemVer implementation Type:Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants