-
Notifications
You must be signed in to change notification settings - Fork 249
[WIP] Interoperability with other Python binding frameworks #1140
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
base: master
Are you sure you want to change the base?
Conversation
6dc5a7e
to
d9ee371
Compare
I am so excited! Thank you for your hard work on this @oremanj. I will do a thorough review in the coming week. Two quick questions just based on the summary: do you plan to also make such a PR for pybind11? Would the idea there be to remove the "conduit" feature and replace it with pymetabind? |
Yes; I've started on it already. It's more awkward and less zero-cost than this one due to the presumed need to avoid a pybind11 ABI version bump, but I haven't hit any blockers yet. (An additional type map lookup will be needed on every failed load if there are any imported foreign bindings, rather than knowing whether the particular type of interest has foreign bindings.)
I don't know if that would be palatable, since the conduit feature has already been released and might need to be supported for a long time. Unfortunately I missed the window to be included in pybind11's most recent ABI break, which occurred with the 3.0 release in July; I'm guessing the next one might not be for a long time after that. The two features don't clash, though of course there's some cost to doing the attribute lookup for the conduit method when it's not needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @oremanj,
this is a very impressive piece of work. I did a first pass over the PR, please see my feedback and questions attached.
Besides these, hear are three additional high level questions:
This PR lacks tests. I am sure you have them on your end as part of the development effort. What would be to test this feature in practice (via CI) so that we can ensure it runs and keeps on running. Would it make sense to have a separate test repository (to avoid duplication) that gets pulled into the CI matrices of both nanobind and pybind11 so that any breaking changes of either project can be caught before shipping a new revision?
Are there features of nanobind that are not supported by pymetabind? Any caveats?
Is there anything to watch out regarding leak tracking when multiple frameworks are involved? I noticed that pymetabind's leak_safe
flag is not used in the actual implementation.
Thanks,
Wenzel
@@ -330,7 +335,7 @@ endfunction() | |||
|
|||
function(nanobind_add_module name) | |||
cmake_parse_arguments(PARSE_ARGV 1 ARG | |||
"STABLE_ABI;FREE_THREADED;NB_STATIC;NB_SHARED;PROTECT_STACK;LTO;NOMINSIZE;NOSTRIP;MUSL_DYNAMIC_LIBCPP;NB_SUPPRESS_WARNINGS" | |||
"STABLE_ABI;FREE_THREADED;NB_STATIC;NB_SHARED;PROTECT_STACK;LTO;NOMINSIZE;NOSTRIP;MUSL_DYNAMIC_LIBCPP;NB_SUPPRESS_WARNINGS;NO_INTEROP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general question is whether this feature should be opt-in or opt-out. Given that it adds overheads (even if small), my tendency would be to make it opt-in. (e.g. INTEROP
instead of NO_INTEROP
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the feature becomes opt-in, would you reverse the polarity of the macro as well? In other words, NB_DISABLE_FOREIGN
becomes NB_ENABLE_FOREIGN
.
Obviously, other build systems do not use nanobind-config.cmake
. By default, any macros you add would not be defined. Developers would opt-in by defining the new macro.
@@ -94,6 +94,11 @@ struct type_caster<std::unique_ptr<T, Deleter>> { | |||
// Stash source python object | |||
src = src_; | |||
|
|||
// Don't accept foreign types; they can't relinquish ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be guarded with an #ifdef
to only compile in the case of interop support being enabled?
Minor: in the nanobind codebase, braces are omitted for if
statements with a simple 1-line body.
@@ -114,7 +111,7 @@ struct type_data { | |||
const char *name; | |||
const std::type_info *type; | |||
PyTypeObject *type_py; | |||
nb_alias_chain *alias_chain; | |||
void *foreign_bindings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this field deserves a comment given that it's unconditionally present (even if interop support is disabled).
In what way is the role of the original alias_chain
subsumed?
detail::nb_type_set_foreign_defaults(export_all, import_all); | ||
} | ||
template <class T = void> | ||
inline void import_foreign_type(handle type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will require documentation. I am not sure why a foreign type would need to be explicitly imported/exported through this API in user code. Isn't this something that the framework will do automatically for us?
return detail::nb_type_lookup(&typeid(detail::intrinsic_t<T>)); | ||
return detail::nb_type_lookup(&typeid(detail::intrinsic_t<T>), false); | ||
} | ||
template <typename T> handle maybe_foreign_type() noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this function? I don't think it is called anywhere? The alternative would be to add a bool
parameter to type()
.
if we need to have a function, then I would prefer the name type_maybe_foreign
.
internals->foreign_imported_any = true; | ||
|
||
auto add_to_list = [binding](void *list_head) -> void* { | ||
if (!list_head) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove braces
if (!internals->foreign_registry) | ||
register_with_pymetabind(internals); | ||
pymb_framework* foreign_self = internals->foreign_self; | ||
pymb_binding* binding = pymb_get_binding(pytype); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a naïve question. Why would we need to look up __pymetabind_binding__
? Won't we be notified of new frameworks/bindings using the hooks?
Or is the idea that the metabind feature is enabled lazily, and if we join the party late then that registry is empty to start with? Still then, I am wondering why we can't populate our own tables from the list of types in the registry, without having to touch the __pymetabind_binding__
member.
* - 0b10: pymb_binding*: This C++ type is not bound in the current nanobind | ||
* domain but is bound by a single other framework. | ||
* | ||
* - 0b11: nb_foreign_seq*: This C++ type is not bound in the current nanobind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naïve question: what is the purpose of mapping a type to multiple frameworks?
Previously, from-python or to-python conversions might not work out, because a type is not registered at all. With pymetabind, there is now a way out because we can use the other framework to do the conversion for us. If multiple frameworks bind the same type, then this adds complexity. (e.g. the alloca() / complex locking code path in nb_foreign.cpp). I am wondering if we can end up with a simpler solution when scrapping this.
* (Once we have a list, we don't switch back to a | ||
* singleton; see nb_type_try_foreign() for why.) | ||
* | ||
* The `slow` map is the source of truth. Its lookups perform string comparisons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I should have added this.
struct nb_inst_seq { | ||
PyObject *inst; | ||
nb_inst_seq *next; | ||
/// nanobind maintains several maps where there is usually a single entry, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I should have added this.
See also pybind/pybind11#5800, the same feature for pybind11.
pymetabind is a proposed standard for Python <-> C-ish binding frameworks to be able to find and work with each other's types. For example, assuming versions of both nanobind and pybind11 that have adopted this standard, it would allow a nanobind-bound function to accept a parameter whose type is bound using pybind11, or to return such a type, or vice versa. Interoperability between different ABI versions or different domains of the same framework is supported under the same terms as interoperability between different frameworks. Compared to pybind11's
_pybind11_conduit_v1_
API, this one also supports implicit conversions and to-Python conversions, and should have significantly less overhead.The essence of this technique has been in use in production by my employer for a couple of years now to enable a large amount of pybind11 binding code to be ported to nanobind one compilation unit at a time. Almost everything that works natively works across framework boundaries too, at only a minor performance cost. Inheritance relationships and relinquishment (from-Python conversion of
unique_ptr<T>
) don't work cross-framework, but those are the only limitations I'm aware of.This PR adds nanobind support for exposing nanobind types to pymetabind for other frameworks to use ("exporting") and using other frameworks' types that they have exposed to pymetabind ("importing"). Types bound by a different framework than the extension module's own nanobind domain are called "foreign". There are some internals changes to allow foreign types to be represented in the same type maps as native nanobind types; this also includes an updated version of the per-thread fast c2p map that allows safe removal of types (since we can make our own types immortal but we can't force everyone else to make their types immortal). It is possible to compile nanobind without the code to support interop, using the new cmake option
NO_INTEROP
.Current status: nominally code complete and existing tests pass, but I haven't added interop-specific tests or public-facing docs yet.
Performance: I have not yet measured the performance impact of this change, but I expect it to be quite low in situations where the foreign bindings don't need to be used. The new type_c2p_fast caches negative lookups, and we note whether any foreign bindings exist for a C++ type at the same time as we look up the nanobind type for it. If any foreign bindings have been imported, we do need to look up in type_c2p_fast before failing in some cases where we previously could avoid a lookup completely. When the foreign bindings do need to be used to perform a cast, they require a second c2p_fast lookup and some likely-modest indirection overhead.
Memory cost: Exporting a type allocates a 56-byte structure, a capsule object to wrap it, and adds that capsule object to the type's dictionary. Importing a type adds a new entry to the type_c2p_slow map.
Code size: With
NO_INTEROP
,size libnanobind.a
adds up to 8533 bytes smaller than baseline on my machine (an arm64 mac), probably due to reusingnb_ptr_map
for the type_c2p_fast map. WithoutNO_INTEROP
libnanobind.a is 8983 bytes larger than baseline.Things that need to happen before this can be released:
[ ] add unit tests
[ ] add user-facing documentation
[ ] test correctness of nanobind/pybind11 interop
[ ] test performance
[ ] solicit feedback from maintainers of other binding libraries
[ ] release pymetabind v1.0, incorporating said feedback