Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Python Fault Handling #22

wants to merge 6 commits into from

Conversation

atroyn
Copy link

@atroyn atroyn commented Aug 22, 2024

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 and raise_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.

@atroyn atroyn requested a review from HammadB August 22, 2024 03:05
@atroyn atroyn marked this pull request as ready for review August 22, 2024 03:05
@@ -952,9 +952,9 @@ class BFIndex
}
};

PYBIND11_PLUGIN(hnswlib)
PYBIND11_PLUGIN(cpp_bindings)
Copy link

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?

Copy link
Author

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
Copy link

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?

Copy link
Author

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",
Copy link

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

Copy link
Author

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 *
Copy link

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link

@HammadB HammadB left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants