feat: python >= 3.12 stable ABI - #73
Closed
jonasteuwen wants to merge 1 commit into
Closed
Conversation
GitOrigin-RevId: 9f02be8643aa77bc8d3ba4f2c82aaaf18d2a501f
There was a problem hiding this comment.
Pull request overview
This PR updates FastSlide’s packaging/build pipeline to ship a single CPython stable-ABI wheel (cp312-abi3) per platform (usable on CPython ≥ 3.12), simplifying the build matrix and aligning Bazel/Meson/CI tooling around that artifact. It also removes several cache-related surfaces from the C/Rust/Python APIs and updates docs accordingly.
Changes:
- Switch wheel production to a single
cp312-abi3target (Bazel + Meson + CI), and drop per-Python-version wheel builds. - Update wheel-building scripts/CLIs and documentation to match the new single-wheel-per-platform flow.
- Remove C/Rust/Python cache API surface (with partial remaining Python call sites that still expect
use_global_cache()).
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/versioned_py_wheel.bzl | Adds stable-ABI build setting transition; wrapper rule needs a small fix for ctx.attr.wheel. |
| tools/build_wheels.py | CLI now builds stable-ABI wheel only (no python-tag selection). |
| tools/artifacts/wheels.py | Builds/copies a single //python:fastslide_wheel per platform. |
| tools/artifacts/specs.py | Replaces per-tag mapping with a stable-ABI tag constant. |
| tools/artifacts/cli.py | Removes --python selection to match stable-ABI wheel strategy. |
| tests/python/cache_test.py | Removes one cache test class but leaves other call sites unchanged (cannot be commented directly due to diff-region limits). |
| tests/meson.build | Removes C API cache test wiring. |
| src/python/fastslide.cpp | Removes from_file_path(..., cache=...) and cache helpers; binding needs compatibility for remaining use_global_cache() call sites. |
| src/python/_fastslide.pyi | Updates stubs to match binding changes; needs to reflect compatibility decision for use_global_cache(). |
| src/c/slide_reader.cpp | Removes C tile-cache functions/impl. |
| src/c/registry.cpp | Removes C API helpers for cached reader creation and global cache functions. |
| src/c/cache_c_api_test.cpp | Deletes C API cache tests. |
| rust/fastslide/src/registry.rs | Removes Rust global cache helpers. |
| rust/fastslide/src/reader.rs | Removes Rust per-reader cache APIs and stats type. |
| rust/fastslide/src/lib.rs | Stops exporting removed cache APIs; removes cache test. |
| rust/fastslide-sys/src/lib.rs | Removes FFI declarations for removed C cache APIs. |
| README.md | Updates Bazel wheel docs to single //python:fastslide_wheel. |
| python/BUILD.bazel | Consolidates to a single stable-ABI wheel target and packages the .abi3.so on non-Windows. |
| pyproject.toml | Floors Python to ≥3.12 and bumps meson-python requirement for limited-API handling. |
| package/Dockerfile | Removes apt retry hardening layer. |
| MODULE.bazel | Registers only the Python 3.12 toolchain to match stable-ABI strategy. |
| meson.build | Enables limited_api: '3.12' and adjusts Windows nanobind deps for correct limited-API linking. |
| include/fastslide/slide_options.h | Introduces cache injection fields but uses an undefined TileCache type and a non-compiling example. |
| include/fastslide/c/slide_reader.h | Removes C cache API declarations. |
| include/fastslide/c/registry.h | Removes cached reader/global cache C API declarations. |
| docs/source/guides/packages_and_releases.rst | Updates wheel build instructions; still mentions narrowing Python matrix. |
| docs/source/caching.rst | Updates caching docs but references non-existent ReaderDependencies APIs. |
| BUILD.bazel | Removes Bazel C API cache test target. |
| benchmarks/cache_benchmark.cpp | Removes cache benchmark source. |
| .github/workflows/release.yml | Builds only cp312-* and tests wheel import on 3.12–3.14; simplifies deb-smoke docker build. |
| .bazelrc | Pins @nanobind_bazel//:py-limited-api=cp312 for stable-ABI builds. |
Comments suppressed due to low confidence (3)
tools/versioned_py_wheel.bzl:50
wheelis declared asattr.label, soctx.attr.wheelis a single Target, not a list. Indexing with[0]will fail during analysis when this rule is used.
docs/source/caching.rst:193fastslide::ReaderDependenciesdoes not exist; the public API currently supports injecting a cache by passingstd::shared_ptr<ITileCache>directly toReaderRegistry::CreateReader.
// Inject via dependencies
auto deps = fastslide::ReaderDependencies::WithCache(*cache_or);
auto reader_or = registry.CreateReader("slide.mrxs", deps);
docs/source/caching.rst:215
- This section documents
ReaderDependencies/enable_caching, but that type/flag is not present in the codebase. Today, caching is enabled by providing a cache; omitting it opens without caching.
// Option 1: No cache in dependencies (default)
auto reader_or = registry.CreateReader("slide.mrxs");
// Option 2: Explicitly disable
fastslide::ReaderDependencies deps;
deps.enable_caching = false;
auto reader_or = registry.CreateReader("slide.mrxs", deps);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
23
to
+29
| #include "fastslide/utilities/colors.h" | ||
|
|
||
| namespace fastslide { | ||
|
|
||
| // Forward declarations | ||
| class TileCache; | ||
|
|
Comment on lines
58
to
60
| /// DependencyBundle deps; | ||
| /// deps.tile_cache = std::make_shared<TileCache>(1024 * 1024 * 1024); // 1GB | ||
| /// deps.background_color = ColorRGB{255, 255, 255}; // White background |
Comment on lines
132
to
+134
| To build wheels locally (optionally narrowing the platform/Python matrix):: | ||
|
|
||
| python3 tools/build_wheels.py --platform darwin_aarch64 --python cp311 | ||
| python3 tools/build_wheels.py --platform darwin_aarch64 |
Comment on lines
617
to
619
| .def("get_cache", &FastSlide::GetCache, "Get current cache") | ||
| .def_prop_ro("cache_enabled", &FastSlide::IsCacheEnabled, | ||
| "True if caching is enabled") |
Comment on lines
497
to
500
| @property | ||
| def cache_enabled(self) -> bool: | ||
| """True if caching is enabled""" | ||
|
|
Comment on lines
127
to
131
| #include "fastslide/runtime/global_cache_manager.h" | ||
| #include "fastslide/runtime/reader_registry.h" | ||
| #include "fastslide/runtime/reader_dependencies.h" | ||
|
|
||
| // Configure global cache at application startup (2 GiB) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitOrigin-RevId: 9f02be8643aa77bc8d3ba4f2c82aaaf18d2a501f