-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
testMergeStability failing for Knn formats #13640
Comments
@msokolov ^ I haven't been able to look into fixing it yet. Just now noticed it. |
hmm thanks I'll take a look soon |
I didn't know about this constraint until now. Basically what happens is during merge we check for disconnected components and attempt to add connections to connect them. So it makes sense we might be adding some bytes to the vex file. Maybe a way to avoid this is to skip recreating the HNSW graph when merging a single segment. Honestly I don't know why we would be doing that. I'll dig a bit more/ |
OK, I guess we would need to actually build a graph when merging a single segment in case there are deletions. In any case it would be nice if the graph reconnection were stable. This test exposes some interesting problems! Yay for our tests. |
Apparently that patch did not fix all the things; this failure got generated on that patched version (reporoduced for me on 9x branch):
|
found and fixed a branch_9x-only problem. Hopefully this calms down now |
Not to be the bearer of bad news:
Fails on main. Its interesting that the old formats are adjusting their outputs at all. I would expect all older formats to be unchanged. I wonder if the default HnswGraph builder behavior changed. |
See #13654 |
No it's not |
It's been a few days since I've seen any automated failures and all known instances have been addressed. I think we can close, wdyt @benwtrent ? |
agreed, closing. |
@msokolov its reared its head again.
Makes me think that we need to mark this merging as unstable. I wanted to verify the format, and verified its:
If I turned off connect components, this seed passes. |
I discovered two other weird behaviors digging into this test failure. But, neither seemed to fix this inconsistency: #14174 |
Curious if you tried git bisect to see if there was any recent change that reintroduced this? |
I did the git bisect dance and found this test seed starts failing with Randomize KnnVector codec params in RandomCodec. That was basically a test change only though so I guess maybe it merely exposed this condition |
Interesting, the randomized case isn't anything special. Its just a plain 'ole Lucene99Hnsw index. No quantization or anything :/ |
Description
All KNN formats are periodically failing
testMergeStability
.I have verified its due to #13566
The stability failure is due to a different size in the
vex
(e.g. vector graph connections).Gradle command to reproduce
The text was updated successfully, but these errors were encountered: