chore: swap build backend from setuptools to maturin - #975
Conversation
Replace the setuptools build backend with maturin so the project builds as a mixed Rust/Python package. This is the backend swap only — the Rust side is an empty placeholder module (src.rs, importable as `from src import rs`) that real functionality will fill in via a follow-up. - pyproject.toml: build-system -> maturin; drop [tool.setuptools.packages.find]; add [tool.maturin] (module-name = "src.rs", manifest-path = "rust/Cargo.toml"); add maturin to the dev dependency group. - rust/: new Cargo.toml + src/lib.rs stub (empty #[pymodule] `rs`, pyo3 abi3). Rust source lives under rust/ to avoid colliding with the Python src/ dir. - .gitignore: ignore rust/target/ and the maturin-develop .so/.pyd/.dylib. - CI/Docker: install a Rust toolchain (dtolnay/rust-toolchain in test.yml, apt cargo in Dockerfile) since every `uv sync` now compiles via cargo. - uv.lock + rust/Cargo.lock regenerated. The importable package stays `src`; nothing under src/ moves or renames, and all console scripts and subpackages are preserved (verified: 402 unit tests pass; wheel bundles src/* plus src/rs.abi3.so).
SkyeAv
added
enhancement
|
This would allow me to expose rust code directly to python. This backend is the base commit I need before I can start refactoring in that direction |
SkyeAv
added
the
structural
astral-sh/ruff-action@v3 floats to the latest ruff, now 0.16.0 (pyproject pins only a floor, >=0.14.14). Ruff 0.16.0 formats Python code fences inside *.md, which overlaps with the dedicated rumdl Markdown check and was failing 'Check Python formatting with ruff' on every open PR. Exclude *.md from ruff format so ruff stays scoped to Python and Markdown formatting CI is deterministic across ruff releases.
There was a problem hiding this comment.
Pull request overview
This PR switches Babel’s packaging/build backend from setuptools to maturin so the project can build as a mixed Python/Rust distribution, while keeping the importable Python package layout unchanged (src remains the top-level package). It adds a minimal Rust/PyO3 stub extension to satisfy maturin’s requirements and updates CI/Docker to include a Rust toolchain for builds.
Changes:
- Swap PEP 517 build backend to
maturinand configure it to bundle the existingsrcPython package while emitting an extension module atsrc/rs.*. - Add a placeholder Rust crate (
rust/) with an empty PyO3#[pymodule]importable asfrom src import rs. - Update CI and Docker build prerequisites to install Rust, and add ignores/locks for new build artifacts and dependencies.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Switch build backend to maturin; configure [tool.maturin]; add dev dependency; adjust ruff formatter config. |
uv.lock |
Record maturin in the dev dependency set and lock resolution. |
rust/Cargo.toml |
Define the Rust crate and PyO3 dependency for the placeholder extension module. |
rust/src/lib.rs |
Add the empty #[pymodule] fn rs stub importable from Python. |
rust/Cargo.lock |
Commit the Rust dependency lockfile for reproducible cargo builds. |
.github/workflows/test.yml |
Install a Rust toolchain in CI before uv sync so maturin/cargo builds can run. |
Dockerfile |
Install cargo in the container image so uv sync can build the maturin extension. |
.gitignore |
Ignore Rust build outputs and local maturin develop-produced extension binaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Renames the module from `rs` to `_accel`: the name should say what it is (a
compiled accelerator) rather than what it is written in, and while the module
is empty this rename is as cheap as it will ever be.
Adds `src/accel.py` as the only thing that imports the compiled module, because
the two failure modes need opposite handling and neither is obvious at the call
site:
- A *missing* extension falls back to Python and logs at INFO. Every
snakefile does a top-level `import src.foo` at DAG-parse time, so raising
would take down all 245 rules for a contributor without a Rust toolchain,
for a reviewer, and for a fork's CI -- not just the rules that would have
used Rust. AGENTS.md's "a log warning is not a control" is about wrong
output; a slower path emitting identical bytes is not that.
- A *stale* extension raises. The extension is installed editable, so the
compiled artifact sits in the checkout at src/_accel.*.so and a `git pull`
does not rebuild it. ABI_VERSION in rust/src/lib.rs is compared against
_REQUIRED_ABI_VERSION at import -- i.e. at DAG-parse time -- so a stale
build fails in the first second rather than partway through a 12h rule.
The check is a separate function so it can be tested against a stub;
building a genuinely stale extension inside a unit test would mean
compiling Rust.
BABEL_DISABLE_RUST=1 forces the Python path, which is what makes an A/B
measurement possible in one checkout. An environment variable rather than a
config.yaml entry: which of two byte-identical implementations runs has no
user-facing meaning, and config.yaml is threaded into Snakemake params and
output paths where changing it could perturb a running DAG. Precedent is
BABEL_DUCKDB_TEMP_DIR.
Also: rust/Cargo.toml's version becomes 0.0.0 (maturin takes the distribution
version from pyproject.toml -- the wheel is still babel_pipeline-1.17-cp311-abi3
-- and nothing publishes this crate, so a second version number is only a thing
to forget to bump); rust-version declares pyo3 0.23's 1.63 floor;
rust-toolchain.toml pins the channel; and `[tool.maturin] exclude` stops the
four per-directory CLAUDE.md agent-instruction files shipping to anyone who
installs the wheel. Verified by rebuilding: the wheel now holds src/_accel.abi3.so,
src/_accel.pyi, 98 .py files and the 19 .snakefiles, and no CLAUDE.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Since the maturin swap, `[tool.uv] package = true` means every `uv run` and
`uv sync` builds the project, and building the project invokes cargo. Three
places had not caught up:
- check-formatting.yml's snakefmt job runs `uv run`, with no toolchain step.
It would have passed by accident, because ubuntu-latest happens to ship
Rust -- which is worse than failing, since it makes a lint job depend on
the runner image. Fixed by not building at all: snakefmt does not need
babel-pipeline installed, so `uv run --no-project --with snakefmt`.
- The Dockerfile installed Debian bookworm's cargo, which is 1.63 -- exactly
pyo3 0.23's minimum. It builds today and breaks on the next pyo3 bump with
an error that reads as unrelated. Swapped for rustup, which also honours
rust-toolchain.toml (apt's cargo ignores it).
- Nothing in slurm/, kubernetes/ or docs/ mentioned Rust at all, while
`uv sync --frozen` on the Hatteras login node now fails outright without a
toolchain -- before Snakemake starts. slurm/README.md now says so, and
notes that uv's own fallback downloads ~600 MB into a cache directory the
UV_CACHE_DIR override in run-babel-on-slurm.sh does not cover.
Adds a cargo fmt/clippy job mirroring the one-linter-per-job structure of the
other three, so "all four linters checked in CI" does not quietly become four
of five languages, plus Swatinem/rust-cache so every PR does not rebuild pyo3
from scratch.
docs/Rust.md is the reference: why targets come from a run's benchmark: TSVs
rather than from reading code, why the FFI boundary is one call per file and
never one per row, the fallback and staleness contracts, how to build in each
environment, and what PR #588's 19 standalone binaries got wrong.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Address Copilot review feedback on PR #975: - Add a checked-in rust-toolchain.toml pinning channel = "1.95.0" (the toolchain used locally) so local builds, CI, and any rustup-based environment compile the maturin/PyO3 extension with the same compiler. - Pin the CI action ref dtolnay/rust-toolchain@stable -> @1.95.0 in both test jobs so CI no longer tracks the moving stable channel; the ref and rust-toolchain.toml are cross-referenced to be bumped together. - Dockerfile: uv sync -> uv sync --frozen so the image installs exactly the committed uv.lock (no re-resolution), matching CI and keeping builds reproducible now that the step compiles Rust via maturin/cargo.
|
Addressed both Copilot review points in cc0192d: Reproducible Rust toolchain (
|
Combines the base branch's exact Rust 1.95.0 pin (cc0192d) with this branch's toolchain-prerequisite work: keep channel=1.95.0 and dtolnay/rust-toolchain@1.95.0, add Swatinem/rust-cache@v2, and in the Dockerfile keep both the rustup install and 'uv sync --frozen'.
…se maturin build gaps > **Targets #975's branch, not `main`.** @SkyeAv — this is offered *to* you, to merge into #975 if you like it, or ignore. It does not modify your commits. Written by Claude at @gaurav's request; @gaurav has reviewed the outcome but not line-by-line. Two things: hardening the extension's contract, and closing three gaps where the maturin swap has consequences that had not been followed through. ## Naming and the two failure modes **`rs` → `_accel`.** The name should say what it is (a compiled accelerator), not what it is written in. While the module is empty this rename is as cheap as it will ever be — not worth a fight if you prefer `rs`. **`src/accel.py` is now the only thing that imports the compiled module**, because the two failure modes need opposite handling and neither is obvious at the call site: - **A missing extension falls back to Python** and logs at INFO. Every snakefile does a top-level `import src.foo` at DAG-parse time, so raising would take down **all 243 rules** for a contributor without a Rust toolchain, for a reviewer, and for a fork's CI — not just the rules that would have used Rust. AGENTS.md's "a log warning is not a control" is about *wrong output*; a slower path emitting identical bytes is not that. - **A stale extension raises.** The extension is installed editable, so the compiled artifact sits in the checkout at `src/_accel.*.so` and a `git pull` does **not** rebuild it. `ABI_VERSION` is compared against `_REQUIRED_ABI_VERSION` at import — i.e. at DAG-parse time — so a stale build fails in the first second rather than partway through a 12 h rule. The check is a separate function so it can be tested against a stub; building a genuinely stale extension inside a unit test would mean compiling Rust. `BABEL_DISABLE_RUST=1` forces the Python path, which is what makes an A/B measurement possible in one checkout. An environment variable rather than a `config.yaml` entry: which of two byte-identical implementations runs has no user-facing meaning, and `config.yaml` is threaded into Snakemake `params` and output paths where changing it could perturb a running DAG. Precedent is `BABEL_DUCKDB_TEMP_DIR`. All three paths verified by hand, not just by test. ## Three gaps the maturin swap opens Since `[tool.uv] package = true`, every `uv run` and `uv sync` builds the project, and building it invokes cargo. - **`check-formatting.yml`'s snakefmt job runs `uv run` with no toolchain step.** It would have passed *by accident*, because `ubuntu-latest` ships Rust — which is worse than failing, since it makes a lint job depend on the runner image. Fixed by not building at all: `uv run --no-project --with snakefmt`. - **The Dockerfile installed Debian bookworm's cargo, which is 1.63 — exactly pyo3 0.23's minimum.** It builds today and breaks on the next pyo3 bump with an error that reads as unrelated. Swapped for rustup, which also honours `rust-toolchain.toml` (apt's cargo ignores it). - **Nothing in `slurm/`, `kubernetes/` or `docs/` mentioned Rust**, while `uv sync --frozen` on the Hatteras login node now **fails outright** without a toolchain — before Snakemake starts. `slurm/README.md` now says so, and notes that uv's own fallback downloads ~600 MB into a cache directory the `UV_CACHE_DIR` override in `run-babel-on-slurm.sh` does *not* cover. ## Housekeeping - `cargo fmt`/`clippy` job mirroring the one-linter-per-job structure of the other three, so "all four linters checked in CI" does not quietly become four of five languages. Plus `Swatinem/rust-cache` so every PR does not rebuild pyo3. - `rust/Cargo.toml` version → `0.0.0`: maturin takes the distribution version from `pyproject.toml` (the wheel is still `babel_pipeline-1.17-cp311-abi3`) and nothing publishes this crate, so a second version number is only a thing to forget to bump. `rust-version` declares pyo3's 1.63 floor; `rust-toolchain.toml` pins the channel. - **`[tool.maturin] exclude` stops the four per-directory `CLAUDE.md` agent-instruction files shipping to anyone who installs the wheel.** Verified by rebuilding: the wheel holds `src/_accel.abi3.so`, `src/_accel.pyi`, 98 `.py` files and the 19 `.snakefile`s, and no `CLAUDE.md`. ## One thing worth flagging about #975 itself Comparing wheels: `main`'s ships **zero** non-Python files, while the maturin wheel adds the 19 `.snakefile`s. That is an accidental **fix** — an installed `babel-pipeline` previously could not run the pipeline — but it is a behaviour change worth knowing about. Also, the "Rust toolchain is now a build prerequisite" caveat in #975's description is a little stronger than reality: **uv bootstraps a toolchain itself** (via `puccinialin`) when cargo is missing. Verified by building with no cargo on `$PATH`. The real cost is a silent ~600 MB download, not a failure — except on Hatteras, where `--frozen` does fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The comments in rust/src/lib.rs and src/accel.py said a hard import failure would 'take down all 245 rules'. The exact count is brittle (verified 244 run: + 26 shell: on main) and adds nothing: a top-level import failure at DAG-parse time takes down the whole parse, i.e. every rule. Wording it as 'every rule' cannot go stale.
The maturin swap (this branch) bundles all of src/* into the wheel, which includes the 19 .snakefile files; main's setuptools wheel shipped zero non-Python files, so an installed babel-pipeline previously could not run the pipeline. Record this accidental fix as a one-line general release note so the next release-notes author picks it up.
The cargo fmt/clippy job used dtolnay/rust-toolchain@stable, which installed rustfmt+clippy for 'stable'. But rust-toolchain.toml pins channel=1.95.0, so when cargo ran it switched to 1.95.0 -- a toolchain without those components -- and failed with "'cargo-fmt' is not installed for the toolchain '1.95.0'". Pin the action to 1.95.0 (as test.yml already is) so the components are installed for the toolchain cargo actually uses.
Status on the two branches offered into this PR#987 (Rust plumbing) — merged ( #988 (boundary-cost experiment) — closed unmerged. Its headline — "in-process can save at most Decision: in-process pyo3 (this PR) is the default mechanism — less code per accelerated routine Follow-ups harvested from #988 (no Rust needed): #996 (JSONL, not CI is green (ruff, snakefmt, cargo fmt/clippy, rumdl, unit-tests). |
| /// | ||
| // ponytail: a hand-bumped integer. Move to a build-time hash of this crate's sources if anyone | ||
| // forgets to bump it twice. | ||
| const ABI_VERSION: u32 = 1; |
There was a problem hiding this comment.
- Do we really need this? I don't think recompiling the Rust code for every Babel build will be a huge imposition.
- Is there any value for storing this in config.yaml rather than here?
There was a problem hiding this comment.
- It's not about build cost — it's about the editable install. The compiled
.sosits in the checkout (src/_accel.*.so, gitignored), and agit pullthat brings new Rust source rebuilds nothing (uv synconly rebuilds whenpyproject.toml/uv.lockchange). Without the guard, a stale binary silently runs 12-hour rules; with it, the run fails at DAG-parse in the first second, with the fix command in the message. Cost is one integer comparison at import, bumped in the same commit as the change that stales the binary. - No — the check compares what was baked into the
.soat compile time against what the Python checkout expects;config.yamlis runtime data and can't detect a stale binary. The two halves (here and_REQUIRED_ABI_VERSIONinsrc/accel.py) also need to move in one commit. Same reasonBABEL_DISABLE_RUSTisn't inconfig.yaml: config is threaded into Snakemakeparams/output paths, and an implementation detail shouldn't be able to perturb the DAG.
The prior commit dropped the "keep a byte-identical Python reference" rule from rust/README.md but left it verbatim in AGENTS.md and src/accel.py's docstring, and left dangling docs/Rust.md / docs/README.md links after the rust/ rename. Also restores BABEL_DISABLE_RUST documentation, reworded as the A/B tool for proving a port before deleting its Python original -- the env var and its test are still live in code, just no longer documented.
| the bump was forgotten; failing locally means the checkout needs | ||
| `uv sync --reinstall-package babel-pipeline`. | ||
| """ | ||
| if src.accel.accel is None: |
There was a problem hiding this comment.
I don't understand -- shouldn't this be built on uv sync? I'm concerned that this test will start skipping at some point but we won't notice it -- a failure would be better IMO.
There was a problem hiding this comment.
Yes — [tool.uv] package = true means every uv sync builds the extension (uv even bootstraps a toolchain if cargo is missing), and CI always has it, so this test runs there today. But your concern is right: if the build ever silently regressed, the skip would hide it. The no-extension fallback is covered separately (test_a_missing_extension_falls_back_instead_of_raising fakes the absence), so I've removed the skip — the test now fails instead: 6ebd4e4.
| return _read_something_python(path) | ||
| ``` | ||
|
|
||
| `accel` is the compiled module when it is present, importable and current, and `None` otherwise. |
There was a problem hiding this comment.
Is there any case where accel is not present?
gaurav
left a comment
gaurav
left a comment
There was a problem hiding this comment.
Thanks so much for all of this work, Skye! Would it be possible to include an example of Rust usage in this file so I can better see how a Rust rule could be incorporated into Snakemake? If there isn't an obvious candidate to pull in from #588, I wondered if #1004 might be a good candidate for Rustification: there's a bunch of different places where the text validation could go, so even if it doesn't make sense to replace to *Factory classes with something better (either entirely in Python or with some Rust code), it might make sense to add a check_encoding(output.synonyms_file) every time we create a synonyms to check the format. What do you think? Feel free to pick another function to Rustify or we can discuss this in person if useful -- I wouldn't try rustifying glom() or write_compendia(), which are probably the top candidates for this, because they're crucial functions that don't have any (?) tests yet and so introducing bugs in them would be baaaad, or the PubMed import, which I'm writing a complete replacement for in https://github.com/TranslatorSRI/pubmed2db as per NCATSTranslator/Core-Components-Working-Group#15. Let me know what you think! If you think this first-Rust-function would make more sense to do in a separate PR, then I'm happy to merge this for now and we can figure that out there, but I think it would be useful to put this example next to the new Rust documentation and pyproject.toml/Cargo settings we are developing here.
Replaces the Python babel_utils.glom() with a union-find over interned CURIE ids (path compression + union by size) exposed as _accel.glom and re-exported from src.babel_utils, so every caller is unchanged. This removes the O(N^2) of copying the whole merged clique on every merge; the unique_prefixes, KEGG/PUBCHEM, and close checks are maintained incrementally per root instead. Merged cliques grow the larger live Python set in place (reassigning only the smaller side's dict entries), and a cross-call cache stops repeated glom calls on the same dict from re-reading/re-interning the whole state. Rust-only: removes BABEL_DISABLE_RUST and the missing-extension fallback, deletes the Python glom, and strips the A/B language from the docs. Bumps ABI_VERSION 1->2 and rust-version to 1.80 (LazyLock for the cache). Offline benchmark vs the git-extracted Python glom: ~103x faster on a 6,000-merge chain (12,000 pairs) with identical cliques; the speedup grows with N (9x -> 25x -> 71x -> 100x), the signature of removing the O(N^2). 449 unit tests pass; cargo fmt/clippy/test, ruff, and rumdl all clean.
…ckend' into feat/migrate-to-maturin-build-backend # Conflicts: # rust/README.md
First real
|
| chain | pairs | python | rust | speedup | match |
|---|---|---|---|---|---|
| 1500 | 7500 | 0.416s | 0.046s | 9.1× | True |
| 3000 | 9000 | 1.667s | 0.067s | 24.8× | True |
| 4500 | 10500 | 4.013s | 0.056s | 71.3× | True |
| 6000 | 12000 | 6.921s | 0.069s | 99.9× | True |
Headline (chain=6000): Python 7.0s → Rust 0.07s (~103×), identical 3,001 cliques. The speedup
grows with N while Rust stays near-flat — the signature of removing the O(N²). At production scale
the quadratic term dominates more, so the real win should be larger; #990 (profile on real data) should
confirm where those rules are actually bound.
Verification (all offline — no BABEL run)
uv run pytest -m unit→ 449 passed (was 437; +12 new glom semantics tests). The suite exercises
glomthrough its real callers — the regression net now that there's no Python reference.cargo fmt --check,cargo clippy --all-targets -- -D warnings(0 warnings),cargo test— clean.ruff check .,ruff format --check,rumdl(0.2.50, as pinned in CI) — clean.git grep BABEL_DISABLE_RUST→ none outside scratch notes.
Questions
- The cross-call cache keys on the dict's address and rebuilds on entry-count/
unique_prefixes
mismatch; it assumes a dict is only mutated throughglom(true today). OK with that invariant, or
prefer the simpler rebuild-every-call (still correct, just re-reads state per call)? - Benchmark is a laptop-scale cost model. Want the coarse builder that also fuses file reading
(read_concord_file/remove_overused_xrefs) for the hottest callers in this PR, or as a follow-up?
|
@gaurav LMK if you want any addtional changes. I drafted this with an LLM and had it benchmark the optimizations to glom and it seems pretty promising. |
|
@gaurav this ran successfully at |
Swaps the build backend from setuptools to maturin so Babel builds as a mixed Rust/Python
package, ahead of landing native (Rust) functionality. This is the backend swap only — the Rust
side is an inert placeholder module, the importable package stays
src, and nothing under itmoves or renames.
Build backend
pyproject.toml:[build-system]is nowrequires = ["maturin>=1.0,<2.0"]/build-backend = "maturin"; dropped[tool.setuptools.packages.find].[tool.maturin]:module-name = "src.rs"places the compiled extension atsrc/rs.*andtells maturin to bundle the
srcpackage (auto-resolved to the repo root — src-layout detectiondoesn't fire because there's no
src/babel_pipeline/__init__.py);manifest-path = "rust/Cargo.toml"keeps Cargo off the Pythonsrc/dir.devgroup: addedmaturin>=1.0,<2.0souv run maturin developworks locally (the[build-system].requiresentry is build-isolation only).Rust stub
rust/Cargo.toml+rust/src/lib.rs: empty#[pymodule] fn rs(pyo3,abi3-py311+extension-module), importable asfrom src import rs. Real functionality lands in a follow-up.rust/so Cargo's defaultsrc/lib.rsnever collides with thePython
src/package.Build prerequisites (CI + Docker)
.github/workflows/test.yml: addeddtolnay/rust-toolchain@stablebeforeuv syncin bothjobs — every
uv syncnow compiles via cargo, andubuntu-latestships no Rust.Dockerfile:apt-get install -y cargo(system-wide, beforeUSER nru) for the same reason.bootstraps one itself (via
puccinialin) when cargo is absent — a silent ~600 MB download into aplatform cache, not a failure. The one place it genuinely fails is
uv sync --frozenon theHatteras login node (see
slurm/README.md); CI and Docker install rustup explicitly.Housekeeping
.gitignore: ignorerust/target/and thematurin developbinaries(
src/rs.*.so/.pyd/.dylib).uv.lock+rust/Cargo.lock: regenerated and committed (CI runsuv sync --frozen).Behavior change (worth knowing)
.snakefiles.main's setuptools wheel shipped zero non-Pythonfiles, so an installed
babel-pipelinepreviously could not run the pipeline; maturin bundles allof
src/*, which includes the.snakefiles. An accidental fix, but a behavior change.Design
supports requires a
Cargo.toml, so a bare backend swap with no Rust won't build. The empty#[pymodule]is the smallest thing that makes maturin viable now.module-name = "src.rs": the import package is literally namedsrc(flat layout).Verified against maturin's resolver (
project_layout.rs/module_writer/mod.rs): this bundlesall of
src/*and drops the extension atsrc/rs.<so>without moving or renaming anything.local
cp311-abi3wheel).Testing
uv sync— buildsbabel-pipelinevia maturin → cargo; installsmaturin==1.14.1.uv run python -c "import src; from src import rs; from src.tools.clique_diff import cli"— ok.uv run pytest -m unit -q→402 passed, 105 deselected.uv build→babel_pipeline-1.17-cp311-abi3-linux_x86_64.whl;unzip -lshowssrc/__init__.py,src/node.py,src/tools/...,src/rs.abi3.so, and all 4 console scripts inentry_points.txt.uv run babel-clique-diff --help— resolves.Questions for the reviewer
(~600 MB) when it's missing, so the practical cost is a one-time download, not a hard blocker —
except
uv sync --frozenon Hatteras, which does fail without a toolchain. Fine for a stub-onlyPR, or would you rather gate the maturin switch behind the PR that adds real Rust?