Skip to content

Conversation

@rdesgroppes
Copy link
Contributor

@rdesgroppes rdesgroppes commented Oct 21, 2025

Motivation

This is a direct follow-up of:

... where a review comment rightfully suggested that:

Instead, we should run a normal single-arch Python distribution where everything will just work by default and we can remove the renaming logic. The source of distributions should be the python-build-standalone project which the makers of UV recently adopted. They are used by everyone nowadays, even rules_python.

What does this PR do?

The present change aims at embracing the above-recommended approach, which indeed looks simpler because the need for bundling targeted architectures is then addressed without additional environment variables, while still allowing delocate_wheel to verify architecture requirements.

Also, the PBS distribution happens to be pretty lightweight (16MB vs 71MB).

JSGette
JSGette previously approved these changes Oct 22, 2025
Copy link
Contributor

@JSGette JSGette left a comment

Choose a reason for hiding this comment

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

LGTM

@JSGette JSGette dismissed their stale review October 22, 2025 08:13

Wrong PR approved

@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/require-archs-on-macos-with-ofek-recommendation branch from a2711b7 to 77d35ca Compare October 22, 2025 21:13
@rdesgroppes rdesgroppes changed the title [WIP] Switch to "Python Build Standalone" Switch to Python Build Standalone for macOS wheels Oct 22, 2025
@rdesgroppes rdesgroppes added the qa/skip-qa Automatically skip this PR for the next QA label Oct 22, 2025
@rdesgroppes rdesgroppes marked this pull request as ready for review October 22, 2025 21:19
@rdesgroppes rdesgroppes requested review from a team as code owners October 22, 2025 21:19
@rdesgroppes rdesgroppes requested a review from ofek October 22, 2025 21:19
@rdesgroppes rdesgroppes requested a review from JSGette October 22, 2025 21:21
@iliakur iliakur self-assigned this Oct 27, 2025
iliakur
iliakur previously approved these changes Oct 27, 2025
Copy link
Contributor

@iliakur iliakur left a comment

Choose a reason for hiding this comment

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

just one comment, lgtm otherwise

@aiuto
Copy link

aiuto commented Oct 27, 2025

Good timing for this PR. We need to switch to a hermetic python for bazel.

I am also sort of curious why we do this at all (for omnibus) when all the build machines and dev images have python 3.12 installed. But answering that should not block the this

@rdesgroppes
Copy link
Contributor Author

I am also sort of curious why we do this at all (for omnibus) when all the build machines and dev images have python 3.12 installed. But answering that should not block the this

Looking at #21160, I guess the question would be better answered by @Kyle-Neale?

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Looks good to me pending removal of the Claude comment!

This is a follow-up of:
- #21692

... where a review comment rightfully suggested that:
> Instead, we should run a normal single-arch Python distribution where
> everything will just work by default and we can remove the renaming
> logic. The source of distributions should be the
> [python-build-standalone](https://github.com/astral-sh/python-build-standalone)
> project which the makers of UV recently adopted. They are used by
> everyone nowadays, even
> [rules_python](https://github.com/bazel-contrib/rules_python/blob/cf594f780c91f13d48e77faad34df48ac57398da/python/versions.bzl#L29).

The present change aims at embracing the above-recommended approach,
which looks indeed promising because the need for bundling targeted
architectures is then addressed without additional environment
variables, while still allowing `delocate_wheel` to verify architecture
requirements.

Also, the PBS distribution happens to be pretty lightweight (16MB vs
71MB).
This is to address:
> n00b claude user, does this need to stay in the source code?
(#21716 (comment))
> Looks good to me pending removal of the Claude comment!
(#21716 (review))
@rdesgroppes rdesgroppes force-pushed the regis.desgroppes/require-archs-on-macos-with-ofek-recommendation branch from 77d35ca to a4238c3 Compare October 27, 2025 14:08
@temporal-github-worker-1 temporal-github-worker-1 bot dismissed iliakur’s stale review October 27, 2025 14:08

Review from iliakur is dismissed. Related teams and files:

  • agent-integrations
    • .github/workflows/resolve-build-deps.yaml
@rdesgroppes
Copy link
Contributor Author

Looks good to me pending removal of the Claude comment!

Done in second commit (a4238c3).

@rdesgroppes rdesgroppes enabled auto-merge October 27, 2025 19:58
@rdesgroppes rdesgroppes requested review from a team and removed request for JSGette October 27, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants