Skip to content

[QUESTION] De-deprecation of direct call to module constructor #2718

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

Open
SylvainCorlay opened this issue Dec 8, 2020 · 6 comments
Open

Comments

@SylvainCorlay
Copy link
Member

We explicitely use the module constructor instead of the macro in xeus-python, and got the deprecation warning:

warning: 'pybind11::module_::module_(const char*, const char*)' is deprecated: Use PYBIND11_MODULE or module_::create_extension_module instead [-Wdeprecated-declarations]

Replacing

py::module foobar_module("foobar");	

with

py::module foobar_module = py::module_::create_extension_module("foobar", nullptr, new py::module_::module_def);

fixes it, but we find it a bit too verbose. Why not allowing direct calls to the constructor?

@YannickJadoul
Copy link
Collaborator

This was part of a long discussion in #2413, #2534, and (unfinished/unmerged) #2551.

To answer questions and summarize those:

  • Originally, the goal was to stop module/module_ from leaking this PyModuleDef on Python 3. This is possible because you only need a PyModuleDef for an extension module, and there's only 1 module as entry point in a C extension module. This is done in static allocation for PyModuleDef, to avoid leak check errors. #2413.

  • This made module have 2 different APIs, depending on Python 2 and 3, which goes against the goals of pybind11 and would possibly cause confusion. I argued to make this private, because of that, making the creation of an extension module part of detail (because it's handled transparently by PYBIND11_MODULE). This was done in Unify Python 2 & 3 py::module constructor, and make contructor with pre-allocated PyModuleDef private #2534. Note: it's still possible to get the old behavior, but this is 1) specialized usage (i.e. in detail), and 2) it's the user's responsibility to do memory management (or to leak, if preferred).

  • The final piece of the puzzle is that the module object of an extension module needs to be create differently (i.e. with PyModuleDef) than any free-standing module object. Use PyModule_New in module_ public constructor #2551 provides a normal, non-leaking constructor, but the PR got stalled and never merged, partially because is a semi-breaking API change.

What is your use case?

a) Do you just want a module (cfr. types.ModuleType('abc', 'def') in pure Python) ? If so, I believe #2551 is the way to go, which would turn py::module_::module_ into the pybind11 equivalent of types.ModuleType, without leaking.

b) Do you want more fine-grained control over the main extension module that is the entry point of the C extension, instead of the basic PYBIND11_MODULE macro? Then I think the current interface is correct: you need to not just construct a module, but explicitly specify it's an extension module, and explicitly take responsibility of managing the PyModuleDef in Python 3.
I argue that this latter scenario is advanced usage, with an interface that's not meant as public, so I believe it's correct to be in detail without taking any optional arguments. (So I'm not a fan of the proposed #2717, I'm afraid, because it hides a leak in pybind11.)

@YannickJadoul
Copy link
Collaborator

I had still forgotten #2552, which actually deprecated the public module constructor and moved the detail::create_extension_module to module::create_extension_module.

The one thing it failed to do is repurpose the old module constructor to a non-extension module. Let me rebase #2551 onto the current master, and see if that could work.

@martinRenou
Copy link
Contributor

What is your use case?

We use an embedded interpreter and create a couple of modules that are exposed to Python.

I am not sure I understand the difference you make between an extension module and a module?

@YannickJadoul
Copy link
Collaborator

@martinRenou, so Python seems to be making a difference between PyModule_New and PyModule_Create (Python3; in Python 2 that's Py_InitModule3). To be specific, PyModule_Create needs this PyModuleDef in Python 3, but cannot be used as extension module.

To illustrate, compare these two minimal examples, based on the example in the Python docs:

#include <Python.h>

static PyMethodDef SpamMethods[] = {
    {NULL, NULL, 0, NULL}
};

static struct PyModuleDef spammodule = {
    PyModuleDef_HEAD_INIT,"spam", "", -1, SpamMethods
};

PyMODINIT_FUNC
PyInit_spam(void)
{
    return PyModule_Create(&spammodule);
}
$ python3
Python 3.6.9 (default, Oct  8 2020, 12:12:24) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import spam
>>> 

The simpler version with PyModule_New does not work:

#include <Python.h>

PyMODINIT_FUNC
PyInit_spam(void)
{
    return PyModule_New("spam");
}
$ python3
Python 3.6.9 (default, Oct  8 2020, 12:12:24) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import spam
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: initialization of spam did not return an extension module
>>> 

So that's why we were trying to move towards a different in pybind11 as well. The first one, that requires the allocation of PyModuleDef (in Python 3 at least) will be py::module_::create_extension_module, while the constructor py::module_::module_ is turned into a "normal" module (cfr. calling types.ModuleType(...) in Python) by #2551.

Does that make sense to you?

I don't immediately know what kind of thing you need for an embedded module, but I would expect the second one (without PyModuleDef or leaks!) would be fine. PYBIND11_EMBEDDED_MODULE should work as old, btw; is there any reason you're not using that?

@martinRenou
Copy link
Contributor

martinRenou commented Dec 10, 2020

py::module_::module_ is turned into a "normal" module (cfr. calling types.ModuleType(...) in Python) by #2551.

Will you consider making this API public? That's the kind of API I am looking for.

PYBIND11_EMBEDDED_MODULE should work as old, btw; is there any reason you're not using that?

IIRC we don't use this because we want to return the py::module object from the function that creates it and use it in other places, and the MACRO didn't give us that freedom.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Dec 10, 2020

Will you consider making this API public? That's the kind of API I am looking for.

Well, I proposed #2551, so yes, I'm definitely up for merging it. Feel free to bump it into the others' attention again ;-)

Meanwhile, you could just try with reinterpret_steal<py::module_>(PyModule_New(...)), though?

IIRC we don't use this because we want to return the py::module object from the function that creates it and use it in other places, and the MACRO didn't give us that freedom.

PYBIND11_EMBEDDED_MODULE(foo, m) gives you m inside the body, though, so you could store that somewhere, after initialization?
Or you could access the pybind11_module_foo implementation detail, but it's not a great plan to depend on that either, ofc, since it's an implementation detail, so I'm not a great fan of doing that :-) Nvm, that won't work, even if it were a good idea.

What about just accessing py::module("sys").attr("modules")["foo"], though, after using PYBIND11_EMBEDDED_MODULE? That should work, and use the functionality pybind11 offers for embedded modules...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants