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

less aggressive overlapping markers #5887

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

BurntSushi
Copy link
Member

@BurntSushi BurntSushi commented Aug 7, 2024

This is like #5733, but implements less aggressive forking. Namely, in
#5733, we would possibly fork any time we say a marker expression, even
if there was only one dependency specification for that package. In
this PR, we only consider forking when there are at least two sibling
dependency specifications for the same package name.

The end result here is that this fixes #4640 but not #4668. It also
doesn't fix some as-yet unreported bugs related to detecting forks in
conflicting transitive dependencies. However, this does have fewer
changes overall in resolutions when compared to the status quo, and
also has a much smaller performance regression in some ad hoc
benchmarks.

Our plan is to compare this PR and #5733 on real world projects to see
how the actual resolutions differ.

Fixes #4640, Closes #4732

@BurntSushi BurntSushi changed the base branch from main to ibraheem/canonical-markers August 7, 2024 18:52
@BurntSushi
Copy link
Member Author

On the transformers benchmark detailed in #5733:

$ hyperfine -w10 -p 'rm -rf uv.lock' 'uv-main lock' 'uv-overlapping-markers-aggressive lock' 'uv-overlapping-markers-conservative lock'
Benchmark 1: uv-main lock
  Time (mean ± σ):      49.4 ms ±   2.6 ms    [User: 47.3 ms, System: 52.2 ms]
  Range (min … max):    44.1 ms …  55.8 ms    63 runs

Benchmark 2: uv-overlapping-markers-aggressive lock
  Time (mean ± σ):     101.4 ms ±   3.9 ms    [User: 106.9 ms, System: 72.7 ms]
  Range (min … max):    94.8 ms … 109.0 ms    30 runs

Benchmark 3: uv-overlapping-markers-conservative lock
  Time (mean ± σ):      53.6 ms ±   3.2 ms    [User: 51.1 ms, System: 54.8 ms]
  Range (min … max):    47.0 ms …  61.2 ms    54 runs

Summary
  uv-main lock ran
    1.09 ± 0.09 times faster than uv-overlapping-markers-conservative lock
    2.05 ± 0.13 times faster than uv-overlapping-markers-aggressive lock

And on Home Assistant:

$ hyperfine -w10 -p 'rm -rf uv.lock' 'uv-main lock' 'uv-overlapping-markers-aggressive lock' 'uv-overlapping-markers-conservative lock'
Benchmark 1: uv-main lock
  Time (mean ± σ):     181.6 ms ±   2.7 ms    [User: 145.5 ms, System: 115.4 ms]
  Range (min … max):   176.4 ms … 186.7 ms    16 runs

Benchmark 2: uv-overlapping-markers-aggressive lock
  Time (mean ± σ):     223.4 ms ±   3.8 ms    [User: 184.9 ms, System: 121.8 ms]
  Range (min … max):   219.4 ms … 232.2 ms    13 runs

Benchmark 3: uv-overlapping-markers-conservative lock
  Time (mean ± σ):     180.8 ms ±   2.6 ms    [User: 143.3 ms, System: 115.9 ms]
  Range (min … max):   177.2 ms … 187.5 ms    16 runs

Summary
  uv-overlapping-markers-conservative lock ran
    1.00 ± 0.02 times faster than uv-main lock
    1.24 ± 0.03 times faster than uv-overlapping-markers-aggressive lock

Note that uv-overlapping-markers-{conservative,aggressive} are both derived from @ibraheemdev's BDD marker work (which isn't on main yet). Without the BDD work, uv-overlapping-markers-aggressive is slower (3-4x regression from main instead of 1-2x).

@ibraheemdev ibraheemdev force-pushed the ibraheem/canonical-markers branch 14 times, most recently from 8cb5abe to cccbe8c Compare August 9, 2024 15:36
Base automatically changed from ibraheem/canonical-markers to main August 9, 2024 17:40
BurntSushi added a commit that referenced this pull request Aug 9, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 9, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 9, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
@BurntSushi BurntSushi force-pushed the ag/overlapping-markers-conservative branch from 1280ca7 to e599124 Compare August 9, 2024 18:56
----- stdout -----

----- stderr -----
warning: `uv lock` is experimental and may change without warning
Resolved 7 packages in [TIME]
error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.
Copy link
Member Author

Choose a reason for hiding this comment

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

These snapshots look wrong. I haven't investigated yet, but we probably shouldn't merge before fixing these.

Copy link

codspeed-hq bot commented Aug 9, 2024

CodSpeed Performance Report

Merging #5887 will degrade performances by 10.88%

Comparing ag/overlapping-markers-conservative (ca708f2) with main (8dbf43c)

Summary

❌ 1 regressions
✅ 13 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main ag/overlapping-markers-conservative Change
resolve_warm_jupyter_universal 282.1 ms 316.5 ms -10.88%

ibraheemdev added a commit that referenced this pull request Aug 10, 2024
## Summary

Follow up to #5898. This should fix
some of the failures in #5887 where
`uv lock --locked` is failing due to `Some(true)` and `None` markers not
comparing equal.
BurntSushi added a commit that referenced this pull request Aug 12, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 12, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 12, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 13, 2024
This adds a new top-level directory with bare-bones directories for a
sampling of ecosystem projects. The idea is for each directory to have
enough that `uv lock` can run.

The point of these tests is to 1) ensure resolution works in common
cases and 2) track changes to resolutions (and the lock file) in real
world projects.

Unfortunately, it does look like in some cases, re-running `uv lock`
results in changes to the lock file. For those cases, I've disabled the
deterministic checking in exchange for getting the lock files tracked
in tests. I haven't investigated yet whether either of #5733 or #5887
fix the deterministic problem.

There is probably a better way to go about integrating ecosystem
projects. In particular, it would be really nice if there was a good
flow for upgrading ecosystem packages to their latest version. The main
complexity is that some projects require edits to their `pyproject.toml`
(or a complete migration from non-`pyproject.toml` to `pyproject.toml`).
Although, the projects added here in this initial set were limited to
those that didn't require any changes.
BurntSushi added a commit that referenced this pull request Aug 13, 2024
At a high level, this PR adds a smattering of new tests that
effectively snapshot the output of `uv lock` for a selection of
"ecosystem" projects. That is, real Python projects for which we expect
`uv` to work well with.

The main idea with these tests is to get a better idea of how changes
in `uv` impact the lock files of real world projects. For example,
we're hoping that these tests will help give us data for how #5733
differs from #5887.

This has already revealed some bugs. Namely, re-running `uv lock` for a
second time will produce a different lock file for some projects. So to
prioritize getting the tests added, for those projects, we don't do the
deterministic checking.
@BurntSushi BurntSushi force-pushed the ag/overlapping-markers-conservative branch from e599124 to eff323b Compare August 13, 2024 14:14
I believe these are all changes that aren't necessarily
expected, but also seem harmless. Like the order in which
fork markers are written to the lock file. (Although one
wonders if we should fix that once and for all by defining
a complete sort function for forks.)
…rker

The test in this case has this comment:

```
/// If a dependency requests a prerelease version with an overlapping marker expression,
/// we should prefer the prerelease version in both forks.
```

With this setup:

```
    let pyproject_toml = context.temp_dir.child("pyproject.toml");
    pyproject_toml.write_str(indoc! {r#"
        [project]
        name = "example"
        version = "0.0.0"
        dependencies = [
            "cffi >= 1.17.0rc1 ; os_name == 'Linux'"
        ]
        requires-python = ">=3.11"
    "#})?;

    let requirements_in = context.temp_dir.child("requirements.in");
    requirements_in.write_str(indoc! {"
        cffi
        .
    "})?;
```

The change in this commit _seems_ more correct that what we had,
although it does seem to contradict the comment. Namely, in the `os_name
!= "Linux"` fork, we don't prefer the pre-release version since the
`cffi >= 1.17.0rc1` bound doesn't apply.

It's not quite clear what to do in this instance.
I didn't mean to commit these in #5970.
@BurntSushi
Copy link
Member Author

This does regress the warm resolve for jupyter by about 10%, but I think that's acceptable for a change like this where we improve correctness at the known cost of possibly forking more. Of course, we should investigate further to improve perf and possibly even avoid forking if we can determine it won't be useful, but I think that should be reserved for future work.

@BurntSushi BurntSushi merged commit 037ba84 into main Aug 13, 2024
56 of 57 checks passed
@BurntSushi BurntSushi deleted the ag/overlapping-markers-conservative branch August 13, 2024 15:35
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Aug 15, 2024
This MR contains the following updates:

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

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.2.36`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0236)

[Compare Source](astral-sh/uv@0.2.35...0.2.36)

##### Bug fixes

-   Use consistent canonicalization for URLs ([#&#8203;5980](astral-sh/uv#5980))
-   Improve warning message when parsing `pyproject.toml` fails ([#&#8203;6009](astral-sh/uv#6009))
-   Improve handling of overlapping markers in universal resolver ([#&#8203;5887](astral-sh/uv#5887))

</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
None yet
Projects
None yet
2 participants