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

Implement --index-strategy unsafe-best-match #3138

Merged
merged 14 commits into from
Apr 27, 2024

Conversation

yorickvP
Copy link
Contributor

Summary

This index strategy resolves every package to the latest possible version across indexes. If a version is in multiple indexes, the first available index is selected.

Implements #3137

This closely matches pip.

Test Plan

Good question. I'm hesitant to use my certifi example here, since that would inevitably break when torch removes this package. Please comment!

@zanieb
Copy link
Member

zanieb commented Apr 19, 2024

I don't mind adding this behavior, but we'll need to reach consensus as a team. Thanks for contributing!

@charliermarsh
Copy link
Member

I suppose I'm open to the behavior itself, but I think the current behavior will be quadratic? Since for every version we select, we're now iterating over all versions to create the merged version map.

@charliermarsh charliermarsh added the compatibility Compatibility with a specification or another tool label Apr 20, 2024
@sergeykolosov
Copy link
Contributor

In use-cases of my concern, this one is the last missing piece to be able to switch to uv.

There's ≈10 internal indexes with pretty much every possible package contained in more than one of those, versions randomly scattered across indexes (so currently the --index-strategy unsafe-any-match is being used).

So for a requirement of some-internal-package>=0.1.0 in every production environment previously provisioned by pip install -U, after running uv pip install -U --index-strategy unsafe-any-match both the package and its dependencies get downgraded to some “random”-ish versions found in the available indexes:

Installed 3 packages in 5ms
 - other-internal-package==0.9.0.post2
 + other-internal-package==0.9.0a1
 - six==1.16.0
 + six==1.11.0
 - some-internal-package==0.1.22
 + some-internal-package==0.1.20

@charliermarsh
Copy link
Member

I need to think on how to implement this.

@charliermarsh
Copy link
Member

I'm working on this now.

@charliermarsh charliermarsh changed the title Implement --index-strategy unsafe-highest Implement --index-strategy unsafe-closest-match Apr 26, 2024
@charliermarsh
Copy link
Member

Okay, this seems reasonable to me now. Assuming we consider the number of indexes to be a small constant factor, this should still be linear.

@charliermarsh
Copy link
Member

Adding some other reviewers.


Some(next)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we could make this generic but I kind of gave up. It felt like a lot of work for minimal gain. (It might be nice to have one iterator that can take Version or Reverse<Version>, but that again felt like a lot of work to save ~30 lines of code. Open to it though!)

@konstin
Copy link
Member

konstin commented Apr 26, 2024

Benchmarks:

Jupyter, an easy but very common use case:

Benchmark 1: target/profiling/uv-branch pip compile scripts/requirements/jupyter.in
  Time (mean ± σ):      14.0 ms ±   0.6 ms    [User: 14.4 ms, System: 16.0 ms]
  Range (min … max):    12.6 ms …  16.4 ms    202 runs
 
Benchmark 2: target/profiling/uv-branch pip compile scripts/requirements/jupyter.in --index-strategy unsafe-closest-match
  Time (mean ± σ):      14.0 ms ±   0.7 ms    [User: 14.2 ms, System: 16.2 ms]
  Range (min … max):    12.5 ms …  17.5 ms    211 runs
 
Summary
  target/profiling/uv-branch pip compile scripts/requirements/jupyter.in --index-strategy unsafe-closest-match ran
    1.00 ± 0.07 times faster than target/profiling/uv-branch pip compile scripts/requirements/jupyter.in

Airflow with all extra, a hard case with a bunch of backtracking:

Benchmark 1: target/profiling/uv-branch pip compile airflow.in
  Time (mean ± σ):     153.4 ms ±   2.8 ms    [User: 142.2 ms, System: 129.7 ms]
  Range (min … max):   149.1 ms … 159.5 ms    19 runs
 
Benchmark 2: target/profiling/uv-branch pip compile airflow.in --index-strategy unsafe-closest-match
  Time (mean ± σ):     153.1 ms ±   2.2 ms    [User: 141.6 ms, System: 132.6 ms]
  Range (min … max):   148.4 ms … 158.0 ms    19 runs
 
Summary
  target/profiling/uv-branch pip compile airflow.in --index-strategy unsafe-closest-match ran
    1.00 ± 0.02 times faster than target/profiling/uv-branch pip compile airflow.in

Boto3, the hardest pathological use case we have user hit:

Benchmark 1: target/profiling/uv-branch pip compile scripts/requirements/boto3.in
  Time (mean ± σ):     359.2 ms ±   1.6 ms    [User: 316.3 ms, System: 54.2 ms]
  Range (min … max):   357.5 ms … 362.3 ms    10 runs
 
Benchmark 2: target/profiling/uv-branch pip compile scripts/requirements/boto3.in --index-strategy unsafe-closest-match
  Time (mean ± σ):     391.2 ms ±   3.4 ms    [User: 358.5 ms, System: 43.9 ms]
  Range (min … max):   387.1 ms … 396.4 ms    10 runs
 
Summary
  target/profiling/uv-branch pip compile scripts/requirements/boto3.in ran
    1.09 ± 0.01 times faster than target/profiling/uv-branch pip compile scripts/requirements/boto3.in --index-strategy unsafe-closest-match

This looks much better than i had expected

@charliermarsh
Copy link
Member

charliermarsh commented Apr 26, 2024

@zanieb @BurntSushi - any feedback on the name of the option, the docs, exposing this? (As opposed to the implementation which seems ok.)

@zanieb
Copy link
Member

zanieb commented Apr 26, 2024

+1 to exposing, I can review the docs too.

For naming, we have...

  • First match: Only versions from the first index with the package name are considered (makes sense to me)
  • Any match: Versions from the indexes are exhausted in-order (not very clear?)
  • Closest match: The version is taken from the index with the closest version (makes sense to me)

I think this name makes sense in the context of the others. Although "any match" confuses the naming scheme a bit.

@BurntSushi
Copy link
Member

Yeah this LGTM. Another possible name is unsafe-best-match. In particular, the docs use the word "best" instead of "closest" to describe its behavior. I don't have a strong opinion on what the right word to choose here, but I loosely prefer "best." In my view, "closest" raises the question of "closest to what?" Where as "best" I think can be cast as "best matching version for the chosen resolution strategy." But I think this is probably a quibble.

@charliermarsh
Copy link
Member

What about...

  • single-index (only consider versions from a single index for each package)
  • unsafe-first-match (use the first index with a compatible version)
  • unsafe-best-match (use the index with the "best" version)

@zanieb
Copy link
Member

zanieb commented Apr 26, 2024

That's an improvement.

Should single-index be first-index — i.e. use the first index with the package regardless of whether or not a version match exists?

@charliermarsh
Copy link
Member

I will make these changes (and add aliases for the existing values), revisit the merge implementation, then ship it.

@charliermarsh charliermarsh force-pushed the yorickvp/unsafe-any-match branch 3 times, most recently from c7c5a2b to 5e9bd0e Compare April 27, 2024 01:07
@charliermarsh charliermarsh enabled auto-merge (squash) April 27, 2024 01:19
@charliermarsh charliermarsh changed the title Implement --index-strategy unsafe-closest-match Implement --index-strategy unsafe-best-match Apr 27, 2024
@charliermarsh charliermarsh merged commit 43181f1 into astral-sh:main Apr 27, 2024
29 checks passed
@zanieb
Copy link
Member

zanieb commented Apr 27, 2024

Thanks @yorickvP !

sergeykolosov added a commit to sergeykolosov/uv that referenced this pull request Apr 27, 2024
Dropped the `--` prefix from the values of the `--index-strategy` option
mistakenly added to documentation in astral-sh#3138.
charliermarsh pushed a commit that referenced this pull request Apr 27, 2024
## Summary

Dropped the `--` prefix from the values of the `--index-strategy` option
mistakenly added to documentation in #3138.

## Test Plan

Verified that actually accepted values of `--index-strategy` don't use a
`--` prefix.
@yorickvP
Copy link
Contributor Author

Looking closer, the behaviour implemented in the PR is subtly different from the behaviour I intended. I bisected it to the kmerge_by commit. It seems to be picking the package from pypi instead of my index if they have the same version. In this case, the pypi package fails to build.

$ git checkout 67b8389aa7579befaa7e6cc64afba58b89d9556b
$ cargo build
$ echo 'tensorrt-llm==0.11.0' | ./target/debug/uv pip compile - --extra-index-url https://pypi.nvidia.com --python-version=3.10 --index-strategy=unsafe-best-match --annotation-style=line
[..]
tensorrt-llm==0.11.0

vs

$ git checkout 275f1503721f4a04639279d6f065de86c5075c6e
$ cargo build
$ echo 'tensorrt-llm==0.11.0' | ./target/debug/uv pip compile - --extra-index-url https://pypi.nvidia.com --python-version=3.10 --index-strategy=unsafe-best-match --annotation-style=line
error: Failed to download and build `tensorrt-llm==0.11.0`
  Caused by: Failed to build: `tensorrt-llm==0.11.0`
  Caused by: Build backend failed to build wheel through `build_wheel()` with exit status: 1
--- stdout:

--- stderr:
Traceback (most recent call last):
  File "<string>", line 11, in <module>
  File "/home/yorick/.cache/uv/.tmpZbNSch/.venv/lib/python3.11/site-packages/nvidia_stub/buildapi.py", line 29, in build_wheel
    return download_wheel(pathlib.Path(wheel_directory), config_settings)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/yorick/.cache/uv/.tmpZbNSch/.venv/lib/python3.11/site-packages/nvidia_stub/wheel.py", line 175, in download_wheel
    report_install_failure(distribution, version, None)
  File "/home/yorick/.cache/uv/.tmpZbNSch/.venv/lib/python3.11/site-packages/nvidia_stub/error.py", line 63, in report_install_failure
    raise InstallFailedError(
nvidia_stub.error.InstallFailedError:
*******************************************************************************

The installation of tensorrt-llm for version 0.11.0 failed.

This is a special placeholder package which downloads a real wheel package
from https://pypi.nvidia.com. If https://pypi.nvidia.com is not reachable, we
cannot download the real wheel file to install.

@charliermarsh
Copy link
Member

Sounds like a problem with breaking ties / stable ordering. Want to open an issue? Interested in working on it?

@yorickvP
Copy link
Contributor Author

#5288

charliermarsh pushed a commit that referenced this pull request Jul 22, 2024
)

This fixes resolving packages that publish an invalid stub to pypi, such
as tensorrt-llm.

## Summary

In #3138 , we implemented
`unsafe-best-match`. However, it seems to not quite work as expected.
When multiple indices contain the same version, it's not clear which
index the current code uses. This PR fixes that to use the first index
the package is in.

## Test Plan

```console
$ echo 'tensorrt-llm==0.11.0' | ./target/debug/uv pip compile - --extra-index-url https://pypi.nvidia.com --python-version=3.10 --index-strategy=unsafe-best-match --annotation-style=line
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with a specification or another tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants