-
Notifications
You must be signed in to change notification settings - Fork 6
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
Python Fault Handling #22
base: master
Are you sure you want to change the base?
Conversation
@@ -952,9 +952,9 @@ class BFIndex | |||
} | |||
}; | |||
|
|||
PYBIND11_PLUGIN(hnswlib) | |||
PYBIND11_PLUGIN(cpp_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.
does this change the name of the module / what does this change do?
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 changes the name of the python bindings package
] | ||
|
||
# compatibility when run in python_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.
just for my understanding - why is this ok to remove?
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 was additional complexity for if you wanted to run setup.py inside the python_bindings
directory - we don't need that.
@@ -31,7 +25,7 @@ | |||
|
|||
ext_modules = [ | |||
Extension( | |||
"hnswlib", | |||
"hnswlib.cpp_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.
for my understanding - is this the module path
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.
yes, this is the module path for the extension module being added here, in this case the cpp bindings are now a submodule of the hnswlib module
@@ -0,0 +1,4 @@ | |||
import faulthandler | |||
from .cpp_bindings import * |
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 the rexport that makes everything appear under hnswlib?
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.
yes
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 makes sense broadly, just have some questions for my understanding.
Repackaging to add the python
faulthandler
to help better identify issues in the python bindings when / if they happen.You can test the effects of this by checking out and building 6ede4f5
This adds two methods to
hnswlib
-raise_segv
andraise_abort
- calling these will cause a crash, but the faulthandler will catch it and produce at least part of a stack trace before the interpreter exits. This is reverted in the subsequent commit.This PR also expands python tests to 3.11 and 3.12, and removes the examples folder from tests. The reason to remove the examples folder is because there are no tests in it, and the 3.12 version of unittest returns code 5 if no tests are run - this is better procedurally, and the actual bindings tests plus the C++ tests exercise the full surface.