-
Notifications
You must be signed in to change notification settings - Fork 35
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
Replace pybind11 with nanobind in Catalyst Runtime #1293
base: main
Are you sure you want to change the base?
Conversation
Unlike pybind11, which raises exceptions of the same type as native Python, nanobind wraps raised exceptions as RuntimeErrors.
Still TODO: Find a way to remove calls to #ifdef INITIALIZE_PYTHON
if (!Py_IsInitialized()) {
pybind11::initialize_interpreter();
}
#endif Nanobind has no plans to port the equivalent feature from pybind11. See https://nanobind.readthedocs.io/en/latest/porting.html#removed-features:
|
EDIT: Ah, I see that this is equivalent to Embedding. Yes, we will need a way to test it. Maybe we should test the runtime from python? |
@erick-xanadu thanks for this. I'm wondering now if we only need to use pybind11 for the sake of testing, then maybe this is fine. Converting the tests from C++ to Python could take some time. The big selling point of nanobind for us is its ability to use the Stable ABI to allow us to ship one wheel across minor Python versions. If I understand correctly, the Maybe one thing we could do is move the #ifdef INITIALIZE_PYTHON
if (!Py_IsInitialized()) {
pybind11::initialize_interpreter();
}
#endif snippets into the tests themselves to make it explicit that we only use pybind11 to embed a Python interpreter for testing. What do you think? |
This reverts commit c7807fa.
The macOS (arm64) "Build Wheels" actions are failing for Python 3.11 and 3.12 because there are multiple versions installed on the runner images:
It's pip installing nanobind with the system versions, but then CMake is running TODO: Find a way to tell CMake to find the system python installation for the given version, e.g. by specifying a root path to search in. |
Done in fe3dbf8. |
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.
Nice work! 💯
} | ||
} | ||
|
||
// TODO: These operations are not yet functional |
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.
Still working on this one?
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.
I'm not so familiar with system, but the Python functions that are called for the Expval
and Var
wrappers, py_expval
and py_var
, defined in runtime/lib/backend/openqasm/openqasm_python_module.cpp, look off to me. They both return the values
attribute of the circuit that was run. However, when I run this example locally, values
is an empty list. Both the wrappers expect a result of type double
, so it throws a bad_cast
exception.
It's not clear to me what the correct way to extract the expval and var for this circuit is; do you happen to know?
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.
I'm not sure either. @maliasadi ?
runtime/lib/registry/CMakeLists.txt
Outdated
# Use a consistant suffix ".so" rather than, e.g. ".abi3.so" (when using the Stable ABI) or | ||
# ".cpython-3xx-darwin.so". This ensures we can locate it when calling `dlopen(LIBREGISTRY)` in | ||
# runtime/lib/capi/RuntimeCAPI.cpp. | ||
set_target_properties(catalyst_callback_registry PROPERTIES SUFFIX ".so") |
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.
Ah nice, I think this answers my previous question then!
Unrelated to this PR, but at some point I actually tried normalizing all our library names to .so
so we would never have to worry about different extensions on different platforms (which is a common source of bugs during development). Currently, on linux we use .so
everywhere, and on mac we have a mixture of .so
and .dylib
. @erick-xanadu What would you think of this?
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 comment I left here might not be strictly correct that it ensures we can locate the file, but it certainly simplifies the process if all our libraries use the same suffix (assuming that doesn't introduce other issues; it seems to be fine, though).
Edit: updated the comment in e528fae
Using `:=` rather than `?=` since these options should not be configurable.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1293 +/- ##
==========================================
+ Coverage 0.37% 81.35% +80.98%
==========================================
Files 17 74 +57
Lines 1346 8180 +6834
Branches 73 858 +785
==========================================
+ Hits 5 6655 +6650
- Misses 1341 1468 +127
- Partials 0 57 +57 ☔ View full report in Codecov by Sentry. |
Context:
The Catalyst runtime makes use of Pybind11 in two places:
This PR is part of a larger effort to replace all pybind11 code with nanobind.
Description of the Change:
This change replaces all the pybind11 code in runtime/lib/registry/Registry.cpp and in runtime/lib/backend/openqasm/openqasm_python_module.cpp with the equivalent nanobind objects and operations.
Benefits:
See Epic 68472 for a list of nanobind's benefits.
[sc-72877]