uv + maturin migration #768
Conversation
dandavison
force-pushed
the
maturin
branch
2 times, most recently
from
4fe5399 to
f864109
Compare
dandavison
force-pushed
the
maturin
branch
5 times, most recently
from
545d5f4 to
79e448a
Compare
|
|
||
| With the prerequisites installed, first clone the SDK repository recursively: | ||
| ```bash | ||
| uv tool install poethepoet |
There was a problem hiding this comment.
We'll probably switch away from poe when uv includes a task runner.
.github/workflows/build-binaries.yml
Outdated
| # Simple test | ||
| - run: poe test-dist-single | ||
| # Run a test | ||
| - run: uv run pytest -k test_activity_hello |
There was a problem hiding this comment.
The goal of the original form of this was to do a literal pip install of the built wheel to confirm it works on the platform. Can we do some form of pip install on this step? Maybe in a completely new folder as a smoke test? Could even be a separate job with its own matrix. Just want to confirm the built wheel works on every platform.
There was a problem hiding this comment.
Thanks, I've modified it to be like
- name: Test wheel
shell: bash
run: |
mkdir __test_wheel__
cd __test_wheel__
cp -r ../tests .
python -m venv .venv
bindir=bin
if [ "$RUNNER_OS" = "Windows" ]; then
bindir=Scripts
fi
./.venv/$bindir/pip install 'protobuf>=3.20' 'types-protobuf>=3.20' 'typing-extensions<5,>=4.2.0' pytest pytest_asyncio grpcio pydantic opentelemetry-api opentelemetry-sdk python-dateutil
./.venv/$bindir/pip install --no-index --find-links=../dist temporalio
./.venv/$bindir/python -m pytest -s -k test_workflow_hello(With test_activity_hello the worker wasn't polling the server; I haven't looked into why.)
| - run: python -m pip install --upgrade wheel poetry poethepoet | ||
| - run: poetry install --no-root --all-extras | ||
| - uses: astral-sh/setup-uv@v5 | ||
| - run: uv tool install poethepoet |
There was a problem hiding this comment.
Can we get rid of poe? Does uv offer simple ways to define tasks?
There was a problem hiding this comment.
Was just commenting on the same thing -- we'll be able to switch away from poe when uv includes a task runner, which it sounds to me like will happen in next few months.
MANIFEST.in
Outdated
| ruff = "^0.5.0" | ||
| [dependency-groups] | ||
| dev = [ | ||
| "cibuildwheel>=2.22.0,<3", |
There was a problem hiding this comment.
Do we need cibuildwheel anymore or can we use maturin to build the wheels?
There was a problem hiding this comment.
This is an area I'm unsure about also. I should have left a TODO. uv build builds the wheel in dist/, and it invokes maturin to do so (invoking maturin build directly builds the wheel in temporalio/bridge).
I am unsure exactly how cibuildwheel works. It must know how to build the Rust extension; I think that means it refers to the [build-system] in pyproject.toml, which leads it to invoke maturin? So, a bit of reasearch needed and also obviously need to verify the wheels built in CI are functioning correctly.
There was a problem hiding this comment.
cibuildwheel is a fairly common approach to building multiplatform wheels in docker images in CI. Yes, it does a traditional Python build I guess with the build system. But maturin says they offer most of this (ref https://www.maturin.rs/distribution.html#build-wheels and https://github.com/PyO3/maturin-action), but I haven't tested. I'm ok staying with cibuildwheel too.
Can you temporarily enable the build-binaries CI job in this PR so we can see the wheels it builds?
There was a problem hiding this comment.
Yes, maybe I'll do it incrementally. I'll probably move in the maturin direction. This GitHub workflow from pydantic-core is probably a reasonable model to consult: https://github.com/pydantic/pydantic-core/blob/main/.github/workflows/ci.yml#L480-L487
Without this,
File "/Users/dan/src/temporalio/sdk-python/.venv/lib/python3.13/site-packages/setuptools/config/pyprojecttoml.py", line 55, in validate
raise ValueError(f"{error}\n{summary}") from None
ValueError: invalid pyproject.toml config: `project.license`.
configuration error: `project.license` must be valid exactly by one definition (2 matches found):
- keys:
'file': {type: string}
required: ['file']
- keys:
'text': {type: string}
required: ['text']
Don't understand why they are showing on this branch only under same versions as main: Python 3.13.1 mypy 1.4.1 (compiled: no)
…dk_bridge/" This reverts commit c62cf00.
dandavison
force-pushed
the
maturin
branch
6 times, most recently
from
bf16fa7 to
8cf4712
Compare
This reverts commit 83a82ef.
Switch from poetry to uv, and use maturin to build the rust extension.
See features repo migration: temporalio/features#600
[For merge after next SDK release]
TODO:
fix-wheelhas been removed; confirm that none of those hacks are needed any longer