Skip to content

Conversation

@charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Jan 7, 2026

Summary

For pyx, we can allow uploads that bypass the registry and send the file directly to S3. This is an opt-in feature, enabled via the --direct flag.

@charliermarsh charliermarsh added the preview Experimental behavior label Jan 7, 2026
})?;

// Use a raw reqwest client for S3 (no auth middleware).
let s3_response = client
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a streaming request, does it need to have an outer retry wrapper?

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

This looks reasonable though @woodruffw or @konstin should be final reviewers.

&form_metadata,
&publish_url,
&upload_client,
&oidc_client,
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should rename this client

)
})?;

let s3_response = s3_client
Copy link
Member

Choose a reason for hiding this comment

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

That's the client with the regular settings wrt to retries and timeouts, and with no auth middleware

};

debug!(
"Got pre-signed URL for upload: {}",
Copy link
Member

Choose a reason for hiding this comment

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

Is that URL confidential, should we redact it in logs?

let reader = ProgressReader::new(file, move |read| {
reporter_clone.on_upload_progress(idx, read as u64);
});
let file_reader = Body::wrap_stream(ReaderStream::new(reader));
Copy link
Member

Choose a reason for hiding this comment

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

We need a retry loop here, streaming uploads can't be retried otherwise

let reserve_request = build_metadata_request(
&group.raw_filename,
&reserve_url,
client,
Copy link
Member

Choose a reason for hiding this comment

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

That's the client without retries, I think that should be the regular client, plus/minus auth settings?

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM, one note and one comment but neither is blocking IMO. I'll follow up with some comments on the pyx side too.

Comment on lines +7590 to +7595
/// Use direct upload to the registry.
///
/// When enabled, the publish command will use a direct two-phase upload protocol
/// that uploads files directly to storage, bypassing the registry's upload endpoint.
#[arg(long, hide = true)]
pub direct: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable to me, although noting that we could in principle do this without a flag by having uv send an Accept: ... header that indicates support, to which pyx would then respond with the pre-signed URL.

(That might be nice for enabling this implicit/by default in the future, but seems like not worth doing initially!)

let status = response.status();

let reserve_response: ReserveResponse = match status {
StatusCode::OK => {
Copy link
Member

Choose a reason for hiding this comment

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

Q, minor: should we use HTTP 409 (Conflict) for this, given that we're not tied to PyPI's response code semantics for our own upload scheme?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would 409 be for the case in which it already exists and isn't the same file? Or already exists and is the same file? Here I'm using 200 for "already exists and is the same" (and 201 for "created, now proceed to upload").

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good point. 409 would be for "exists but with different content" I believe, 200 is semantically correct for "exists with the same content already."

@charliermarsh charliermarsh marked this pull request as ready for review January 8, 2026 17:03
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 8, 2026

Merging this PR will not alter performance

Summary

✅ 5 untouched benchmarks


Comparing charlie/direct-s3 (fa48051) with main (25d691e)

Open in CodSpeed

@charliermarsh charliermarsh disabled auto-merge January 9, 2026 02:26
@charliermarsh charliermarsh merged commit 85dedc8 into main Jan 9, 2026
100 of 102 checks passed
@charliermarsh charliermarsh deleted the charlie/direct-s3 branch January 9, 2026 02:26
let oidc_client = client_builder
let unauthenticated_client = client_builder
.clone()
.retries(0)
Copy link
Member

Choose a reason for hiding this comment

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

For the OIDC client, we need retries (GitHub's networking is too unreliable) and ideally the default timeouts, the S3 is separate in that it uses its own retry loop, upload timeouts and no auth middleware.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Jan 12, 2026
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.9.22` → `0.9.24` |

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

[Compare Source](astral-sh/uv@0.9.23...0.9.24)

Released on 2026-01-09.

##### Bug fixes

- Fix handling of `UV_NO_SYNC=1 uv run ...` ([#&#8203;17391](astral-sh/uv#17391))
- Rebuild dynamic distribution when version changes with `--no-cache` ([#&#8203;17387](astral-sh/uv#17387))

##### Documentation

- Add Rust language classifier ([#&#8203;17389](astral-sh/uv#17389))

### [`v0.9.23`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0923)

[Compare Source](astral-sh/uv@0.9.22...0.9.23)

Released on 2026-01-09.

##### Enhancements

- Only write portable paths in `RECORD` files ([#&#8203;17339](astral-sh/uv#17339))
- Support relative paths in `UV_PYTHON_BIN_DIR` and `UV_TOOL_BIN_DIR` ([#&#8203;17367](astral-sh/uv#17367))

##### Preview features

- Enable uploads to S3 via pre-signed URLs ([#&#8203;17349](astral-sh/uv#17349))

##### Configuration

- Allow setting proxy variables via global / user configuration ([#&#8203;16918](astral-sh/uv#16918))
- Manually parse and reconcile Boolean environment variables ([#&#8203;17321](astral-sh/uv#17321))

##### Bug fixes

- Avoid broken build artifacts on build failure ([#&#8203;17276](astral-sh/uv#17276))
- Fix missing dependencies on synthetic root in SBOM export ([#&#8203;17363](astral-sh/uv#17363))
- Recognize `armv8l` as an alias for `armv7l` in platform tag parsing ([#&#8203;17384](astral-sh/uv#17384))
- Fix redaction of a URL in a middleware trace log ([#&#8203;17346](astral-sh/uv#17346))

##### Documentation

- Add `index.md` suggestion to `llms.txt` ([#&#8203;17362](astral-sh/uv#17362))
- Clarify that `uv run` uses inexact syncing by default ([#&#8203;17366](astral-sh/uv#17366))

</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:eyJjcmVhdGVkSW5WZXIiOiI0Mi43NS4xIiwidXBkYXRlZEluVmVyIjoiNDIuNzUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6OnBhdGNoIl19-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Experimental behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants