astral-sh / uv

An extremely fast Python package and project manager, written in Rust.
https://docs.astral.sh/uv
Apache License 2.0
25.87k stars 755 forks source link

Is the brotli feature of the reqwest crate actually used/useful in uv? #5283

Closed musicinmybrain closed 2 months ago

musicinmybrain commented 3 months ago

The maintainer (@decathorpe) of the rust-reqwest package in Fedora, and of much of the Rust stack in Fedora, “explicitly dropped the brotli feature [from the Fedora package] because the dependencies might not be available in the future. There have been multiple breaking releases of the brotli and brotli-decompressor crates recently, and dropbox upstream is ignoring any pull requests / issues we've filed since those crates were packaged for Fedora.” The maintainer is therefore trying to reduce dependencies on the https://github.com/dropbox/rust-brotli crates.

This prompted me to investigate how the brotli feature was used in uv, to decide if I could easily patch it out downstream and understand what, if anything I might lose by doing so.

What I found was that the feature was originally enabled in 8032d4606e24f15c97d66a9a58a1a7fe854d691d along with other work relating to JSON metadata. I presume that the idea was that it might speed up metadata operations a bit, since in general brotli does compress a little more effectively and decompress with a little less CPU effort than gzip on text content.

The same commit added only Accept-Encoding: gzip to the PypiClient’s request headers, and there are still a couple of .header("Accept-Encoding", "gzip") in the uv-client crate in the current main, 5a23f0579962da26dbf117ca008822c26f5d15e5. There are no explicit references to brotli, nor Accept-Encoding headers that contain br, in the code. However, https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.brotli indicates that, when the brotli feature is enabled, br will be inserted to the AcceptEncoding for any requests that don’t already have an explicit AcceptEncoding header. I assume that there are some cases in which uv makes such requests, although possibly not in uv-client. I also note (using Firefox devtools) that PyPI does not currently appear to compress responses to requests for JSON metadata, e.g. https://pypi.org/pypi/uv/json.

All of this leads me to ask – is it worthwhile to leave this feature enabled? Have I missed a way in which it is significantly useful? Or would it make sense to drop it,

diff --git a/Cargo.toml b/Cargo.toml
index 5a854a41..439d0300 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -117,7 +117,7 @@ quote = { version = "1.0.36" }
 rayon = { version = "1.8.0" }
 reflink-copy = { version = "0.1.15" }
 regex = { version = "1.10.2" }
-reqwest = { version = "0.12.3", default-features = false, features = ["json", "gzip", "brotli", "stream", "rustls-tls", "rustls-tls-native-roots"] }
+reqwest = { version = "0.12.3", default-features = false, features = ["json", "gzip", "stream", "rustls-tls", "rustls-tls-native-roots"] }
 reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
 reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
 rkyv = { version = "0.7.43", features = ["strict", "validation"] }

as it looks like I will be doing downstream in Fedora either way, and thereby remove the crates alloc-no-stdlib, alloc-stdlib, brotli, and brotli-decompressor from uv’s dependency tree? If so, I can easily open a PR.

Mentioning @charliermarsh as the person who originally enabled the crate feature, and who might therefore have an opinion on its ongoing potential usefulness in uv.

musicinmybrain commented 2 months ago

Given that the brotli feature was removed from the reqwest crate dependency in 2c5cc62106d88d8a0a94f6f91e89887433ba0da2 (released in 0.4.0), I’m going to consider this question answered as “no, it’s not actually needed.”

https://github.com/astral-sh/uv/commit/2c5cc62106d88d8a0a94f6f91e89887433ba0da2#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542L122

zanieb commented 2 months ago

Sorry about the lack of response!

musicinmybrain commented 2 months ago

No problem – glad to have closure one way or the other!