-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Free threaded support #12555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Free threaded support #12555
Conversation
I guess because I'm not using the |
Great
…On Thu, Mar 6, 2025 at 5:03 PM Nathan Goldbaum ***@***.***> wrote:
@ngoldbaum commented on this pull request.
________________________________
In src/rust/src/padding.rs:
> @@ -110,6 +110,62 @@ impl PKCS7PaddingContext {
}
}
+#[pyo3::pyclass]
I'll cherrypick the Rust ports and make them their own PRs and keep the free-threading-specific stuff in this PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
e0636ce
to
3028574
Compare
3028574
to
146a9a3
Compare
146a9a3
to
3419fed
Compare
8beff7d
to
ece9508
Compare
01f5670
to
62e1ee6
Compare
@alex it looks like all the wheel builds are successful except for win32 and armv7. See these actions runs on my fork: win32: https://github.com/ngoldbaum/cryptography/actions/runs/16601346439/job/46961605807?pr=2 Not sure what to do about these. Do you have any ideas? I also disabled some manylinux2014 builds due to the issue that should be fixed by python-cffi/cffi#184. I also wasn't able to run ppc64le on my fork so let's see if there are any issues there. Are you OK with me marking all the rust PyModules with |
IDK what's going on with the win32 one. The armv7 one seems to suggest the build is building an aarch64 wheel, which is obviously just wrong. And yes, going to gil_used=False is fine, we don't rely on it. |
it occurs to me that the |
I'll put this in along with the new CFFI version constraints in EDIT: actually on second thought let's try to sort out the build issues first... |
62e1ee6
to
04c566e
Compare
At this point I think we're just waiting for the cffi release! Thanks for your work here! |
While the remaining build issues here aren't very fun I've been telling people that working on the CFFI reverse dependencies is the programming equivalent of peeling plastic off of new electronics: so satisfying to fix stuff that's been broken for a year or so! When you say "waiting for a CFFI release" does that mean you're not comfortable doing a cryptography release until there's a CFFI 2.0.0 final release? I think as long as there's an explicitly dependency on the beta release in cryptography's pyproject.toml, everything should be fine downstream. Unless I'm missing something. |
OK win32 is fixed. That came down to uv not selecting free-threaded python unless you explicitly opt into it. I added I tried building the armv7 wheel manually in a docker container on my Mac, but frustratingly the build succeeds there and produces an armv7 wheel: root@18e5360863db: so I still have no idea what's going wrong on the CI builds... |
That's correct that we don't want to ship a release until this is in a cffi release. |
I reported a bug against maturin: PyO3/maturin#2701 |
is there any chance it's an issue in the manylinux image, and that the 3.14-freethreaded binary in the armv7l image is accidentally aarch64? |
looks like that's indeed what's happening: pypa/manylinux#1825 (comment) |
Apologies for the noise here, I'm trying to debug the armv7 issue. |
No worries, make as much noise and use as much CI as you need :-)
…On Tue, Aug 5, 2025 at 6:17 PM Nathan Goldbaum ***@***.***> wrote:
*ngoldbaum* left a comment (pyca/cryptography#12555)
<#12555 (comment)>
Apologies for the noise here, I'm trying to debug the armv7 issue.
—
Reply to this email directly, view it on GitHub
<#12555 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBF6TGDZGLOO3NBPL4T3MEUODAVCNFSM6AAAAABYKW7YW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNJWHAYDKNRYHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
269aa12
to
dae8ad2
Compare
Looks like that fixed it! There's one unrelated failure. I'll go ahead and do a separate PR for the |
93f7ac4
to
2622389
Compare
OK, I think this should be ready whenever CFFI does a release. I'll try to rebase every now and then since this will bitrot quickly whenever the CI or wheel building config changes. |
cec0246
to
1fe8f0a
Compare
1fe8f0a
to
b7c6406
Compare
.github/workflows/wheel-builder.yml
Outdated
OPENSSL_DIR="/opt/pyca/cryptography/openssl" \ | ||
OPENSSL_STATIC=1 \ | ||
manylinux-entrypoint uv build --python=/opt/python/${{ matrix.PYTHON.VERSION }}/bin/python --wheel --require-hashes --build-constraint=$BUILD_REQUIREMENTS_PATH $PY_LIMITED_API cryptography*.tar.gz -o tmpwheelhouse/ | ||
manylinux-entrypoint uv build -v --python=/opt/python/${{ matrix.PYTHON.VERSION }}/bin/python --wheel --require-hashes --build-constraint=$BUILD_REQUIREMENTS_PATH $PY_LIMITED_API cryptography*.tar.gz -o tmpwheelhouse/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manylinux-entrypoint uv build -v --python=/opt/python/${{ matrix.PYTHON.VERSION }}/bin/python --wheel --require-hashes --build-constraint=$BUILD_REQUIREMENTS_PATH $PY_LIMITED_API cryptography*.tar.gz -o tmpwheelhouse/ | |
manylinux-entrypoint uv build --python=/opt/python/${{ matrix.PYTHON.VERSION }}/bin/python --wheel --require-hashes --build-constraint=$BUILD_REQUIREMENTS_PATH $PY_LIMITED_API cryptography*.tar.gz -o tmpwheelhouse/ |
- run: ${{ matrix.PYTHON.BIN_PATH }} -m pip install -r "${UV_REQUIREMENTS_PATH}" | ||
- name: add free-threaded python tools directory to PATH | ||
run: echo "/Library/Frameworks/PythonT.framework/Versions/3.14/bin" >> "$GITHUB_PATH" | ||
if: matrix.PYTHON.VERSION == '3.14t' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooof, why does this need to be special cased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://docs.python.org/3.14/using/mac.html#installing-free-threaded-binaries for more about this. We had to do more or less the same thing to get free-threading working in the setup-python github action: https://github.com/actions/python-versions/pull/319/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually on second thought I might be able to delete this, let me see if I can remember why I added this, I don't think my last comment is the correct reason...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, now I remember. This is described in the mac installer notes that I linked above, on the last bullet point under "known limitations". The GIL-enabled build adds the search path to PATH by default as part of the installer process, but doesn't add the PythonT.framework
path, so we have to add it manually.
I think @ned-deily is planning to update the installer eventually to fix these papercuts but those updates didn't make it in time for 3.14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I can't use python -m uv
, because that doesn't behave the same way as plain uv
does.
shell: bash | ||
- run: uv venv | ||
- run: uv venv --python='${{ steps.setup-python.outputs.python-path }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uv's solver sees the free-threaded build as experimental, so you need need to explicitly opt in to using it. See astral-sh/uv#8480 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is tehre an issue tracking when they'll treat it as stable? Can you add a comment pointing to that issue or (if there's no issue) just saying that the --python can be removed when free threading is stable?
.github/workflows/wheel-builder.yml
Outdated
echo $PY_LIMITED_API | ||
ls cryptography*.tar.gz | ||
uv build --python='${{ steps.setup-python.outputs.python-path }}' -v --wheel --require-hashes --build-constraint=$BUILD_REQUIREMENTS_PATH cryptography*.tar.gz $PY_LIMITED_API -o wheelhouse/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can undo this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is still needed because of the uv solver issue I explained about in my earlier comment.
Co-authored-by: Alex Gaynor <[email protected]>
|
||
- run: ${{ matrix.PYTHON.BIN_PATH }} -m pip install -r "${UV_REQUIREMENTS_PATH}" | ||
- name: add free-threaded python tools directory to PATH | ||
run: echo "/Library/Frameworks/PythonT.framework/Versions/3.14/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment with a link to a CPython issue so we'll remember in teh future we can remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is python/cpython#137450, I'll add a link
shell: bash | ||
- run: uv venv | ||
- run: uv venv --python='${{ steps.setup-python.outputs.python-path }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is tehre an issue tracking when they'll treat it as stable? Can you add a comment pointing to that issue or (if there's no issue) just saying that the --python can be removed when free threading is stable?
Fixes #12489