HyukjinKwon commented on code in PR #101: URL: https://github.com/apache/spark-connect-rust/pull/101#discussion_r3986362302
########## .github/workflows/freethreaded.yml: ########## @@ -0,0 +1,54 @@ +name: Free-threaded build + +# Builds the pyspark-rs extension against a free-threaded (no-GIL, PEP 703) +# CPython. The extension is excluded from the workspace default-members, so the +# regular Rust CI never compiles it against a free-threaded interpreter -- and +# compiling against Py_GIL_DISABLED is exactly what enforces that every #[pyclass] +# is Sync. This workflow is that missing check. + +on: + push: + branches: [master, "branch-*"] + pull_request: + branches: [master, "branch-*"] + workflow_dispatch: + +permissions: + contents: read + +jobs: + freethreaded: + name: Build the extension against the free-threaded ABI + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.13t" + - name: Install Rust toolchain + shell: bash + run: | + rustup toolchain install stable --profile minimal + rustup default stable + - name: Install protoc + shell: bash + run: sudo apt-get update && sudo apt-get install -y protobuf-compiler + - name: Build pyspark-rs against the free-threaded interpreter + # If any #[pyclass] were not Sync, this build would fail: pyo3 requires + # Sync pyclasses on the free-threaded ABI. So a green build proves the + # extension is free-threading-safe. + env: + PYO3_PYTHON: python + shell: bash + run: cargo build -p pyspark-rs --release Review Comment: This is the valuable part of the PR and it's correct. `[lib] name = "_pyspark"` + `crate-type = ["cdylib"]` (`crates/pyspark-rs/Cargo.toml:10-12`) ⇒ `target/release/lib_pyspark.so`, and building it here against a free-threaded interpreter is exactly what makes pyo3 enforce `#[pyclass]: Sync` — the workspace `default-members` (`Cargo.toml:44`) excludes `pyspark-rs`, so no other job proves this. A green run here is the real guarantee — nothing needs to change on this step. ########## .github/workflows/freethreaded.yml: ########## @@ -0,0 +1,54 @@ +name: Free-threaded build + +# Builds the pyspark-rs extension against a free-threaded (no-GIL, PEP 703) +# CPython. The extension is excluded from the workspace default-members, so the +# regular Rust CI never compiles it against a free-threaded interpreter -- and +# compiling against Py_GIL_DISABLED is exactly what enforces that every #[pyclass] +# is Sync. This workflow is that missing check. + +on: + push: + branches: [master, "branch-*"] + pull_request: + branches: [master, "branch-*"] + workflow_dispatch: + +permissions: + contents: read + +jobs: + freethreaded: + name: Build the extension against the free-threaded ABI + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.13t" + - name: Install Rust toolchain + shell: bash + run: | + rustup toolchain install stable --profile minimal + rustup default stable + - name: Install protoc + shell: bash + run: sudo apt-get update && sudo apt-get install -y protobuf-compiler + - name: Build pyspark-rs against the free-threaded interpreter + # If any #[pyclass] were not Sync, this build would fail: pyo3 requires + # Sync pyclasses on the free-threaded ABI. So a green build proves the + # extension is free-threading-safe. + env: + PYO3_PYTHON: python + shell: bash + run: cargo build -p pyspark-rs --release + - name: Import the extension on the free-threaded interpreter + # Load the built extension standalone (avoiding the Python skin's heavier + # imports) to confirm it initializes on a free-threaded interpreter, and + # report whether the GIL is disabled. The GIL only actually stays off once + # the module declares gil_used = false (SPARK-59432); until then CPython + # re-enables it on import, which is expected and not a failure here. Review Comment: This comment is inaccurate under the pinned pyo3 0.28. Free-threading is **opt-out** there, so the plain `#[pymodule]` already emits `Py_MOD_GIL_NOT_USED` — the GIL does **not** get re-enabled on import, with or without #100. So line 54 will print `gil_enabled: False` on master too, not `True`. The step doesn't assert on it, so no failure, but the comment (and the "until #100" framing) is misleading. Suggest rewording to: "reports whether the GIL is disabled; under pyo3 0.28 modules are free-threading by default, so this is expected to be False." Ref: `pyo3-macros-backend-0.28.3/src/module.rs:452`, `pyo3-0.28.3/CHANGELOG.md:49` (#5564), migration guide https://pyo3.rs/v0.28.3/migration#default-to-supporting-free-threaded-python ########## .github/workflows/release.yml: ########## @@ -298,6 +298,27 @@ jobs: args+=(--zig) fi maturin build "${args[@]}" + - name: Set up free-threaded interpreters + if: runner.os == 'Linux' + uses: actions/setup-python@v5 + with: + python-version: | + 3.13t + 3.14t + - name: Build free-threaded wheels with maturin + # The free-threaded (PEP 703) ABI is incompatible with abi3, so these are + # separate, version-specific cp313t/cp314t wheels; pyo3 ignores the abi3-py39 + # feature when it detects a free-threaded interpreter. They land in the same + # dist/ and are uploaded alongside the abi3 wheel. Linux only for now -- + # macOS/Windows free-threaded legs can follow. + if: runner.os == 'Linux' + shell: bash + run: | + args=(--release --out dist --target "${{ matrix.target }}" --interpreter python3.13t python3.14t) Review Comment: Minor: `--interpreter python3.13t python3.14t` relies on `actions/setup-python@v5` exposing exactly those `t`-suffixed executables on PATH. That's correct for free-threaded builds, but a name mismatch would silently build **nothing** for that interpreter rather than fail — worth confirming both cp313t and cp314t wheels actually appear in `dist/` in the dry-run. ########## .github/workflows/release.yml: ########## @@ -298,6 +298,27 @@ jobs: args+=(--zig) fi maturin build "${args[@]}" + - name: Set up free-threaded interpreters + if: runner.os == 'Linux' + uses: actions/setup-python@v5 + with: + python-version: | + 3.13t + 3.14t + - name: Build free-threaded wheels with maturin + # The free-threaded (PEP 703) ABI is incompatible with abi3, so these are + # separate, version-specific cp313t/cp314t wheels; pyo3 ignores the abi3-py39 + # feature when it detects a free-threaded interpreter. They land in the same + # dist/ and are uploaded alongside the abi3 wheel. Linux only for now -- + # macOS/Windows free-threaded legs can follow. + if: runner.os == 'Linux' + shell: bash + run: | + args=(--release --out dist --target "${{ matrix.target }}" --interpreter python3.13t python3.14t) + if [[ "${{ matrix.zig }}" == "true" ]]; then Review Comment: **Please verify in a release dry-run — likely broken.** `maturin` and `ziglang` are `pip install`ed into the **3.11** interpreter in the "Build abi3 wheel" step (`release.yml:289-299`, `python -m pip install ziglang` at :297). The preceding "Set up free-threaded interpreters" step (:301-307) then makes **3.14t** the default `python`/`python3` on PATH (last entry wins). maturin locates zig by probing `python -m ziglang`; if it probes the now-default 3.14t (no `ziglang` installed there), `--zig` fails here — and both Linux matrix rows are `zig: true` (:255-256), so this is the common path. It isn't exercised by `freethreaded.yml`. Safer: install `ziglang` (and maturin) **after** the free-threaded `setup-python`, or pin the zig interpreter explicitly. Please attach a green dry-run link. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
