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

(dev): 3.11 testing #3923

Merged
merged 15 commits into from
Jul 6, 2022
Merged

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented May 4, 2022

Description

  • Testing 3.11

Suggested changelog entry:

* Fix issues and test against CPython 3.11 betas.

@Skylion007 Skylion007 added the python dev Working on development versions of Python label May 4, 2022
@Skylion007
Copy link
Collaborator Author

@rwgk Any ideas how to address this failure? I would like to get PyBind11 fully supported before 3.11 leaves alpha.

@rwgk
Copy link
Collaborator

rwgk commented May 5, 2022

@rwgk Any ideas how to address this failure? I would like to get PyBind11 fully supported before 3.11 leaves alpha.

No, unfortunately not. First thought: this could be really tricky to debug and fix. MRO is complex, especially between native Python & pybind11:

https://pybind11.readthedocs.io/en/stable/advanced/classes.html

... but once you begin mixing Python and C++ multiple inheritance, things will fall apart due to differences between Python’s MRO and C++’s mechanisms.

That's all I know though, I don't have first-hand experience working on the MI code.

What I'd try first: comment out tests until everything that's left passes. Then look at the overall picture to decide what to do next.

@Skylion007
Copy link
Collaborator Author

@rwgk I know what causes the issue. It's a corner case caused when a PyBind11 Class has multiple inheritance and the first base does not have dynamic-attrs, but another base class does not.

@rwgk
Copy link
Collaborator

rwgk commented May 5, 2022

I only remembered that very vaguely.

Variation of my "comment out" strategy: manually add py::dynamic_attr to see if that makes everything pass. If that's do-able: could that even be the final solution, maybe with a better error message?

@Skylion007
Copy link
Collaborator Author

@rwgk Adding it to the children classes did not help. Adding it to the base classes does work, but then that makes this unit test stops doing anything meaningful and the base class and any of its parents inherit the performance penalty of the dynamic-attrs.

@rwgk
Copy link
Collaborator

rwgk commented May 5, 2022

@rwgk Adding it to the children classes did not help. Adding it to the base classes does work, but then that makes this unit test stops doing anything meaningful and the base class and any of its parents inherit the performance penalty of the dynamic-attrs.

Sound fine to me:

  • Most importantly, everything works. (I guess)
  • In all likelihood, the performance penalty is inconsequential. (If not, they have a bigger problem IMO.)
  • If someone disagrees regarding the performance penalty, invite them to work out a fix.

My net conclusion: Add py::dynamic_attrs to the base class with a terse comment and link pointing here.

@henryiii
Copy link
Collaborator

FYI, beta1 is out, no more non-bug fix changes.

@henryiii henryiii force-pushed the skylion007/311-testing branch from c4d14bc to 99bafd0 Compare May 25, 2022 14:02
@henryiii henryiii force-pushed the skylion007/311-testing branch from 99bafd0 to a5a0e5d Compare May 25, 2022 14:10
@henryiii henryiii force-pushed the skylion007/311-testing branch from a5a0e5d to 9887cdc Compare May 25, 2022 14:18
@henryiii henryiii closed this Jun 21, 2022
@henryiii henryiii reopened this Jun 21, 2022
include/pybind11/embed.h Outdated Show resolved Hide resolved
include/pybind11/embed.h Outdated Show resolved Hide resolved
@vstinner
Copy link
Contributor

In the Python 3.10 code path, PySys_SetArgvEx() is called after Python initialization, when sys.path was already computed. In this case, the function inserts a new path at sys.path[0]:

If argv[0] is not '-c' nor '-m', prepend argv[0] to sys.path.

If you don't call PySys_SetArgvEx(), Py_RunMain() inserts a new path at sys.path[0] if safe_path=0: the path depends on PyConfig.argv[0].

I tried to document everything about the Python Path Configuration at: https://docs.python.org/dev/c-api/init_config.html#python-path-configuration See the "Py_RunMain() and Py_Main() modify sys.path: (...)" section.

If you control argv and you know what should be in sys.argv[0], you can just modify sys.path manually. Dummy example without error handling:

PyRun_SimpleString("import sys; sys.path.insert(0, '/my/path')");

Note: the code to compute sys.path has been rewritten in pure Python in Python 3.11: https://github.com/python/cpython/blob/main/Modules/getpath.py But this code doesn't include changes done afterwards by Py_RunMain(), as I wrote.

include/pybind11/embed.h Outdated Show resolved Hide resolved
@henryiii henryiii marked this pull request as ready for review July 4, 2022 18:57
include/pybind11/embed.h Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the skylion007/311-testing branch from 380c3d1 to e0e5673 Compare July 5, 2022 18:54
hswong3i added a commit to alvistack/pybind-pybind11 that referenced this pull request Jul 6, 2022
    git clean -xdf
    tar zcvf ../python-pybind11_2.9.2.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-pybind11.spec ../python-pybind11_2.9.2-2.spec
    mv ../python*-pybind11*2.9.2*.{gz,xz,spec,dsc} /osc/home\:alvistack/pybind-pybind11-2.9.2/
    rm -rf ../*pybind11*2.9.2*.*

See pybind#3923

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@henryiii henryiii merged commit 2af163d into pybind:master Jul 6, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 6, 2022
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks like Henry was faster by a few minutes.

Py_InitializeEx(init_signal_handlers ? 1 : 0);

detail::set_interpreter_argv(argc, argv, add_program_dir_to_path);
// Before it was special-cased in python 3.8, passing an empty or null argv
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really concerned about inlining the two complex functions here, especially because each version introduces multiple variables which could clash in non-obvious way for one Python version or another. We don't know how this code will need to be evolved in the future, accidents seem likely. I think it would be much better to keep the functions separate, even if we need #ifdef in both places, above for the function definitions, and here (I didn't look enough to know).

@@ -14,7 +14,7 @@ env:

jobs:
standard:
name: "🐍 3.11 dev • ubuntu-latest • x64"
name: "🐍 3.11 latest internals • ubuntu-latest • x64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistency would be nice, e.g.

3.11-latest above (line 33) and here.

I often look at CI logs in bulk, having to manually map latest internals here to -dev elsewhere is confusing/distracting.


PySys_SetArgvEx(argc, pysys_argv, static_cast<int>(add_program_dir_to_path));
#else
PyConfig config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment here would be useful (similar to Henry's comment from a few days ago):

  • Link to the corresponding Python sources.
  • Something like "unfortunately the Python code is private, therefore we are forced to duplicate here".

Copy link
Contributor

Choose a reason for hiding this comment

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

What has to be duplicated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had this comment in mind:

#3923 (comment)

@henryiii wrote 14 days ago:

_PyPathConfig_ComputeSysPath0 is private. :( It seems looking at PEP 587 that all the sys.path handling is now tied to the high level API (Py_Main, Py_BytesMain, and Py_RunMain), and line-by-line runners like us are stuck reimplementing the logic ourselves now.

When I saw the email that Henry had already merged this PR I stopped looking further. Doing that now:

This seems to be what @henryiii was referring to (and the "Link" I had in mind):

Comparing that with the new #else (Python 3.11) block here in embed.h, it looks like something completely different. Apparently I misinterpreted Henry's comment. Sorry.

Looking more, it seems this is @vstinner's response to the @henryiii's comment above:

Which makes me think adding this link would be useful:

    if (add_program_dir_to_path) {
            // https://docs.python.org/dev/c-api/init_config.html#python-path-configuration
            PyRun_SimpleString("import sys, os.path; "
                           "sys.path.insert(0, "
                           "os.path.abspath(os.path.dirname(sys.argv[0])) "
                           "if sys.argv and os.path.exists(sys.argv[0]) else '')");
    }

Or maybe alternatively the link to Victor's comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

PyConfig API initializes the default value of sys.path. But Py_RunMain() then inserts a path to sys.path. Python 3.11 safe_path=1 disables this behavior:

It might be interesting to move more code changing sys.path into the Python initialization (PyConfig), but it's complicated.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
hswong3i added a commit to alvistack/pybind-pybind11 that referenced this pull request Jul 18, 2022
    git clean -xdf
    tar zcvf ../python-pybind11_2.9.2.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-pybind11.spec ../python-pybind11_2.9.2-2.spec
    mv ../python*-pybind11*2.9.2*.{gz,xz,spec,dsc} /osc/home\:alvistack/pybind-pybind11-2.9.2/
    rm -rf ../*pybind11*2.9.2*.*

See pybind#3923

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Aug 5, 2022
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Aug 17, 2022
hswong3i added a commit to alvistack/pybind-pybind11 that referenced this pull request Oct 17, 2022
    git clean -xdf
    tar zcvf ../python-pybind11_2.9.2.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-pybind11.spec ../python-pybind11_2.9.2-2.spec
    mv ../python*-pybind11*2.9.2*.{gz,xz,spec,dsc} /osc/home\:alvistack/pybind-pybind11-2.9.2/
    rm -rf ../*pybind11*2.9.2*.*

See pybind#3923

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
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.

4 participants