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

Treat less compatible tags as lower priority in resolver #9339

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

charliermarsh
Copy link
Member

Summary

This is a second pass at #7556, which was reverted in #7608 due to a regression in #7606. The behavior is actually correct, but a package (nmslib) publishes inconsistent metadata, and the change here happened to cause us to select a wheel with "wrong" metadata. It's arbitrary, but it did cause a regression for folks.

Since we're now seeing other issues caused by the wrongness here (and since the reporter in #7606 has since removed the dependency), I'm inclined to ship this fix.

Closes #7553.
Closes #9283.

@charliermarsh charliermarsh requested a review from zanieb November 21, 2024 22:47
@charliermarsh charliermarsh added the bug Something isn't working label Nov 21, 2024
Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still sort of hesitant.

One of the linked issues comes from selecting Python 2 wheels? I feel like we should never read metadata from those?

@charliermarsh
Copy link
Member Author

We can change that, but why hesitant? The current behavior is legitimately incorrect — it treats less compatible wheels as more compatible.

@zanieb
Copy link
Member

zanieb commented Nov 22, 2024

Perhaps I'm not following. I'm going off the title:

invalid platform as more compatible than invalid Python

This sounds like we prefer an invalid platform over an invalid Python version — I don't see one of those as more-correct?

@charliermarsh
Copy link
Member Author

I think the PR title is kind of bad (I copied it over from the previous PR). My understanding of what's happening is that, at present, if we have two wheels with incompatible tags, we treat the wheel the wheel with "less compatible" tags as "higher priority" (which is wrong). Later, in error reporting, we say no wheels with a matching Python implementation tag if the "highest priority" tag is IncompatibleTag::Python, because we're demonstrating that the best wheel we could find uses a different Python implementation. But that's not true -- there are wheels with matching Python implementations, we just treated them as "lower priority" by accident.

@charliermarsh
Copy link
Member Author

Basically: I think everywhere else we actually do encode that mismatching platform tag is more compatible than mismatching Python implementation or ABI:

#[derive(Debug, Eq, Ord, PartialEq, PartialOrd, Clone)]
pub enum IncompatibleTag {
    /// The tag is invalid and cannot be used.
    Invalid,
    /// The Python implementation tag is incompatible.
    Python,
    /// The ABI tag is incompatible.
    Abi,
    /// The Python version component of the ABI tag is incompatible with `requires-python`.
    AbiPythonVersion,
    /// The platform tag is incompatible.
    Platform,
}

So the system assumes that in various places. Right now, we would actually actively prioritize Python 2 wheels over Python 3 wheels.

@zanieb
Copy link
Member

zanieb commented Nov 22, 2024

Thanks for explaining! Makes sense to me then.

@charliermarsh charliermarsh changed the title Treat invalid platform as more compatible than invalid Python Treat less compatible tags as lower priority in resolver Nov 26, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) November 26, 2024 14:44
@charliermarsh charliermarsh merged commit edcff57 into main Nov 26, 2024
64 checks passed
@charliermarsh charliermarsh deleted the charlie/prio branch November 26, 2024 14:51
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Nov 28, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.4` -> `0.5.5` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.5.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#055)

[Compare Source](astral-sh/uv@0.5.4...0.5.5)

##### Enhancements

-   Add aliases for build backend requests ([#&#8203;9294](astral-sh/uv#9294))
-   Avoid displaying empty paths ([#&#8203;9312](astral-sh/uv#9312))
-   Allow constraints in `uv tool upgrade` ([#&#8203;9375](astral-sh/uv#9375))
-   Remove conflict between `--no-sync` and `--frozen` in `uv run` ([#&#8203;9400](astral-sh/uv#9400))
-   Respect dependency sources in overrides and constraints ([#&#8203;9455](astral-sh/uv#9455))
-   Show an interpreter-focused message for `--target` and `--prefix` ([#&#8203;9373](astral-sh/uv#9373))
-   Add `--no-extra` flag and setting ([#&#8203;9387](astral-sh/uv#9387))
-   Add `uv export --prune` ([#&#8203;9389](astral-sh/uv#9389))
-   Add dedicated error message for musl install attempts ([#&#8203;9430](astral-sh/uv#9430))
-   Add various grammar changes to conflict error messages ([#&#8203;9369](astral-sh/uv#9369))
-   Annotate default groups in conflict error messages ([#&#8203;9368](astral-sh/uv#9368))
-   Report marker diagnostics during parsing, rather than evaluation ([#&#8203;9338](astral-sh/uv#9338))
-   Use consistent formatting for build system errors ([#&#8203;9340](astral-sh/uv#9340))
-   Use rich diagnostics for build failures ([#&#8203;9335](astral-sh/uv#9335))

##### Preview features

-   Improve build backend excludes ([#&#8203;9281](astral-sh/uv#9281))
-   Include PEP 639 `license-files` metadata during `uv publish` ([#&#8203;9442](astral-sh/uv#9442))

##### Performance

-   Initialize rayon lazily ([#&#8203;9435](astral-sh/uv#9435))
-   Migrate to PubGrub's arena for package names ([#&#8203;9448](astral-sh/uv#9448))

##### Bug fixes

-   Allow dependency groups to include the containing package ([#&#8203;9385](astral-sh/uv#9385))
-   Allow syncing to empty virtual environment directories ([#&#8203;9427](astral-sh/uv#9427))
-   Allow system Python discovery with `--target` and `--prefix` ([#&#8203;9371](astral-sh/uv#9371))
-   Don't warn when `--output-file` is empty ([#&#8203;9417](astral-sh/uv#9417))
-   Fix Python interpreter discovery on non-glibc hosts ([#&#8203;9005](astral-sh/uv#9005))
-   Fix `tool.uv.dependency-metadata.[].version` schema ([#&#8203;9468](astral-sh/uv#9468))
-   Only respect preferences across the same indexes ([#&#8203;9302](astral-sh/uv#9302))
-   Re-compile when `--compile` is passed to an install operation ([#&#8203;9378](astral-sh/uv#9378))
-   Remove `--upgrade`, `--no-upgrade`, and `--upgrade-package` from `uv tool upgrade` ([#&#8203;9318](astral-sh/uv#9318))
-   Remove dev dependencies in `--all-groups --no-dev` ([#&#8203;9300](astral-sh/uv#9300))
-   Surface extras and group conflicts in `uv export` ([#&#8203;9365](astral-sh/uv#9365))
-   Treat deprecated aliases as equivalent in marker algebra ([#&#8203;9342](astral-sh/uv#9342))
-   Treat less compatible tags as lower priority in resolver ([#&#8203;9339](astral-sh/uv#9339))

##### Documentation

-   Avoid referencing `scikit-build` (instead of `scikit-build-core`) ([#&#8203;9320](astral-sh/uv#9320))
-   Expand entry points documentation ([#&#8203;9329](astral-sh/uv#9329))
-   Fix example `pyproject.toml` in project concept documentation ([#&#8203;9298](astral-sh/uv#9298))
-   Fix header level of "Conflicting dependencies" page ([#&#8203;9330](astral-sh/uv#9330))
-   Touch-up the extension module guide ([#&#8203;9293](astral-sh/uv#9293))
-   Update the dependencies documentation ([#&#8203;9359](astral-sh/uv#9359))
-   Reference `--no-progress` option in related environment variable ([#&#8203;9357](astral-sh/uv#9357))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants