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

Avoid adding non-extra package with extra dependencies #9540

Merged
merged 1 commit into from
Dec 1, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Nov 30, 2024

Summary

Previously, when we encountered foo[bar], we'd add a dependency on PubGrubPackage::Package for foo, and then PubGrubPackage::Extra for foo[bar].

Later, when we ask for the dependencies of the PubGrubPackage::Extra, we add PubGrubPackage::Package for foo, and PubGrubPackage::Package for foo[bar]. This is an intentional strategy because it ensures that PubGrub "knows" that these have to be solved to the same version as early as possible.

It turns out that the first part here ("add a dependency on PubGrubPackage::Package for foo") is suboptimal, because it means PubGrub might try to solve just foo without realizing that it also has to accommodate all the constraints from the extra.

Instead, we now add just PubGrubPackage::Extra for foo[bar], and defer adding the base package. It looks like this leads to a far more efficient solve for Airflow.

Copy link

codspeed-hq bot commented Nov 30, 2024

CodSpeed Performance Report

Merging #9540 will improve performances by 77.1%

Comparing charlie/transitive (042f88a) with main (8126a5e)

Summary

⚡ 1 improvements
✅ 13 untouched benchmarks

Benchmarks breakdown

Benchmark main charlie/transitive Change
resolve_warm_airflow 2 s 1.1 s +77.1%

@charliermarsh
Copy link
Member Author

The gain actually seems real? At least to some degree:

❯ hyperfine "./target/release/uv pip compile req.in" "./target/release/transitive pip compile req.in" --warmup 10 --runs 50
Benchmark 1: ./target/release/uv pip compile req.in
  Time (mean ± σ):     198.6 ms ±   6.0 ms    [User: 224.9 ms, System: 244.6 ms]
  Range (min … max):   189.8 ms … 213.9 ms    50 runs

Benchmark 2: ./target/release/transitive pip compile req.in
  Time (mean ± σ):     149.4 ms ±   4.1 ms    [User: 160.6 ms, System: 182.2 ms]
  Range (min … max):   144.9 ms … 173.2 ms    50 runs

Summary
  ./target/release/transitive pip compile req.in ran
    1.33 ± 0.05 times faster than ./target/release/uv pip compile req.in

@charliermarsh
Copy link
Member Author

This change is sound but I clearly need to modify some of how we interpret the downstream graph in pip compile.

@charliermarsh charliermarsh force-pushed the charlie/transitive branch 11 times, most recently from 3f8bb22 to 6069f79 Compare November 30, 2024 17:42
@charliermarsh charliermarsh marked this pull request as ready for review November 30, 2024 22:18
@charliermarsh charliermarsh added the performance Potential performance improvement label Nov 30, 2024
@charliermarsh charliermarsh force-pushed the charlie/transitive branch 2 times, most recently from 6af4741 to 2b5803f Compare November 30, 2024 22:32
if self_dev.is_none()
&& self_name.is_some_and(|self_name| self_name == dependency_name)
{
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

I reasoned through these and most of them can be removed.

warn_user_once!(
"The direct dependency `{package}` is unpinned. \
"The direct dependency `{name}` is unpinned. \
Copy link
Member Author

Choose a reason for hiding this comment

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

If we just use name, we can rely on warn_user_once.

@charliermarsh charliermarsh force-pushed the charlie/transitive branch 3 times, most recently from 7413668 to 374649f Compare December 1, 2024 01:45
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

wow wouldn't have thought about that trick for such a speed!

@charliermarsh charliermarsh merged commit 71df509 into main Dec 1, 2024
64 checks passed
@charliermarsh charliermarsh deleted the charlie/transitive branch December 1, 2024 13:42
BurntSushi added a commit that referenced this pull request Dec 2, 2024
This _partially_ unwinds the optimization in #9540 by adding back the
base package dependency as a sibling to the extra package dependency
in some cases. Specifically, this occurs when _any_ of the extras are
declared as conflicting.

This is believed to be necessary (until another method is found) to
handle the forking logic based on conflicts. Namely, the forking logic
depends on the base and extra packages being sibling dependencies. If
only the extra is present, then it won't be included in the fork that
excludes all conflicting extras. And that means the base package won't
either, even though it should be included in that fork in some cases. If
the base package dependency is deferred, then it will never be reached.

This also adds another test and updates the snapshots that would have
caught the regression in #9540 if the conflict tests had been enabled.
BurntSushi added a commit that referenced this pull request Dec 2, 2024
This _partially_ unwinds the optimization in #9540 by adding back the
base package dependency as a sibling to the extra package dependency
in some cases. Specifically, this occurs when _any_ of the extras are
declared as conflicting.

This is believed to be necessary (until another method is found) to
handle the forking logic based on conflicts. Namely, the forking logic
depends on the base and extra packages being sibling dependencies. If
only the extra is present, then it won't be included in the fork that
excludes all conflicting extras. And that means the base package won't
either, even though it should be included in that fork in some cases. If
the base package dependency is deferred, then it will never be reached.

This also adds another test and updates the snapshots that would have
caught the regression in #9540 if the conflict tests had been enabled.
BurntSushi added a commit that referenced this pull request Dec 2, 2024
This _partially_ unwinds the optimization in #9540 by adding back the
base package dependency as a sibling to the extra package dependency
in some cases. Specifically, this occurs when _any_ of the extras are
declared as conflicting.

This is believed to be necessary (until another method is found) to
handle the forking logic based on conflicts. Namely, the forking logic
depends on the base and extra packages being sibling dependencies. If
only the extra is present, then it won't be included in the fork that
excludes all conflicting extras. And that means the base package won't
either, even though it should be included in that fork in some cases. If
the base package dependency is deferred, then it will never be reached.

This also adds another test and updates the snapshots that would have
caught the regression in #9540 if the conflict tests had been enabled.
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Dec 5, 2024
This MR contains the following updates:

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

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

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

##### Enhancements

-   Add `--dry-run` to `uv pip uninstall` ([#&#8203;9557](astral-sh/uv#9557))
-   Allow `--constraints` and `--overrides` in `uv tool install` ([#&#8203;9547](astral-sh/uv#9547))
-   Display removed Python executables on uninstall ([#&#8203;9459](astral-sh/uv#9459))
-   Warn when keyring has no password for `uv publish` ([#&#8203;8827](astral-sh/uv#8827))
-   Add suggested action when `.python-version` pin is incompatible with the project ([#&#8203;9590](astral-sh/uv#9590))
-   Improve error messages for mismatches in `tool.uv.sources` ([#&#8203;9482](astral-sh/uv#9482))
-   Use constraints in trace rather than irrelevant `requires-python` ([#&#8203;9529](astral-sh/uv#9529))

##### Preview features

-   Add `uv python install --default` ([#&#8203;8650](astral-sh/uv#8650))
-   Fix Python executable installation when multiple patch versions are requested ([#&#8203;9607](astral-sh/uv#9607))
-   Build backend: Revamp `include` / `exclude` ([#&#8203;9525](astral-sh/uv#9525))
-   Build backend: Add fast path  ([#&#8203;9556](astral-sh/uv#9556))
-   Build backend: Add functions to collect file list ([#&#8203;9602](astral-sh/uv#9602))
-   Build backend: Default excludes ([#&#8203;9552](astral-sh/uv#9552))
-   Build backend: Refactoring before list ([#&#8203;9558](astral-sh/uv#9558))
-   Build backend: Warn when visiting over 10k files  ([#&#8203;9523](astral-sh/uv#9523))

##### Configuration

-   Make `check-url` available in configuration files ([#&#8203;9032](astral-sh/uv#9032))

##### Performance

-   Avoid adding non-extra package with extra dependencies ([#&#8203;9540](astral-sh/uv#9540))
-   Avoid cloning `String` in marker evaluation ([#&#8203;9598](astral-sh/uv#9598))

##### Rust API

-   `uv-pep508`: Add more methods for simplifying `extra`-related expressions ([#&#8203;9469](astral-sh/uv#9469))

##### Bug fixes

-   Allow `file:` URLs to include package names ([#&#8203;9493](astral-sh/uv#9493))
-   Avoid using IDs across PubGrub states ([#&#8203;9538](astral-sh/uv#9538))
-   Consistently enforce requested-vs.-built metadata when retrieving wheels ([#&#8203;9484](astral-sh/uv#9484))
-   Do not show empty version specifier in `uv tool list` ([#&#8203;9605](astral-sh/uv#9605))
-   Include Git member information when getting metadata from cache ([#&#8203;9388](astral-sh/uv#9388))
-   Include base installation directory in uv run PATH ([#&#8203;9585](astral-sh/uv#9585))
-   Insert backslash when appending to system drive ([#&#8203;9488](astral-sh/uv#9488))
-   Normalize paths when lowering Git dependencies ([#&#8203;9595](astral-sh/uv#9595))
-   Omit origin when comparing requirements ([#&#8203;9570](astral-sh/uv#9570))
-   Override `manylinux_compatible` with `--python-platform` ([#&#8203;9526](astral-sh/uv#9526))
-   Pass extra when evaluating lockfile markers ([#&#8203;9539](astral-sh/uv#9539))
-   Propagate markers for recursive extras in resolver ([#&#8203;9509](astral-sh/uv#9509))
-   Respect path dependencies within Git dependencies ([#&#8203;9594](astral-sh/uv#9594))
-   Support recursive extras with marker in `pip compile -r pyproject.toml` ([#&#8203;9535](astral-sh/uv#9535))
-   Don't emit unpinned warning for proxy packages ([#&#8203;9497](astral-sh/uv#9497))
-   Fix `--refresh-package` flag mentioned as `--refresh-dependency` ([#&#8203;9486](astral-sh/uv#9486))
-   Handle Windows AV/EDR file locks during script installations ([#&#8203;9543](astral-sh/uv#9543))
-   Re-enable conflicting extra/group tests and fix regression from [#&#8203;9540](astral-sh/uv#9540) ([#&#8203;9582](astral-sh/uv#9582))

##### Documentation

-   Add missing word to docs for `run.md` ([#&#8203;9527](astral-sh/uv#9527))
-   Add policies reference section and license document ([#&#8203;9367](astral-sh/uv#9367))
-   Fix typo in entry point docs ([#&#8203;9491](astral-sh/uv#9491))
-   Fix up version in prior uninstall instructions ([#&#8203;9485](astral-sh/uv#9485))
-   Mention `uv pip` behavior in build system note ([#&#8203;9586](astral-sh/uv#9586))
-   Update build failures document ([#&#8203;9584](astral-sh/uv#9584))
-   Correct wording for multiple sources section ([#&#8203;9504](astral-sh/uv#9504))

</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
performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants