-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor: Weaviate query with filters #3628
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.
LGTM
Thanks @ZanSara 🙏 |
Hi @ZanSara , @bobvanluijt, @masci, I was the one who wrote the Haystack-Weaviate code enabling the BM25 functionality on Weaviate and throwing that exception for BM25 with filters not being supported in Weaviate (https://github.com/deepset-ai/haystack/pull/3628/files#diff-d4e4f2566db6c2fbe852debffcf7830447ecb969d4cdcce6d796fd773eba379bR986) and in parallel have raised an issue for it with Weaviate back in July: weaviate/weaviate#2061 I was just bugging @etiennedi from Weaviate today about the filter support with BM25 and then I was surprised to see this PR in Haystack starting to use that filter support with BM25. My Weaviate issue hasn't been closed and @etiennedi has confirmed that there is still no filter support with BM25 in Weaviate (see weaviate/weaviate#2134 (comment)). Are we absolutely sure that there is filter support for BM25 in Weaviate yet? The odd thing that the unit test in Haystack was changed correctly by @ZanSara to remove the error catch (https://github.com/deepset-ai/haystack/pull/3628/files#diff-bc2015fb1dc5e70c32cf9c8bba6a2497388e0293d385cb4e614cd9cea534b679L158) and I assume that the tests were running successfully. Something doesn't add up. Either the unit test on Haystack side was not running, or Weaviate is not properly throwing that error now or @etiennedi is wrong and in fact that filtered BM25 is now supported in Weaviate. Let me investigate. UPDATE1: I have confirmed @etiennedi 's finding, Weaviate still throws the expected error with filters on BM25 (using Weaviate UPDATE2: I have confirmed that the Weaviate error is still properly thrown from Haystack too when trying to use BM25 with filters, just this PR has a coding error at https://github.com/deepset-ai/haystack/pull/3628/files#diff-d4e4f2566db6c2fbe852debffcf7830447ecb969d4cdcce6d796fd773eba379bR999, as that should be on the
Once that IN SUMMARY: |
Side note: thanks for the detailed write-up @zoltan-fedor 👍 |
Related Issues
Proposed Changes:
How did you test it?
Notes for the reviewer
n/a
Checklist