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

Deprecate public constructors of module_ class #2552

Merged
merged 6 commits into from
Oct 9, 2020

Conversation

YannickJadoul
Copy link
Collaborator

@YannickJadoul YannickJadoul commented Oct 5, 2020

OK, so, this does a few things. A summary:

Main goal

Minor extra cleanup

  • Fully qualify the pybind11 namespace by using ::pybind11 in these module-related macros.
  • Turn the comment above module_::add_object into a valid doxygen-documentation of the function (and teach Doxygen that PYBIND11_NOINLINE isn't really a relevant part of the signature to generate docs).

Two more questions to be answered:

  1. Should this still be part of 2.6.0? I'd argue "yes", since
    a) I don't see a lot of uses for a plain py::module_ constructor (@rwgk also found out that module_'s constructor is hardly used in Google's code base, which strengthens the belief that there's not a lot of use for that constructor), and
    b) this will allow us to maybe reuse this constructor later? (cfr. Use PyModule_New in module_ public constructor #2551), and
    c) what's the point of postponing this, and delaying the pain (if any). That's what deprecation is for, no?

  2. Should we have pybind11::detail::create_top_level_module or static pybind11::module_::create_extension_module? I.e., is this a public interface or an internal one? I know I was the one arguing for the latter, before, but I think this being a public, but "named" constructor would work for me, as well.

EDIT: pybind11::detail::create_top_level_module was moved to static pybind11::module_::create_extension_module, with a unified signature, though @rwkg's suggestion of a typedef!

@YannickJadoul YannickJadoul force-pushed the deprecate-module-public-ctrs branch 3 times, most recently from 19c3079 to 81dd20e Compare October 5, 2020 23:01
@YannickJadoul YannickJadoul added this to the v2.6.0 milestone Oct 5, 2020
@YannickJadoul YannickJadoul marked this pull request as draft October 5, 2020 23:51
@YannickJadoul YannickJadoul force-pushed the deprecate-module-public-ctrs branch from 81dd20e to 4be6029 Compare October 6, 2020 14:28
@YannickJadoul YannickJadoul force-pushed the deprecate-module-public-ctrs branch from 575cdf0 to 7e51331 Compare October 7, 2020 23:15
@YannickJadoul YannickJadoul marked this pull request as ready for review October 7, 2020 23:15
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I think I mildly prefer the public name - macros like PYBIND11_MODULE tend to invariably have a few power users that need to avoid them for some special reason, so then this would be a requirement in that case. But we could wait and see, changing it to a public method in 1.6.1 if requested?

I believe the future direction is that we can better align with CPython's tools (like PyModule_New) in the future, possibly supporting multi-phase initialization eventually.

@YannickJadoul
Copy link
Collaborator Author

I think I mildly prefer the public name - macros like PYBIND11_MODULE tend to invariably have a few power users that need to avoid them for some special reason, so then this would be a requirement in that case. But we could wait and see, changing it to a public method in 1.6.1 if requested?

👍 That's kind of what I was aiming at, yes. I had/have actually changed it in #2551 (which has become a lot smaller, after rebasing onto this PR :-) ), since it makes sense in that context (i.e., vanilla module constructor vs. a special "named constructor" for extension modules). The only thing that's bothering me is that the interface differs between Python 2 and 3 (but ... yeah, Python 2 should die anyway, so ...).
Just wondered if we should already use that new name here; but doing that in a later release also works for me!

I believe the future direction is that we can better align with CPython's tools (like PyModule_New) in the future, possibly supporting multi-phase initialization eventually.

Yessssss!

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.

Hi @YannickJadoul, I agree this is better, I really like the deprecation. Please feel free to merge.

Optional, only if you feel it has merit: there is a lot of PY2 vs PY3 #if branching. Maybe it could be reduced via something like:

#if PY_MAJOR_VERSION >= 3
using module_def = PyModuleDef;
#else 
struct module_def {};
#endif

Then the code for PY2 code could be mostly identical to PY3 code.
If this is feasible, there will be a universal create_top_level_module(). I'd definitely make it public in that case. I'm less certain about making it public if there are different versions for PY2 and PY3, because it will force downstream users to also have #if, unless they are certain only PY3 is needed.

};
auto m = PyModule_Create(def);
if (m == nullptr)
pybind11_fail("Internal error in detail::create_top_level_module()");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea 1: does throw error_already_set() work?
Idea 2: somehow include name in the error message, to be helpful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Answered in the main thread, but that was maybe not a great idea:

I'm now just looking at the error message, to finish this PR, but I can't find if a nullptr mean the error indicator is already set?
https://docs.python.org/3/c-api/module.html#c.PyModule_Create

EDIT: If it definitely does, we should throw py::error_already_set to be fully correct!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the 3.7 sources, I see some code paths set an error for sure, but it's difficult to know if that's always the case. The bomb-proof solution would be something like if (PyErr_Occurred()) throw error_already_set(); and pybind11_fail() as a fallback, with name in the error message.
I kind of feel it's worth it: in case this fires it's most likely a difficult to debug situation, and showing the original error (root cause) or the module name could save a lot of time troubleshooting.
Unless we can convince the Python developers to document that a NULL return guarantees the error is set ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might not need to add name to the error message, if it usually or hopefully always works with error_already_set(), as that makes pybind11_fail() a last-ditch fallback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Assuming error_already_set would show the name?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Myeah, or we just trust that this part of the API works in the same way? An error_already_set won't cause things to be messed up too badly.

I can't both throw error_already_set ánd have the name in there, though. So that's only as a fall-back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Assuming error_already_set would show the name?)

I don't know; we don't control that, since that the message is already set by Python. It might be something as cryptic as "didn't manage to create a str from this const char*"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, didn't add the name, as the pybind11_fail is only our fallback option. We can add it, if we find out that (and when!) the control flow can reach that statement?

@YannickJadoul
Copy link
Collaborator Author

If this is feasible, there will be a universal create_top_level_module(). I'd definitely make it public in that case.

Great idea, yes! That's exactly what's holding me back. I had been thinking about a typedef with void(so we'd get void*), but you can't define void variables. I guess that extra word for an empty static struct instance won't make a difference?

@rwgk
Copy link
Collaborator

rwgk commented Oct 8, 2020

I guess that extra word for an empty static struct instance won't make a difference?

It's only one per module, which is almost certainly below the noise level, but also only for the un-dead Python 2: IMO nothing to worry about at all.

@henryiii
Copy link
Collaborator

henryiii commented Oct 8, 2020

but also only for the un-dead Python 2

We are definitely at the point where we should design for Python 3, and if it makes Python 2 a little ugly, I don't think that's something to worry about. Python 3 will be around a lot longer. :)

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Oct 8, 2020

OK, perfect! I agree, but wanted to have it mentioned :-)

I'm now just looking at the error message, to finish this PR, but I can't find if a nullptr mean the error indicator is already set?
https://docs.python.org/3/c-api/module.html#c.PyModule_Create

EDIT: If it definitely does, we should throw py::error_already_set to be fully correct!

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.

Awesome, I just have some minor polishing suggestions.

include/pybind11/pybind11.h Outdated Show resolved Hide resolved
auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
if (m == nullptr)
pybind11_fail("Internal error in module_::create_extension_module()");
return reinterpret_borrow<module_>(m);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you could move this #endif and the #elif above up, and not duplicate the 3 code lines?

Did you see my question about pybind11_fail vs throw error_already_set() from yesterday?

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 thought about that, but decided not to because of the // TODO: Should be reinterpret_steal, but Python also steals it again when returned from PyInit_... comment (which only holds for Python 3; in Python 2, it should be reinterpret_borrow).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you see my question about pybind11_fail vs throw error_already_set() from yesterday?

Yes. Wait, let me reply in that thread.

tests/test_modules.cpp Show resolved Hide resolved
…ule, and unify Python 2 and 3 signature again
@YannickJadoul YannickJadoul force-pushed the deprecate-module-public-ctrs branch from 7d17263 to a65a38b Compare October 8, 2020 14:20
@YannickJadoul YannickJadoul force-pushed the deprecate-module-public-ctrs branch from b5b4c25 to 0d0b596 Compare October 8, 2020 14:39
@henryiii henryiii merged commit 0c5cc03 into pybind:master Oct 9, 2020
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 9, 2020
try { \
PYBIND11_CONCAT(pybind11_init_, name)(m); \
return m.ptr(); \
} catch (::pybind11::error_already_set &e) { \
Copy link
Member

Choose a reason for hiding this comment

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

Can we use PYBIND11_CATCH_INIT_EXCEPTIONS here?

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'll pour that into another PR, yes.

@wjakob
Copy link
Member

wjakob commented Oct 12, 2020

Looks all good to me, just a minor note about the repeated exception handling code in the macro declaration.

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

Successfully merging this pull request may close these issues.

4 participants