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

Adds set_name method of pybind11::capsule class #3866

Merged
merged 12 commits into from
Apr 14, 2022

Conversation

oleksandr-pavlyk
Copy link
Contributor

This calls PyCapsule_SetName on the underlying Python object.

Description

Definition of pybind11::capsule class was missing set_name method.

This is useful if using pybind11 to implement DLPack protocol which requires to rename consumed capsule.

Suggested changelog entry:

pybind11::capsule::set_name added to mutate the name of the capsule instance.

@Skylion007
Copy link
Collaborator

Okay, we just need to fix our custom destructors with PyCapsule_GetName() instead of assuming the name is null.

@rwgk
Copy link
Collaborator

rwgk commented Apr 12, 2022

Okay, we just need to fix our custom destructors with PyCapsule_GetName() instead of assuming the name is null.

I think it's too complicated, trying to establish a safe context for managing the lifetime of name. Maybe even impossible, because we don't name who owns the passed name (is it static, or system malloc, from a pool, etc.). To not turn this into something big, I'd just add the thin wrapper (as-is) with the suggested comment.

include/pybind11/pytypes.h Outdated Show resolved Hide resolved
include/pybind11/pytypes.h Outdated Show resolved Hide resolved
This calls PyCapsule_SetName on the underlying capsule object.

modified destructors to query capsules's Name

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Handle possible exception thrown by PyCapsule_GetName

Also removed accidentally reintroduced use of `const char *&`.

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fixed function name
@rwgk
Copy link
Collaborator

rwgk commented Apr 13, 2022

Looks good to me but (3.9-dbg (deadsnakes) • Valgrind • x64) ...

terminate called after throwing an instance of 'pybind11::error_already_set'
  what():  RuntimeError: Unable to cast Python instance of type <class 'numpy.ndarray'> to C++ type 'Eigen::Ref<Eigen::Matrix<double, -1, -1, 0, -1, -1>, 0, Eigen::Stride<-1, -1> >'
Fatal Python error: Aborted

huh?
(just to say I don't see what's wrong; needs debugging)

include/pybind11/pytypes.h Outdated Show resolved Hide resolved
include/pybind11/pytypes.h Outdated Show resolved Hide resolved
If PyCapsule_GetName raises an error we should write as unraisable
to consume it and notify user, and then restore the error in flight if any.
This way this method called from destructor would not modify interpreter
error state.
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.

Thanks Oleksandr!

include/pybind11/pytypes.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

This is actually a better solution then what I was going to suggest (as we still throw errors for if we can't get the context or the destructor is nullptr). Regardless, we can consider fixing those in a separate PR.

@Skylion007 Skylion007 merged commit fa98804 into pybind:master Apr 14, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 14, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 9, 2022
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