Skip to content
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

fix: Use PyObject_VisitManagedDict() of Python 3.13 #4973

Merged
merged 4 commits into from Dec 15, 2023

Conversation

vstinner
Copy link
Contributor

@vstinner vstinner commented Dec 14, 2023

Use PyObject_VisitManagedDict() and PyObject_ClearManagedDict() in pybind11_traverse() and pybind11_clear() on Python 3.13 and newer.

Description

Suggested changelog entry:

Use ``PyObject_VisitManagedDict()`` and ``PyObject_ClearManagedDict()`` on Python 3.13 and newer.

Use PyObject_VisitManagedDict() and PyObject_ClearManagedDict() in
pybind11_traverse() and pybind11_clear() on Python 3.13 and newer.
@vstinner
Copy link
Contributor Author

cc @rwgk

@vstinner
Copy link
Contributor Author

I added Python 3.13 to .github/workflows/ci.yml to test the change.

@vstinner
Copy link
Contributor Author

Oh, "🐍 3.13 • ubuntu-20.04 • x64" failed with:

ERROR: Could not find a version that satisfies the requirement numpy~=1.26.0 (from versions: none)
ERROR: No matching distribution found for numpy~=1.26.0

@vstinner
Copy link
Contributor Author

@henryiii: Oh thanks for your fix, the three CI jobs running on Python 3.13 succeeded with your change.

Should I add Python 3.13 to the CI in a separated PR? Or is it ok to add it in the same PR? As you wish.

@henryiii
Copy link
Collaborator

Any breaking changes in 3.13 alphas/betas will also break our main workflow, which isn't ideal. In the past we've had an upstream job for this. Actually, it looks like it's still there, just running 3.12 still. Let me see if I can switch to that.

(Making the CI change in this PR is fine, btw)

@henryiii henryiii added the python dev Working on development versions of Python label Dec 14, 2023
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@rwgk
Copy link
Collaborator

rwgk commented Dec 15, 2023

The 🐍 3.12 • macos-latest • x64 workflow was stuck for 3+ hours. I canceled it and then triggered a rerun.

@rwgk rwgk merged commit dc477fa into pybind:master Dec 15, 2023
84 checks passed
@vstinner vstinner deleted the visit_managed_dict branch December 15, 2023 02:57
@vstinner
Copy link
Contributor Author

Nice, thanks for the help with the CI change.

@rwgk
Copy link
Collaborator

rwgk commented Dec 15, 2023

FWIW — Maybe for later:

upstream.yml only exercises one specific platform (ubuntu).

I think adding 3.13 testing in ci.yml would be best in general, because then we'd get a lot more coverage.

It would also have value to run 3.13 testing by default, with an option to disable it via a Label when we hit a time window in which 3.13 testing is broken because of a root cause that's in core Python (rather than pybind11). That way we wouldn't have to remember to test with 3.13, and we'd catch new core Python issues more-or-less immediately. The only annoyance would be that we'd have to use the Label until the issue is fixed.

What I don't know: Is there an easy way to exclude a matrix element (in ci.yml) based on a Label?

@henryiii henryiii added needs changelog Possibly needs a changelog entry and removed needs changelog Possibly needs a changelog entry labels Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python dev Working on development versions of Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants