-
Notifications
You must be signed in to change notification settings - Fork 250
[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 all commits
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. |
||
"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 |
||
#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) { | ||
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. 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? |
||
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 |
||
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 |
||
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 | ||
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. Should this be guarded with an Minor: in the nanobind codebase, braces are omitted for |
||
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? |
||
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.