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

bpo-45482 Add PySimpleNamespace_New() function #28970

Closed
wants to merge 2 commits into from
Closed

bpo-45482 Add PySimpleNamespace_New() function #28970

wants to merge 2 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 15, 2021

Add a new PySimpleNamespace_New() function to create a simple
namespace: C API of the types.SimpleNamespace type.

  • Move Include/namespaceobject.h to Include/cpython/namespaceobject.h.
  • Rename _PyNamespace_New() to PySimpleNamespace_New()
  • Rename _PyNamespace_Type to _PySimpleNamespace_Type
  • PySimpleNamespace_New() now calls the ns_dict.update(attrs) method,
    rather than calling PyDict_Update(ns_dict, attrs), to raise more
    accurate exceptions if the argument has the wrong type or format.
  • Add _testcapimodule.PySimpleNamespace_New().
  • Add test_types.SimpleNamespaceTests.test_capi().

https://bugs.python.org/issue45482

Add a new PySimpleNamespace_New() function to create a simple
namespace: C API of the types.SimpleNamespace type.

* Move Include/namespaceobject.h to Include/cpython/namespaceobject.h.
* Rename _PyNamespace_New() to PySimpleNamespace_New()
* Rename _PyNamespace_Type to _PySimpleNamespace_Type
* PySimpleNamespace_New() now calls the ns_dict.update(attrs) method,
  rather than calling PyDict_Update(ns_dict, attrs), to raise more
  accurate exceptions if the argument has the wrong type or format.
* Add _testcapimodule.PySimpleNamespace_New().
* Add test_types.SimpleNamespaceTests.test_capi().
@vstinner vstinner changed the title bpo-45482 Add PySimpleNamespace_New() bpo-45482 Add PySimpleNamespace_New() function Oct 15, 2021
@vstinner
Copy link
Member Author

cc @pablogsal @ambv @corona10

Maybe the function must require a dict and check explicitly if attribute names are non-empty strings.

@vstinner
Copy link
Member Author

It would be nice to replace usage of structseq with SimpleNamespace, like sys.flags. But sadly, a SimpleNamespace is mutable, whereas structseq is used because it's immutable.

Maybe we need an immutable namespace? Simple API to build a "read-only". object.

@vstinner
Copy link
Member Author

Maybe the function must require a dict and check explicitly if attribute names are non-empty strings.

I added a check to reject attribute names which are not str.

The check is implemented with PyArg_ValidateKeywordArguments(). This function doesn't reject empty strings. It's written to validate arguments of functions defined as def func(**kwargs): the Python grammar disallows empty strings. types.SimpleNamespace currently accepts empty strings:

>>> import types
>>> ns=types.SimpleNamespace(**{'': 5})
>>> getattr(ns, '')
5

I guess that it's an implementation detail.

@vstinner
Copy link
Member Author

I created this issue while cleaning the C API. I'm not fully convinced that PySimpleNamespace_New() must be made public.

The other choice is to move the function to the internal C API.

@pablogsal
Copy link
Member

I created this issue while cleaning the C API. I'm not fully convinced that PySimpleNamespace_New() must be made public.

The other choice is to move the function to the internal C API.

IMHO I would prefer to not have more public C API functions if we can avoid it. Would it be possible to make it internal?

@vstinner
Copy link
Member Author

@pablogsal:

IMHO I would prefer to not have more public C API functions if we can avoid it. Would it be possible to make it internal?

Ok, I created the opposite PR, remove private functions from the public C API, move them to the internal C API: #28975

@vstinner
Copy link
Member Author

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.

@vstinner vstinner closed this Oct 15, 2021
@vstinner vstinner deleted the pynamespace branch October 15, 2021 13:21
@henryiii
Copy link
Contributor

We are actually using _PyNamespace_New in pybind11 since pybind/pybind11#2840, so it would have been useful for the wrapper we provided. I'm assuming getting types.SimpleNamespace is the proper workaround for us.

@vstinner
Copy link
Member Author

We are actually using _PyNamespace_New in pybind11 since pybind/pybind11#2840, so it would have been useful for the wrapper we provided. I'm assuming getting types.SimpleNamespace is the proper workaround for us.

Can you try to get types.SimpleNamespace? I have the same issue with the _testmultiphase module which I had to modify to access to the internal C API. I guess that a similar change should be made.

@henryiii
Copy link
Contributor

I tested it out in pybind/pybind11#3374 - it's 20% slower, but that's all (and is easy to write with pybind11). I'm advocating we drop our (very recently added) py::make_simple_namespace in favor of manually writing the import, unless this becomes public in the C API.

@vstinner
Copy link
Member Author

I tested it out in pybind/pybind11#3374 - it's 20% slower

You call import + getattr at each call. If you are concerned about performance, you could cache the SimpleNamespace type between calls, no?

@henryiii
Copy link
Contributor

In that case, it's 2% faster, actually.

@joukewitteveen
Copy link
Contributor

Faster than using the C API? That's a surprise!

@vstinner: I'm reading up on how all this works. Is it true that the only way to instantiate an object with custom attributes through the C API is as Struct Sequence Objects? Interestingly, these objects are more complicated than Simple Namespaces. I am trying to understand what the guiding principles are regarding design and evolution of the interpreter. Maybe all decisions by now are guided by historical baggage? Superficially, the current MR looks quite sensible to me. A little more so than #28975.

@vstinner
Copy link
Member Author

This PR is closed. If you have a feature request, please open an issue at https://bugs.python.org/ ;-)

@encukou
Copy link
Member

encukou commented Oct 20, 2021

"Historical baggage" sounds almost right :)

StructSequence objects are used in APIs that used to return tuples. They need to be compatible with tuples, and they need to be as close as possible to tuples performance-wise. That's what makes them complicated.

But calling it "historical baggage" is too much of a simplification. Tuples are a natural way to return two or three values, but when you later find out you need one more, it can't be added to the tuple because that would break unpacking. So in a new version the tuple gets converted to StructSequence and a named-only attribute is added. It can happen in the future, and it's not an issue when it does.

speth added a commit to speth/cantera that referenced this pull request Nov 16, 2022
This function was removed from Python.h in Python 3.11. See
python/cpython#28970
ischoegl pushed a commit to Cantera/cantera that referenced this pull request Nov 21, 2022
This function was removed from Python.h in Python 3.11. See
python/cpython#28970
jongyoonbae pushed a commit to jongyoonbae/cantera that referenced this pull request Feb 2, 2023
This function was removed from Python.h in Python 3.11. See
python/cpython#28970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants