-
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
Add nightly CI which runs sanitizers #476
Conversation
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.
Thanks for all this work! I think it will greatly improve our confidence for the correctness of the C++ code.
@@ -888,7 +888,7 @@ def test_ingestion_with_batch_updates(tmp_path): | |||
gt_i, gt_d = get_groundtruth(dataset_dir, k) | |||
|
|||
for index_type, index_class in zip(INDEXES, INDEX_CLASSES): | |||
minimum_accuracy = 0.84 if index_type == "IVF_PQ" else 0.99 | |||
minimum_accuracy = 0.83 if index_type == "IVF_PQ" else 0.99 |
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.
Why do you change this value in this PR? I don't think that we should make this accuracy bound that tight. Maybe we can make it 0.8
and avoid test flakiness.
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 just changed it b/c a test failed b/c of this flakiness - going to merge this to keep moving, but if I see it (or others) fail again I'll make them a bit looser to prevent more flakiness, good call.
What
Add a nightly CI job which runs sanitizers.
Note that I've updated so that they pass even if the sanitizer fails. We'll want to remove this once we've fixed all errors, but this is a nice first step.
Testing
This creates the following CI: