Skip to content

Unify Python 2 & 3 py::module constructor, and make contructor with pre-allocated PyModuleDef private #2534

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Unify Python 2 & 3 py::module constructor, and make contructor with pre-allocated PyModuleDef private #2534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ extern "C" {
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
static PyModuleDef PYBIND11_CONCAT(pybind11_module_def_, name);
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
auto m = pybind11::module( \
auto m = pybind11::detail::create_top_level_module( \
PYBIND11_TOSTRING(name), nullptr, \
&PYBIND11_CONCAT(pybind11_module_def_, name));
#else
Expand Down
60 changes: 44 additions & 16 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,35 +856,32 @@ class cpp_function : public function {
}
};


#if PY_MAJOR_VERSION >= 3
class module_;

PYBIND11_NAMESPACE_BEGIN(detail)
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
PYBIND11_NAMESPACE_END(detail)
#endif

/// Wrapper for Python extension modules
class module_ : public object {
public:
PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check)

/// Create a new top-level Python module with the given name and docstring
explicit module_(const char *name, const char *doc = nullptr)
#if PY_MAJOR_VERSION >= 3
explicit module_(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
if (!def) def = new PyModuleDef();
def = new (def) PyModuleDef { // Placement new (not an allocation).
/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ -1,
/* m_methods */ nullptr,
/* m_slots */ nullptr,
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr
};
m_ptr = PyModule_Create(def);
: module_(name, doc, new PyModuleDef()) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the new PyModuleDef() actually useful? If this is used there will be leak check errors again. — I'm just trying to understand. I intentionally side-stepped / deferred this question in #2413 (see "potential follow-up" note in top-level comment), to not have it block the straightforward solution, but if we invest more time to get things as clean as possible, the question seems important to consider.

Copy link
Collaborator Author

@YannickJadoul YannickJadoul Sep 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is the new PyModuleDef() actually useful?

Good point; I don't really know. Things worked like that before, so I just kept it, but it's worth an investigation.

Is there any point to constructing a module without PYBIND11_MODULE? Nested submodules are created with module_::def_submodule, modules are imported through module_::import, and extra embedded modules are added through PYBIND11_EMBEDDED_MODULE. So I don't know if there a valid use case for constructing a loose, non-nested, standalone module which is not the top-level module of an extension?

If not, maybe the module constructors ought to just be private (providing access through pybind11::detail::create_module for internal purposes)?

If there is a use case and this is used, we might not want to break old code? Correct me if I'm wrong, but the new vs. static doesn't actually matter a lot, and is just to shut up the leak detectors? In practice, both a static PyModuleDefas well as a new-allocated one will just stay alive for as long as Python is running.
So I saw #2413 more as a way to make valgrind and other sanitizers happy (which is a perfectly valid thing to do; don't get me wrong! :-) ), rather than plugging an actual leak.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A quick update: some pybind11 unit tests fail if I make the change suggested in the #2413 top-level comment.
But I will run our global testing anyway, to see if there are any other uses of module_::module_.

For easy references, this is the change I'm trying:

- if (!def) def = new PyModuleDef();
+ if (!def) pybind11_fail("PyModuleDef *def must not be a nullptr.");

So I saw #2413 more as a way to make valgrind and other sanitizers happy

Yes, that's true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I will run our global testing anyway, to see if there are any other uses of module_::module_.

@YannickJadoul No failures!
That observation strongly suggests that it's very unlikely that anyone outside needs the public module_::module_ constructor. I think if we can fix up the unit tests we can make it private; my suggestion is to try that after the 2.6.0 release is out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are only two call sites that need the module_::module_ constructor:

diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h
index eae86c7..eee1f45 100644
--- a/include/pybind11/embed.h
+++ b/include/pybind11/embed.h
@@ -48,7 +48,7 @@
 #define PYBIND11_EMBEDDED_MODULE(name, variable)                              \
     static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module &);    \
     static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() {        \
-        auto m = pybind11::module(PYBIND11_TOSTRING(name));                   \
+        auto m = pybind11::module(PYBIND11_TOSTRING(name), nullptr, nullptr); \
         try {                                                                 \
             PYBIND11_CONCAT(pybind11_init_, name)(m);                         \
             return m.ptr();                                                   \
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 0cde4fa..a909e52 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -863,7 +863,7 @@ public:
 
     /// Create a new top-level Python module with the given name and docstring
 #if PY_MAJOR_VERSION >= 3
-    explicit module_(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
+    explicit module_(const char *name, const char *doc, PyModuleDef *def) {
         if (!def) def = new PyModuleDef();
         def = new (def) PyModuleDef {  // Placement new (not an allocation).
             /* m_base */     PyModuleDef_HEAD_INIT,
diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp
index c1475fa..d5b2b43 100644
--- a/tests/test_modules.cpp
+++ b/tests/test_modules.cpp
@@ -60,7 +60,7 @@ TEST_SUBMODULE(modules, m) {
         class Dupe3 { };
         class DupeException { };
 
-        auto dm = py::module("dummy");
+        auto dm = py::module("dummy", nullptr, nullptr);
         auto failures = py::list();
 
         py::class_<Dupe1>(dm, "Dupe1");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great news! Let's go back a bit in history and check if there are any related issues or PRs that would say otherwise. If not, I like the idea of just making this constructor private and having access through pybind11::detail::create_top_level_module! :-)

I am in the middle of moving, though, so it'll be only this weekend or early next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think if we can fix up the unit tests we can make it private; my suggestion is to try that after the 2.6.0 release is out.

Any reason to wait for this until after 2.6.0, though? My initial suggestion would be to merge the current state of this PR (as to nót release a change to the public API that was introduced in #2413, and that would be annoying to deprecate again, later), and potentially add a deprecation warning to the public module_ constructors. Once people start using it, we can see if we get complaints and revert the deprecation, or go on with it?

But especially about the latter part, I can be persuaded to change our approach. I would really like to úndo the change to the public API before officially releasing it, though; your tests have given me a bit more confidence that this should actually be possible :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to wait for this until after 2.6.0, though?

My only concern is that it adds a little bit of complexity, but I'm OK if you feel merging this is best, on the way to making the entire interface private.

#else
explicit module_(const char *name, const char *doc = nullptr) {
{
m_ptr = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
#endif
if (m_ptr == nullptr)
pybind11_fail("Internal error in module_::module_()");
inc_ref();
}
#endif

/** \rst
Create Python binding for a new function within the module scope. ``Func``
Expand Down Expand Up @@ -949,10 +946,41 @@ class module_ : public object {

PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
}

private:
#if PY_MAJOR_VERSION >= 3
friend module_ detail::create_top_level_module(const char *, const char *, PyModuleDef *);

explicit module_(const char *name, const char *doc, PyModuleDef *def) {
def = new (def) PyModuleDef { // Placement new (not an allocation).
/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ -1,
/* m_methods */ nullptr,
/* m_slots */ nullptr,
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr
};
m_ptr = PyModule_Create(def);
if (m_ptr == nullptr)
pybind11_fail("Internal error in module::module()");
inc_ref();
}
#endif
};

using module = module_;

#if PY_MAJOR_VERSION >= 3
PYBIND11_NAMESPACE_BEGIN(detail)
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) {
return module_(name, doc, def);
}
PYBIND11_NAMESPACE_END(detail)
#endif

/// \ingroup python_builtins
/// Return a dictionary representing the global variables in the current execution frame,
/// or ``__main__.__dict__`` if there is no frame (usually when the interpreter is embedded).
Expand Down