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

(breaking) FIX for unexpected segfault when index dimensions don't add up #413

Draft
wants to merge 10 commits into
base: main-dev
Choose a base branch
from

Conversation

jbrummack
Copy link

This should fix the problem of segfaulting by checking whether the dimensions are adding up and handling the edge case of hamming distance.
Due to cxx::Exception being private I had to create a new error type that wraps C++ Exceptions and can output errors from "rustland".
Therefore this fix could introduce breaking changes
It works in my (small) codebase and passes tests, but due to finding more cases that cause an unexpected segfault in rust, I suggest merging these fixes together later.

@ashvardanian
Copy link
Contributor

If we perform the size check in the C layer - we would avoid breaking changes and could merge as is. How does that sound?

@jbrummack
Copy link
Author

Sounds like a better solution. Adding some more tests in the Rust part is probably a good idea; throwing out the changes in Rust except the test for having different vector sizes and relying on the C layer for making it pass.

@ashvardanian
Copy link
Contributor

Yes, @jbrummack! Please let me know if you'd have time to implement those changes 🤗

@jbrummack
Copy link
Author

I certainly have time to implement more Rust tests, as its probably synergistic with using the library in projects.
I am not really constrained by time, but looking at the c layer would take a while since I don't have much experience with c/c++ (yet).

This reverts commit 8641535.
checks if wrong dimensions are handled properly
@ashvardanian ashvardanian changed the base branch from main to main-dev May 27, 2024 10:15
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.

None yet

2 participants