-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Keep track of retries in ManagedPythonDownload::fetch_with_retry
#14378
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
Conversation
| fn attempts(&self) -> u32 { | ||
| // Unfortunately different variants of `Error` track retry counts in different ways. We | ||
| // could consider unifying the variants we handle here in `Error::from_reqwest_middleware` | ||
| // instead, but both approaches will be fragile as new variants get added over time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fragility here makes me feel like I'm doing something wrong. @konstin do you have any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc streaming error can show up as different error types (e.g. Error::ExtractError), but we're only attaching the retries if we have a reqwest error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if opening the connection (the inner loop) retries a couple times and then ultimately succeeds, we'll never see those counts. Konsti and I talked about it and decided that it's not too big of a problem. It is confusing for folks reading the code and trying to understand how many retries are possible, but it's unlikely that it will bother any users in practice.
If/when we see #14171 again, this should clarify whether our retry logic was skipped (i.e. a transient error wasn't correctly identified as transient), or whether we exhausted our retries. Previously, if you ran a local example fileserver as in #14171 (comment) and then you tried to install Python from it, you'd get: ``` $ export UV_TEST_NO_CLI_PROGRESS=1 $ uv python install 3.8.20 --mirror http://localhost:8000 2>&1 | cat error: Failed to install cpython-3.8.20-linux-x86_64-gnu Caused by: Failed to extract archive: cpython-3.8.20-20241002-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz Caused by: failed to unpack `/home/jacko/.local/share/uv/python/.temp/.tmpS4sHHZ/python/lib/libpython3.8.so.1.0` Caused by: failed to unpack `python/lib/libpython3.8.so.1.0` into `/home/jacko/.local/share/uv/python/.temp/.tmpS4sHHZ/python/lib/libpython3.8.so.1.0` Caused by: error decoding response body Caused by: request or response body error Caused by: error reading a body from connection Caused by: Connection reset by peer (os error 104) ``` With this change you get: ``` error: Failed to install cpython-3.8.20-linux-x86_64-gnu Caused by: Request failed after 3 retries Caused by: Failed to extract archive: cpython-3.8.20-20241002-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz Caused by: failed to unpack `/home/jacko/.local/share/uv/python/.temp/.tmp4Ia24w/python/lib/libpython3.8.so.1.0` Caused by: failed to unpack `python/lib/libpython3.8.so.1.0` into `/home/jacko/.local/share/uv/python/.temp/.tmp4Ia24w/python/lib/libpython3.8.so.1.0` Caused by: error decoding response body Caused by: request or response body error Caused by: error reading a body from connection Caused by: Connection reset by peer (os error 104) ``` At the same time, I'm updating the way we handle the retry count to avoid nested retry loops exceeding the intended number of attempts, as I mentioned at #14069 (comment). It's not clear to me whether we actually want this part of the change, and I need feedback here.
3d9e3bc to
9225c3b
Compare
|
Note sure about this CI failure: Retrying failed a few times in a row. Gonna see if rebasing helps :) Edit: That worked. #codemonkeywritecode |
|
https://github.com/astral-sh/uv/actions/runs/16060816582/job/45326189810 includes this change and does not indicate that it retried. Does that make it the smoking gun for #14425 ? |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.7.16` -> `0.7.19` | 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.7.19`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0719) [Compare Source](astral-sh/uv@0.7.18...0.7.19) The **[uv build backend](https://docs.astral.sh/uv/concepts/build-backend/) is now stable**, and considered ready for production use. The uv build backend is a great choice for pure Python projects. It has reasonable defaults, with the goal of requiring zero configuration for most users, but provides flexible configuration to accommodate most Python project structures. It integrates tightly with uv, to improve messaging and user experience. It validates project metadata and structures, preventing common mistakes. And, finally, it's very fast — `uv sync` on a new project (from `uv init`) is 10-30x faster than with other build backends. To use uv as a build backend in an existing project, add `uv_build` to the `[build-system]` section in your `pyproject.toml`: ```toml [build-system] requires = ["uv_build>=0.7.19,<0.8.0"] build-backend = "uv_build" ``` In a future release, it will replace `hatchling` as the default in `uv init`. As before, uv will remain compatible with all standards-compliant build backends. ##### Python - Add PGO distributions of Python for aarch64 Linux, which are more optimized for better performance See the [python-build-standalone release](https://github.com/astral-sh/python-build-standalone/releases/tag/20250702) for more details. ##### Enhancements - Ignore Python patch version for `--universal` pip compile ([#​14405](astral-sh/uv#14405)) - Update the tilde version specifier warning to include more context ([#​14335](astral-sh/uv#14335)) - Clarify behavior and hint on tool install when no executables are available ([#​14423](astral-sh/uv#14423)) ##### Bug fixes - Make project and interpreter lock acquisition non-fatal ([#​14404](astral-sh/uv#14404)) - Includes `sys.prefix` in cached environment keys to avoid `--with` collisions across projects ([#​14403](astral-sh/uv#14403)) ##### Documentation - Add a migration guide from pip to uv projects ([#​12382](astral-sh/uv#12382)) ### [`v0.7.18`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0718) [Compare Source](astral-sh/uv@0.7.17...0.7.18) ##### Python - Added arm64 Windows Python 3.11, 3.12, 3.13, and 3.14 These are not downloaded by default, since x86-64 Python has broader ecosystem support on Windows. However, they can be requested with `cpython-<version>-windows-aarch64`. See the [python-build-standalone release](https://github.com/astral-sh/python-build-standalone/releases/tag/20250630) for more details. ##### Enhancements - Keep track of retries in `ManagedPythonDownload::fetch_with_retry` ([#​14378](astral-sh/uv#14378)) - Reuse build (virtual) environments across resolution and installation ([#​14338](astral-sh/uv#14338)) - Improve trace message for cached Python interpreter query ([#​14328](astral-sh/uv#14328)) - Use parsed URLs for conflicting URL error message ([#​14380](astral-sh/uv#14380)) ##### Preview features - Ignore invalid build backend settings when not building ([#​14372](astral-sh/uv#14372)) ##### Bug fixes - Fix equals-star and tilde-equals with `python_version` and `python_full_version` ([#​14271](astral-sh/uv#14271)) - Include the canonical path in the interpreter query cache key ([#​14331](astral-sh/uv#14331)) - Only drop build directories on program exit ([#​14304](astral-sh/uv#14304)) - Error instead of panic on conflict between global and subcommand flags ([#​14368](astral-sh/uv#14368)) - Consistently normalize trailing slashes on URLs with no path segments ([#​14349](astral-sh/uv#14349)) ##### Documentation - Add instructions for publishing to JFrog's Artifactory ([#​14253](astral-sh/uv#14253)) - Edits to the build backend documentation ([#​14376](astral-sh/uv#14376)) ### [`v0.7.17`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0717) [Compare Source](astral-sh/uv@0.7.16...0.7.17) ##### Bug fixes - Apply build constraints when resolving `--with` dependencies ([#​14340](astral-sh/uv#14340)) - Drop trailing slashes when converting index URL from URL ([#​14346](astral-sh/uv#14346)) - Ignore `UV_PYTHON_CACHE_DIR` when empty ([#​14336](astral-sh/uv#14336)) - Fix error message ordering for `pyvenv.cfg` version conflict ([#​14329](astral-sh/uv#14329)) </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:eyJjcmVhdGVkSW5WZXIiOiI0MC42Mi4xIiwidXBkYXRlZEluVmVyIjoiNDAuNjIuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
If/when we see #14171 again, this should clarify whether our retry logic was skipped (i.e. a transient error wasn't correctly identified as transient), or whether we exhausted our retries. Previously, if you ran a local example fileserver as in #14171 (comment) and then you tried to install Python from it, you'd get:
With this change you get:
At the same time, I'm updating the way we handle the retry count to avoid nested retry loops exceeding the intended number of attempts, as I mentioned at #14069 (comment). It's not clear to me whether we actually want this part of the change, and I need feedback here.