Add make_simple_namespace function and tests #2840

joukewitteveen · 2021-02-01T10:20:08Z

Description

This feature request has not been discussed (by me, at least) elsewhere.

Namespaces can be convenient as arguments to Python functions called from C++. In simple cases, they are easier to construct than that classes are instantiated.

On the Python side, these simple namespaces are very simple. They are basically objects on which attributes can be set and removed.

Next to the addition of make_simple_namespace, I did some minor expansion of documentation and tests.

Suggested changelog entry:

Add ``make_simple_namespace`` function for instantiating Python SimpleNamespace objects.

edit: modified changelog suggestion to reflect the move away from make_namespace.
edit2: restored changelog suggestion to reflect the move back to make_namespace.
edit3: modified changelog suggestion to reflect the rename to make_simple_namespace.

joukewitteveen · 2021-02-01T10:39:13Z

Indeed, simple namespaces are only available from Python 3.3 onward. That is why some checks are not successful. Please let me know if this MR has any chance of being accepted before I try and think of a fallback for older Python versions.

YannickJadoul · 2021-02-01T21:49:29Z

Hi, @joukewitteveen! Thanks for this!

A few questions, if you don't mind:

I personally never had any use for these, but I'm assuming you have something in mind?
Is there any Python documentation on this? I'm wondering if we should link to it in the docs, to avoid confusion with C++ namespaces. At any rate, maybe you could still add a note that the equivalent of C++ namespaces would normally just be def_submodule?
Is there a point in having a py::namespace object? Since you're making the comparison to py::tuple/py::make_tuple, that would make sense. But at the same time, there doesn't seem to be a lot of API related to Python's namespaces (and hardly any docs, it seems).

Other than that: shrug, why not, if it can be useful :-)

YannickJadoul · 2021-02-01T21:50:23Z

tests/test_pytypes.cpp

@@ -75,6 +75,19 @@ TEST_SUBMODULE(pytypes, m) {
        return dict.contains(val);
    });

+    // test_tuple
+    m.def("make_tuple", []() { return py::make_tuple(42, py::none(), "spam"); });


Isn't this tested anywhere else!? If so, wow, good catch! If not: do we want to repeat this here?

There are other tests that rely on make_tuple to work, so if it wouldn't work that would not go unnoticed. However, nowhere are specific tests for make_tuple grouped. I thought it would be good to be a bit more explicit in that regard and, if nothing else, provide a space for such tests to be collected.

joukewitteveen · 2021-02-02T10:36:27Z

* I personally never had any use for these, but I'm assuming you have something in mind?

I needed to pass an instantiated Logger class in a Python call. I wanted logging to be taken care of by C++ functions, and using a namespace with a few attributes (cppfunctions) is easier than instantiating and configuring a class.

* Is there any Python documentation on this? I'm wondering if we should link to it in the docs, to avoid confusion with C++ namespaces. At any rate, maybe you could still add a note that the equivalent of C++ namespaces would normally just be `def_submodule`?

I added a link to it, but decided against a note on C++ namespaces versus (sub)modules. I'm not sure it is true that those are conceptually linked so strongly. As I understand it, a namespace is pretty fundamental in Python and modules (and classes and everything else) are mostly namespaces with "extra logic". Thus, a SimpleNamespace (from make_namespace) can in fact be thought of as an (maybe not "the") equivalent of a C++ namespace.

* Is there a point in having a `py::namespace` object? Since you're making the comparison to `py::tuple`/`py::make_tuple`, that would make sense. But at the same time, there doesn't seem to be a lot of API related to Python's namespaces (and hardly any docs, it seems).

The core API relevant to a SimpleNamespace, I think, consists of hasattr, delattr, getattr, setattr. Those are present on objects, so we don't need a py::namespace. In fact, make_namespace() is one way to get an "empty" object (not just what effectively amounts to a nullptr).

YannickJadoul · 2021-02-02T11:57:25Z

include/pybind11/cast.h

+template <typename... Args,
+          typename = detail::enable_if_t<args_are_all_keyword_or_ds<Args...>()>>
+object make_namespace(Args&&... args_) {
+    return reinterpret_steal<object>(_PyNamespace_New(dict(std::forward<Args>(args_)...).ptr()));


Can _PyNamespace_New return NULL and set an error inside CPython?

https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Objects/namespaceobject.c#L33

It can return NULL, but not sure about setting the error. Error part is on PyDict_New.

It can indeed return NULL. I'm not sure this will actually happen, but I revised it anyway so that it is more explicit.

bstaletic · 2021-02-02T12:01:30Z

Does py::isinstance work with these things? Usually there's PyThing_Check(PyThingObject*), but there isn't a PyNamespace_Check. However, besides the _PyNamespace_New(), we also have access to _PyNamespace_Type type object. So... if PyObject_IsInstance(namespace_ptr, &_PyNamespace_Type); works, py::isinstance should work as well. At that point, it would be inconsistent to allow py::isinstance(object, type) to work, but not py::isinstance<pybind_type>(object).

YannickJadoul · 2021-02-02T12:02:26Z

but decided against a note on C++ namespaces versus (sub)modules. I'm not sure it is true that those are conceptually linked so strongly.

Isn't this the closest you can practically get to C++ namespaces?

To me, it seems Python namespaces are basically the lambdas of objects; some kind of anonymous object?
C++ namespaces and Python submodules are +- meant to organize functions, classes, ... hierarchically within a project, so are to me (practially speaking) much closer.

Also, we really need to take care of people coming from both C++ as well as Python. The link to the Python docs already helps a bit, though.

joukewitteveen · 2021-02-02T14:14:04Z

Does py::isinstance work with these things? Usually there's PyThing_Check(PyThingObject*), but there isn't a PyNamespace_Check. However, besides the _PyNamespace_New(), we also have access to _PyNamespace_Type type object. So... if PyObject_IsInstance(namespace_ptr, &_PyNamespace_Type); works, py::isinstance should work as well. At that point, it would be inconsistent to allow py::isinstance(object, type) to work, but not py::isinstance<pybind_type>(object).

I am not sure what you mean here. Can you give a specific code example? Without a py::namespace type, it is unclear to me how this is relevant.

but decided against a note on C++ namespaces versus (sub)modules. I'm not sure it is true that those are conceptually linked so strongly.

Isn't this the closest you can practically get to C++ namespaces?

To me, it seems Python namespaces are basically the lambdas of objects; some kind of anonymous object?
C++ namespaces and Python submodules are +- meant to organize functions, classes, ... hierarchically within a project, so are to me (practially speaking) much closer.

Also, we really need to take care of people coming from both C++ as well as Python. The link to the Python docs already helps a bit, though.

I agree that we should avoid confusion. Do you have any specific addition to the documentation in mind?

The way I think of it, a namespace is nothing more than a mapping from names to, in the case of Python, objects. A SimpleNamespace does this in a very bare bones manner. I'm not sure what you mean by 'anonymous object', but indeed, modules usually have more attributes. Whether this makes them more akin to C++ namespaces I don't know. Both can provide a hierarchical organization of functions, classes, etc..

joukewitteveen · 2021-02-15T07:55:19Z

@YannickJadoul @bstaletic Is there anything expected from me to advance this pull request, or is the request simply awaiting its turn in the 2.7 cycle for further decisions?

bstaletic · 2021-02-15T09:36:13Z

I am not sure what you mean here. Can you give a specific code example? Without a py::namespace type, it is unclear to me how this is relevant.

Does py::isinstance(namespace_object, &_PyNamespace_Type); return true?

joukewitteveen · 2021-02-15T10:28:42Z

Does py::isinstance(namespace_object, &_PyNamespace_Type); return true?

py::isinstance(obj, (PyObject *) &_PyNamespace_Type); returns true on namespace object and false on others (e.g. tuples). Shall I add this observation to the tests?

bstaletic · 2021-02-15T10:53:33Z

I'm saying that if py::isinstance works, and you've just said that it does, this warrants a proper py::namespace type. Why should _PyNamespace_Type be different from all the other python types that pybind11 supports?

joukewitteveen · 2021-02-15T13:16:38Z

@bstaletic: That makes sense. I tried my hand at making namespace_ a proper type (do we want using namespace = namespace_ to enable py::namespace?). It fails on PyPy, where SimpleNamespaces are implemented as Python classes. I would like to do

diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
index e6cfc70..e070026 100644
--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -764,7 +764,14 @@ inline bool PyUnicode_Check_Permissive(PyObject *o) { return PyUnicode_Check(o)
 #endif
 
 #if PY_MAJOR_VERSION >= 3
-inline bool PyNamespace_Check(PyObject *o) { return isinstance(o, (PyObject *) &_PyNamespace_Type); }
+inline bool PyNamespace_Check(PyObject *o) {
+#if !defined(PYPY_VERSION)
+    auto type = (PyObject *) &_PyNamespace_Type;
+#else
+    auto type = module_::import("types").attr("SimpleNamespace");
+#endif
+    return isinstance(o, type);
+}
 #endif
 
 inline bool PyStaticMethod_Check(PyObject *o) { return o->ob_type == &PyStaticMethod_Type; }

but I guess module_ is not available in pytypes.h. Any suggestions?

bstaletic · 2021-02-15T13:22:46Z

Do we want using namespace = namespace_ to enable py::namespace?

No. using module = module_; is there for backwards compatibility. You'll notice that there isn't py::class, only py::class_.

module_::import("types").attr("SimpleNamespace");

auto types_module = py::reinterpret_steal<py::object>(PyImport_ImportModule("types"));

YannickJadoul · 2021-02-15T13:24:00Z

module_::import("types").attr("SimpleNamespace");
auto types_module = py::reinterpret_steal<py::object>(PyImport_ImportModule("types")); 

Why? That's what module_::import("types") is also doing, no?

EDIT: Oh, wait. To solve the issue from above, ofc!

YannickJadoul · 2021-02-15T13:25:47Z

Given this complication with PyPy, I'm wondering, btw: is there any point in doing isinstance(obj, py::namespace_)? These objects are completely meant as duck-typed things, anyway, right? Is there then any point in making this a full py::object subtype?

joukewitteveen · 2021-02-25T10:02:27Z

Another possibility is to introduce a detail::namespace_type, which is set to (PyObject *) &_PyNamespace_Type with Python and to module_::import("types").attr("SimpleNamespace") with PyPy. The latter definition would be placed below the definition of class module_ in pybind11.h.
This does mean we import types whenever we run with PyPy.

Given this complication with PyPy, I'm wondering, btw: is there any point in doing isinstance(obj, py::namespace_)? These objects are completely meant as duck-typed things, anyway, right? Is there then any point in making this a full py::object subtype?

Personally I cannot think of a use case where it makes a lot of sense to check for namespace instances. As I argued before, these simple namespaces are as close to a bare object as I think you can get. I don't think it hurts to think of them as just py::objects.

Shall I revert back to my original make_namespace proposal?

YannickJadoul · 2021-02-26T12:41:34Z

Shall I revert back to my original make_namespace proposal?

If it's up to me, and you don't mind, sure. I think I was fine with the PR, but hadn't gotten the chance to click "approve".

Personally I cannot think of a use case where it makes a lot of sense to check for namespace instances. As I argued before, these simple namespaces are as close to a bare object as I think you can get. I don't think it hurts to think of them as just py::objects.

Not saying this PR isn't useful or good, but funny you say this: why not just literally create a py::object and assign with obj.attr("foo") = ...? Just out of curiosity; not saying this PR needs a big overhaul!
I recently also had a situation where this would have been useful, btw :-)

joukewitteveen · 2021-03-01T08:43:56Z

Shall I revert back to my original make_namespace proposal?

If it's up to me, and you don't mind, sure. I think I was fine with the PR, but hadn't gotten the chance to click "approve".

Done.

Personally I cannot think of a use case where it makes a lot of sense to check for namespace instances. As I argued before, these simple namespaces are as close to a bare object as I think you can get. I don't think it hurts to think of them as just py::objects.

Not saying this PR isn't useful or good, but funny you say this: why not just literally create a py::object and assign with obj.attr("foo") = ...? Just out of curiosity; not saying this PR needs a big overhaul!
I recently also had a situation where this would have been useful, btw :-)

Maybe I am missing something completely, but this would not work because constructing a py::object does not instantiate a Python object (it defaults to a nullptr), right? At least .attr segfaults on a freshly created pure py::object for me.

YannickJadoul

Done.

Thanks a lot! Apologies for the back and forth on this one; guess it's part of figuring out the desired API. But this pretty looks good, to me :-)

If you still feel adventurous, allowing auto d = py::dict("foo"_a = 42, "bar"_a = py::none(); py::make_namespace(**d); could be fun, but I'm perfectly happy adding that when there's any need for it.

Maybe I am missing something completely, but this would not work because constructing a py::object does not instantiate a Python object (it defaults to a nullptr), right? At least .attr segfaults on a freshly created pure py::object for me.

Nope, you're not wrong. I was just loosely thinking that maybe there should be a way of constructing an empty py::object. But let's not dive into that in this PR anymore :-)

joukewitteveen · 2021-03-01T15:56:02Z

Done.

Thanks a lot! Apologies for the back and forth on this one; guess it's part of figuring out the desired API. But this pretty looks good, to me :-)

No problem. It was a good opportunity to get to know some of the internals of pybind11.

If you still feel adventurous, allowing auto d = py::dict("foo"_a = 42, "bar"_a = py::none(); py::make_namespace(**d); could be fun, but I'm perfectly happy adding that when there's any need for it.

I'm not sure this would be a good idea, but I agree that it looks like a fun addition. I would not encourage people to think of namespaces as dictionaries too much, since namespaces have different key requirement.

Maybe I am missing something completely, but this would not work because constructing a py::object does not instantiate a Python object (it defaults to a nullptr), right? At least .attr segfaults on a freshly created pure py::object for me.

Nope, you're not wrong. I was just loosely thinking that maybe there should be a way of constructing an empty py::object. But let's not dive into that in this PR anymore :-)

Haha, we've come full circle. You can't really have an empty py::object, because there is no such thing as a bare object type. One thing that comes real close to it will be py::make_namespace() ;-).

YannickJadoul · 2021-03-01T16:05:57Z

Haha, we've come full circle. You can't really have an empty py::object, because there is no such thing as a bare object type. One thing that comes real close to it will be py::make_namespace() ;-).

Right!
Well, also kind of wrong: you can make an object() (I've seen it being used as sentinel = object() when None is a valid value; see here). But you can't assign attributes to it, so I did miss that, indeed!

>>> x = object()
>>> x.foo = 3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'object' object has no attribute 'foo'

rwgk

Really nice, thank you @joukewitteveen!
Caveat: I didn't read the many PR comments. But I really like the end result!

rwgk · 2021-08-18T21:27:04Z

docs/advanced/pycpp/object.rst

+
+Attributes on a namespace can be modified with the :func:`py::delattr`,
+:func:`py::getattr`, and :func:`py::setattr` functions. Namespaces can be useful
+as stand-ins for class instances.


... as very light-weight stand-ins ...

include/pybind11/cast.h

docs/advanced/pycpp/object.rst

tests/test_pytypes.cpp

joukewitteveen · 2021-08-22T16:51:00Z

Thanks, @rwgk, for your suggestions. I implemented them.

Since 2.7 is out, I think the corresponding milestone should be closed. It would be cool if that happened automatically when tagging a release. There's probably a way to do that.

Note that #2451 introduces functionality that should also be mentioned in the documentation added in this PR. I think it is best to first merge this one and only merge #2451 after it is rebased and amended with some documentation updates.

Skylion007 · 2021-08-24T01:22:04Z

My only other qualm is make_namespace to generic / not specific of a name? Especially with C++ namespaces.

rwgk · 2021-08-24T05:37:01Z

My only other qualm is make_namespace to generic / not specific of a name? Especially with C++ namespaces.

That's a good point. Systematically inserting "simple" would be great.

joukewitteveen · 2021-08-24T09:27:28Z

I don't think there is a real risk of confusion. At the end of the day, both C++ and Python have namespaces and the objects returned from make_namespace fit the Python definition of a namespace.

That being said, I too think adding simple is a good idea (although the name gets a bit long). Shall I go with make_simplenamespace or make_simple_namespace?

rwgk · 2021-08-24T13:59:58Z

That being said, I too think adding simple is a good idea (although the name gets a bit long). Shall I go with make_simplenamespace or make_simple_namespace?

The 2nd version, with the underscore, looks much better to me.
(I think code will be easier to read with the longer name. Without the "simple" people would first go huh? until they go look up what it is.)

tests/test_pytypes.cpp

rwgk

Awesome, thanks!
Just waiting for the CI to finish, and @Skylion007 to take another look.

rwgk · 2021-08-26T15:04:07Z

Thanks everybody! Merging :-)

henryiii · 2021-10-17T03:56:08Z

include/pybind11/cast.h

+template <typename... Args,
+          typename = detail::enable_if_t<args_are_all_keyword_or_ds<Args...>()>>
+object make_simple_namespace(Args&&... args_) {
+    PyObject *ns = _PyNamespace_New(dict(std::forward<Args>(args_)...).ptr());


Sorry for not noticing this! (I never reviewed this PR, it seems). Why are we using private API here? This private API moved recently in CPython 3.11 development, so this breaks CPython 3.11 support (after alpha 1, I think).

They were going to add this to the public API 2 days ago, but dropped it because:

When I created this PR, I understood that types.SimpleNamespace was immutable. Since it's possible to add new attributes and modify attributes, I'm no longer convinced that it's so useful. It is still possible to access it in C by getting types.SimpleNamespace type and then call the type.

I prefer to close this PR and move the API to the internal C API.

python/cpython#28970 (comment)

So I think that's the workaround for us.

What was the benefit here over py::module_::import("types").attr("SimpleNamespace")(...)? Besides being less discoverable with a custom name, it also adds extra header complexity and compile time to expose a private implementation detail of CPython. People should be able to combine simple things to produce complex things, not have a long list of tiny custom functions that have to be learned.

diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 2157dc09..9a1e9188 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -84,7 +84,7 @@ TEST_SUBMODULE(pytypes, m) { #if PY_VERSION_HEX >= 0x03030000 // test_simple_namespace m.def("get_simple_namespace", []() { - auto ns = py::make_simple_namespace("attr"_a=42, "x"_a="foo", "wrong"_a=1); + auto ns = py::module_::import("types").attr("SimpleNamespace")("attr"_a=42, "x"_a="foo", "wrong"_a=1); py::delattr(ns, "wrong"); py::setattr(ns, "right", py::int_(2)); return ns;

This literally passes the test suite. This should not have been added.

My suggestion would be to change the #if to exclude Python 3.11 and higher, deprecate the feature, and document what you have here as a replacement for the deprecated function.
I could create such a PR tomorrow.

To be honest, I overlooked the _. Clearly, this always was in the "do we want this" gray area, even assuming there is a public C API, but if that's not true anymore, I agree it's definitely best to remove py::make_simple_namespace().

joukewitteveen force-pushed the feature-make_namespace branch from dff2903 to f80db19 Compare February 1, 2021 10:23

joukewitteveen force-pushed the feature-make_namespace branch 2 times, most recently from ca210c7 to 913f382 Compare February 1, 2021 15:49

YannickJadoul reviewed Feb 1, 2021

View reviewed changes

YannickJadoul added this to the v2.7 milestone Feb 1, 2021

joukewitteveen force-pushed the feature-make_namespace branch from 913f382 to 9ebef6f Compare February 2, 2021 08:56

YannickJadoul reviewed Feb 2, 2021

View reviewed changes

joukewitteveen force-pushed the feature-make_namespace branch from 9ebef6f to 861a644 Compare February 2, 2021 14:02

joukewitteveen force-pushed the feature-make_namespace branch from 861a644 to c249fc1 Compare February 15, 2021 12:19

joukewitteveen changed the title ~~Add make_namespace function and tests~~ Add namespace_ type and tests Feb 15, 2021

joukewitteveen force-pushed the feature-make_namespace branch from c249fc1 to e9dc8d7 Compare March 1, 2021 08:40

joukewitteveen changed the title ~~Add namespace_ type and tests~~ Add make_namespace function and tests Mar 1, 2021

YannickJadoul approved these changes Mar 1, 2021

View reviewed changes

joukewitteveen mentioned this pull request Mar 4, 2021

Create sequences using initializer list #2451

Open

henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021

Skylion007 requested a review from rwgk August 13, 2021 15:52

rwgk approved these changes Aug 18, 2021

View reviewed changes

joukewitteveen force-pushed the feature-make_namespace branch from 71e95ae to fce90ef Compare August 22, 2021 16:45

joukewitteveen force-pushed the feature-make_namespace branch from fce90ef to 536fa59 Compare August 24, 2021 15:25

joukewitteveen requested a review from rwgk August 24, 2021 15:27

rwgk reviewed Aug 24, 2021

View reviewed changes

tests/test_pytypes.cpp Outdated Show resolved Hide resolved

joukewitteveen force-pushed the feature-make_namespace branch from 536fa59 to d4f19c1 Compare August 24, 2021 15:48

Add make_simple_namespace function and tests

02e38e9

joukewitteveen force-pushed the feature-make_namespace branch from d4f19c1 to 02e38e9 Compare August 24, 2021 15:49

rwgk approved these changes Aug 24, 2021

View reviewed changes

rwgk changed the title ~~Add make_namespace function and tests~~ Add make_simple_namespace function and tests Aug 24, 2021

Skylion007 approved these changes Aug 26, 2021

View reviewed changes

rwgk merged commit 031a700 into pybind:master Aug 26, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 26, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021

henryiii reviewed Oct 17, 2021

View reviewed changes

This was referenced Oct 17, 2021

bpo-45482 Add PySimpleNamespace_New() function python/cpython#28970

Closed

fix: deprecate make_simple_namespace, fix Python 3.11 #3374

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2840

Closed

Add make_simple_namespace function and tests #2840

Add make_simple_namespace function and tests #2840

Conversation

joukewitteveen commented Feb 1, 2021 • edited by henryiii Loading

Description

Suggested changelog entry:

joukewitteveen commented Feb 1, 2021

YannickJadoul commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joukewitteveen commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstaletic commented Feb 2, 2021

YannickJadoul commented Feb 2, 2021

joukewitteveen commented Feb 2, 2021

joukewitteveen commented Feb 15, 2021

bstaletic commented Feb 15, 2021

joukewitteveen commented Feb 15, 2021

bstaletic commented Feb 15, 2021

joukewitteveen commented Feb 15, 2021

bstaletic commented Feb 15, 2021

YannickJadoul commented Feb 15, 2021 • edited Loading

YannickJadoul commented Feb 15, 2021

joukewitteveen commented Feb 25, 2021

YannickJadoul commented Feb 26, 2021

joukewitteveen commented Mar 1, 2021

YannickJadoul left a comment

Choose a reason for hiding this comment

joukewitteveen commented Mar 1, 2021

YannickJadoul commented Mar 1, 2021 • edited Loading

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joukewitteveen commented Aug 22, 2021

Skylion007 commented Aug 24, 2021

rwgk commented Aug 24, 2021

joukewitteveen commented Aug 24, 2021

rwgk commented Aug 24, 2021

rwgk left a comment

Choose a reason for hiding this comment

rwgk commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joukewitteveen commented Feb 1, 2021 •

edited by henryiii

Loading

YannickJadoul commented Feb 15, 2021 •

edited

Loading

YannickJadoul commented Mar 1, 2021 •

edited

Loading