-
Notifications
You must be signed in to change notification settings - Fork 255
[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?
Changes from 3 commits
036c91c
26971e0
940500d
6769a72
54c5e11
2c5e718
4fb9c85
245fa5a
6fe43c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,6 +190,7 @@ function (nanobind_build_library TARGET_NAME) | |
${NB_DIR}/src/nb_static_property.cpp | ||
${NB_DIR}/src/nb_ft.h | ||
${NB_DIR}/src/nb_ft.cpp | ||
${NB_DIR}/src/nb_foreign.cpp | ||
${NB_DIR}/src/common.cpp | ||
${NB_DIR}/src/error.cpp | ||
${NB_DIR}/src/trampoline.cpp | ||
|
@@ -236,6 +237,10 @@ function (nanobind_build_library TARGET_NAME) | |
target_compile_definitions(${TARGET_NAME} PUBLIC NB_FREE_THREADED) | ||
endif() | ||
|
||
if (TARGET_NAME MATCHES "-local") | ||
target_compile_definitions(${TARGET_NAME} PRIVATE NB_DISABLE_FOREIGN) | ||
endif() | ||
|
||
# Nanobind performs many assertion checks -- detailed error messages aren't | ||
# included in Release/MinSizeRel/RelWithDebInfo modes | ||
target_compile_definitions(${TARGET_NAME} PRIVATE | ||
|
@@ -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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just chiming in for another vote for opt-in. I imagine that most projects don't need to pay the cost (as the bindings will be self contained), and the ones that do would probably just use it to use as a transition period and then turn it off again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The authors of a particular extension module don't generally know when they build it whether anyone will want to use its types from a different framework (or a different ABI version of the same framework). I think this is what pybind11 was referring to in their rationale for adding the |
||
"NB_DOMAIN" "") | ||
|
||
add_library(${name} MODULE ${ARG_UNPARSED_ARGUMENTS}) | ||
|
@@ -375,6 +380,10 @@ function(nanobind_add_module name) | |
set(libname "${libname}-ft") | ||
endif() | ||
|
||
if (ARG_NO_INTEROP) | ||
set(libname "${libname}-local") | ||
endif() | ||
|
||
if (ARG_NB_DOMAIN AND ARG_NB_SHARED) | ||
set(libname ${libname}-${ARG_NB_DOMAIN}) | ||
endif() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,9 +91,6 @@ enum class type_init_flags : uint32_t { | |
all_init_flags = (0x1f << 19) | ||
}; | ||
|
||
// See internals.h | ||
struct nb_alias_chain; | ||
|
||
// Implicit conversions for C++ type bindings, used in type_data below | ||
struct implicit_t { | ||
const std::type_info **cpp; | ||
|
@@ -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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The interop-disabled flag doesn't change the ABI version string, so we can't conditionally include fields based on its presence. Will add a comment. The original |
||
#if defined(Py_LIMITED_API) | ||
PyObject* (*vectorcall)(PyObject *, PyObject * const*, size_t, PyObject *); | ||
#endif | ||
|
@@ -332,6 +329,19 @@ inline void *type_get_slot(handle h, int slot_id) { | |
#endif | ||
} | ||
|
||
// nanobind interoperability with other binding frameworks | ||
inline void set_foreign_type_defaults(bool export_all, bool import_all) { | ||
detail::nb_type_set_foreign_defaults(export_all, import_all); | ||
} | ||
template <class T = void> | ||
inline void import_foreign_type(handle type) { | ||
|
||
detail::nb_type_import(type.ptr(), | ||
std::is_void_v<T> ? nullptr : &typeid(T)); | ||
} | ||
inline void export_type_to_foreign(handle type) { | ||
detail::nb_type_export(type.ptr()); | ||
} | ||
|
||
template <typename Visitor> struct def_visitor { | ||
protected: | ||
// Ensure def_visitor<T> can only be derived from, not constructed | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -667,15 +667,19 @@ class iterable : public object { | |
|
||
/// Retrieve the Python type object associated with a C++ class | ||
template <typename T> handle type() noexcept { | ||
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 commentThe 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 if we need to have a function, then I would prefer the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's like nb::type() but it can return a foreign type also. I found it useful in client code. I'm indifferent between a bool parameter and separate function, so much so that I seem to have made different choices for two adjacent functions - regardless of which direction we go, we can pick one scheme and use it for both. |
||
return detail::nb_type_lookup(&typeid(detail::intrinsic_t<T>), true); | ||
} | ||
|
||
template <typename T> | ||
NB_INLINE bool isinstance(handle h) noexcept { | ||
NB_INLINE bool isinstance(handle h, bool foreign_ok = false) noexcept { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I am wondering about the role of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type and isinstance are the only two pieces of public API that expose this question. I included the option to allow foreign or not for backward compatibility; I was concerned about cases where client code might assume the result of |
||
if constexpr (std::is_base_of_v<handle, T>) | ||
return T::check_(h); | ||
else if constexpr (detail::is_base_caster_v<detail::make_caster<T>>) | ||
return detail::nb_type_isinstance(h.ptr(), &typeid(detail::intrinsic_t<T>)); | ||
return detail::nb_type_isinstance(h.ptr(), &typeid(detail::intrinsic_t<T>), | ||
foreign_ok); | ||
else | ||
return detail::make_caster<T>().from_python(h, 0, nullptr); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
if (!src.is_none() && !inst_check(src)) { | ||
return false; | ||
} | ||
|
||
/* Try casting to a pointer of the underlying type. We pass flags=0 and | ||
cleanup=nullptr to prevent implicit type conversions (they are | ||
problematic since the instance then wouldn't be owned by 'src') */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,12 +217,28 @@ builtin_exception::~builtin_exception() { } | |
|
||
NAMESPACE_BEGIN(detail) | ||
|
||
void register_exception_translator(exception_translator t, void *payload) { | ||
nb_translator_seq *cur = &internals->translators, | ||
*next = new nb_translator_seq(*cur); | ||
cur->next = next; | ||
cur->payload = payload; | ||
cur->translator = t; | ||
void register_exception_translator(exception_translator t, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious about the use of atomics here, it seems complicated. Exceptions and class definitions are created when a module is loaded, and those regions run single-threaded even in free-threaded builds. Perhaps it would be more clear to document that this function is unsafe to use concurrently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AIUI, while module imports serialize against each other (you can't have two modules importing at the same time), they don't serialize against ordinary execution. There may be concurrent accesses to the list of exception translators from other threads, which would create a data race (at least formally) if we didn't use atomics here. We're no longer limited to additions at the front of the list, so the previous argument that concurrent accesses will see an unchanging sub-list no longer holds. If we assume all translator registrations occur during a module load, then I do think we could get away with using relaxed atomics for the next pointers and dropping the compare-exchange. But doing a fully correct lock-free add is really not much harder, and gives some extra protection against users not doing what we expect them to. Note that any other framework registering itself with pymetabind will result in a new exception translator for us, and we can't really control when that occurs. For example, nanobind doesn't register itself until some types are actually exported. An extension module could expose a Python function that wraps Not all class definitions are created when a module is loaded. For example, the iterator class returned when one uses |
||
void *payload, | ||
bool at_end) { | ||
// We will insert the new translator so it is pointed to by `*insert_at`, | ||
// i.e., so that it is executed just before the current `*insert_at` | ||
nb_maybe_atomic<nb_translator_seq *> *insert_at = &internals->translators; | ||
if (at_end) { | ||
// Insert before the default exception translator (which is last in | ||
// the list) | ||
nb_translator_seq *next = insert_at->load_acquire(); | ||
while (next && next->next.load_relaxed()) { | ||
insert_at = &next->next; | ||
next = insert_at->load_acquire(); | ||
} | ||
} | ||
nb_translator_seq *new_head = new nb_translator_seq{}; | ||
nb_translator_seq *cur_head = insert_at->load_relaxed(); | ||
new_head->payload = payload; | ||
new_head->translator = t; | ||
do { | ||
new_head->next.store_release(cur_head); | ||
} while (!insert_at->compare_exchange_weak(cur_head, new_head)); | ||
} | ||
|
||
NB_CORE PyObject *exception_new(PyObject *scope, const char *name, | ||
|
Uh oh!
There was an error while loading. Please reload this page.