Improve Python 3.11 support by Skylion007 · Pull Request #3694 · pybind/pybind11 · GitHub
Skip to content

Improve Python 3.11 support - #3694

Merged
Skylion007 merged 21 commits into
pybind:masterfrom
Skylion007:test-311-frame-changes
Mar 2, 2022
Merged

Improve Python 3.11 support#3694
Skylion007 merged 21 commits into
pybind:masterfrom
Skylion007:test-311-frame-changes

Conversation

@Skylion007

@Skylion007 Skylion007 commented Feb 7, 2022 •
edited by henryiii
Loading

Copy link
Copy Markdown
Collaborator

Description

  • Unfortunately, 3.11a uncovered a bug in our multi-inheritance code that may cause memory leaks with dynamic_attrs, will deal with a follow up PR.
  • Fixes a compilation error caused by 3.11 requiring the use of the helper to get the f_back. This PR also backports the reference counting behavior of newer pythons (f_back being a strong reference that needs to be decremented).
  • Closes [BUG]: Remove Python 3.11/PyPy dispatch workaround #3530 by removing the workaround

Suggested changelog entry:

* Better support for Python 3.11 alphas.

@Skylion007 Skylion007 added the python dev Working on development versions of Python label Feb 7, 2022
@Skylion007 Skylion007 closed this Feb 7, 2022
@Skylion007 Skylion007 reopened this Feb 7, 2022
@Skylion007
Skylion007 requested review from henryiii and rwgk February 7, 2022 16:41
@Skylion007 Skylion007 changed the title Test 311 frame changes Python 3.11 support Feb 7, 2022
Skylion007
Skylion007 commented Feb 10, 2022
View reviewed changes
Comment thread include/pybind11/detail/class.h Outdated
@Skylion007

Skylion007 commented Feb 12, 2022

Copy link
Copy Markdown
Collaborator Author

Skylion007
Skylion007 commented Feb 14, 2022
View reviewed changes
Comment thread tests/test_multiple_inheritance.cpp Outdated

// test_mi_static_properties
py::class_<Vanilla>(m, "Vanilla").def(py::init<>()).def("vanilla", &Vanilla::vanilla);
py::class_<Vanilla>(m, "Vanilla", py::dynamic_attr())

@Skylion007 Skylion007 Feb 14, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making all bases in the hierarchy either static or dynamic_attrs works, but is not a tenable solution.

@Skylion007 Skylion007 mentioned this pull request Feb 27, 2022
Closed
@Skylion007 Skylion007 changed the title Python 3.11 support Improve Python 3.11 support Feb 28, 2022
Skylion007
Skylion007 commented Feb 28, 2022
View reviewed changes
Comment thread include/pybind11/detail/type_caster_base.h
+ std::to_string(lineno)
+ "): " + handle(f_code->co_name).cast<std::string>() + "\n";
frame = frame->f_back;
# if PY_VERSION_HEX >= 0x030900B1

@Skylion007 Skylion007 Feb 28, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we should add a backport section details/common.h . I just wasn't sure if the recommend static inline stuff would cause any nasty ODR / LTO issues so I left it as is.

@tacaswell

tacaswell commented Feb 28, 2022

Copy link
Copy Markdown

This fixes compiling scipy with py311.

@rwgk

rwgk commented Feb 28, 2022

Copy link
Copy Markdown
Collaborator

I looked a bit but have questions for my understanding:

Is it true that

  • the change in pybind11.h is purely a 3.11 compatibility fix?

  • the change in type_caster_base.h is for 3.11 compatibility, but also an actual fix for a bug that exists already pre 3.11? (Does the fix involve the Py_XINCREF(frame); and matching Py_DECREF(frame);?)

It would be useful to explain in a couple sentences in the PR description.

@Skylion007

Skylion007 commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator Author

@rwgk This fixes 3.11 compatability. The issue is the new API returns a strong reference (instead of a weak reference) which means we need to decref it. The easiest way to handle this was to have the old code path make a strong reference too. I'll edit the PR description.

rwgk
rwgk approved these changes Mar 1, 2022
View reviewed changes

@rwgk rwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM, too, but I'm wondering about

  • @Skylion007 I didn't see edits to the PR description, would be nice to have them.
  • the valgrind error looks like it needs attention
  • is it OK to ignore the upstream failure?

@Skylion007

Skylion007 commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator Author

@rwgk Good catch on the Valgrind error, seems I am not doing the incref / decrefs properly. Thoughts @henryiii ?

henryiii
henryiii reviewed Mar 1, 2022
View reviewed changes
Comment thread include/pybind11/detail/type_caster_base.h
henryiii
henryiii reviewed Mar 1, 2022
View reviewed changes
Comment thread include/pybind11/pybind11.h Outdated
@Skylion007

Skylion007 commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator Author

Thanks for pointing out that issue @henryiii. Looks good to me now.

@rwgk

rwgk commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator

I'll run some quick interactive tests in with the Google toolchain.

rwgk
rwgk approved these changes Mar 1, 2022
View reviewed changes
Comment thread include/pybind11/pybind11.h Outdated
@rwgk

rwgk commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator

@Skylion007
Skylion007 merged commit 42a8e31 into pybind:master Mar 2, 2022
@Skylion007
Skylion007 deleted the test-311-frame-changes branch March 2, 2022 18:18
@github-actions github-actions Bot added the needs changelog Possibly needs a changelog entry label Mar 2, 2022
@rwgk rwgk mentioned this pull request Mar 2, 2022
Merged
henryiii added a commit to henryiii/pybind11 that referenced this pull request Mar 2, 2022
@Skylion007 @henryiii @pre-commit-ci
Improve Python 3.11 support (pybind#3694)
1713203 
* Test out Python 3.11 migration

* Clean up a bit

* Remove todo

* Test workaround

* Fix potential bug uncovered in 3.11

* Try to fix it more

* last ditch fix

* Revert. Tp-traverse isn't the problem

* Test workaround

* Try this hack

* Revert MRO changes

* Use f_back properly

* Qualify auto

* Update include/pybind11/pybind11.h

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Simplify code slightly

* Ensure co_varnames decref if dict_getitem throws

* Eager decref f_code

Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
henryiii added a commit that referenced this pull request Mar 2, 2022
@Skylion007 @henryiii @pre-commit-ci
Improve Python 3.11 support (#3694)
6f01c60 
* Test out Python 3.11 migration

* Clean up a bit

* Remove todo

* Test workaround

* Fix potential bug uncovered in 3.11

* Try to fix it more

* last ditch fix

* Revert. Tp-traverse isn't the problem

* Test workaround

* Try this hack

* Revert MRO changes

* Use f_back properly

* Qualify auto

* Update include/pybind11/pybind11.h

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Simplify code slightly

* Ensure co_varnames decref if dict_getitem throws

* Eager decref f_code

Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@rwgk rwgk mentioned this pull request Mar 10, 2022
Merged
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
svenevs added a commit to svenevs/pybind11 that referenced this pull request Nov 4, 2022
@svenevs
Improve Python 3.11 support
3d90c7d 
Backport of pybind#3694
@svenevs svenevs mentioned this pull request Nov 4, 2022
Merged
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
@kevinsmia1939 kevinsmia1939 mentioned this pull request Jun 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henryiii henryiii henryiii approved these changes
@rwgk rwgk rwgk approved these changes

Assignees

No one assigned

Labels

python dev Working on development versions of Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: Remove Python 3.11/PyPy dispatch workaround

4 participants

@Skylion007 @tacaswell @rwgk @henryiii