-
Notifications
You must be signed in to change notification settings - Fork 130
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
fix(weaviate): support v4.6.3 #1134
Conversation
Thanks @tibor-reiss, did you try it to make sure it works with the new version? |
Hi @nirga, yes, 9 passed, 1 skipped. Update: are there some other tests which need to be done apart from the unit tests? |
@tibor-reiss can you run one of the sample apps that uses weaviate and make sure that you see traces, and add a screenshot here? I vaguely remember that there was a reason why @paolorechia who wrote this instrumentation didn't enable it for v4 |
@nirga Took me some time to set up a fully functioning environment locally - consequently, I have updated the sample apps to accommodate for local work. I adjusted v4 so it resembles v3. Could you please check that it works with openai as well? The free tier was removed, so I tested with cohere. It seems weaviate_v3.py still runs with weaviate-v4, but it will be deprecated at some point. |
@tibor-reiss actually no :/ the GET / DELETE spans you're seeing are instrumented by otel's URLLIB3 that we're installing :/ Looks like the weaviate spans aren't there. You can easily see it by disabling all instrumentations except for weaviate, like this:
From that, I'm guessing that if we update the test files to use the v4 syntax they will also start failing |
9acd429
to
8672dbb
Compare
Thanks for your patience @nirga! Below the updated screenshot. The weaviate api changed quite heavily. Combining their official documentation with the already present instrumentation, now there are references to "private" classes/methods. It works, but it means that this will probably change in the future. Let me know what you think. I bumped the version just for this instrumentation to 0.20.0 since it's quite a big change. Could you please check that the examples (weaviate_v4.py and weaviate_v3.py) work with the openai "backend" as well? |
Thanks @tibor-reiss! Don't update version numbers, they get bumped automatically when we release 😃 So now it works both with old and new versions of weaviate? Or did we drop support for old versions? |
8672dbb
to
9b3776c
Compare
@nirga Oh, sorry, I did not think that you would like to keep support for v3 - installing 0.19.0 with weaviate==3.26.2 would have still worked. Anyway, I have put it back - with minor changes:
So now both versions work - tested both the weaviate_v3.py and weaviate_v4.py. Could you please add the cassettes for test_weaviate_instrumentation? I have generated them but they are with localhost since I am running weaviate via docker. |
edb8ef0
to
4f985a3
Compare
@tibor-reiss the recording of the cassettes don't work for you? |
Good morning @nirga, I can generate them - it just has localhost instead of traceloop :) However, I have noticed that one test fails in test_weaviate_instrumentation.py with vcr enabled. In the old tests (now renamed to test_weaviate_instrumentation_v3.py), there was Example from test_weaviate_instrumentation.py:
It seems to me that vcr does not fetch all details, e.g. |
Hey @tibor-reis! What VCR does is just recording and replaying of HTTP requests, so as long as the database is sending back responses this should work. I can try and look into this as well today. During CI, I think spinning off a local weaviate might be more cumbersome than recording and replaying HTTP responses but if you think it's easier and stable enough - let's do it. Can you assist in fixing the CI yaml though? (it's under the .github folder) |
The reason for some test failures is that grpc is built into weaviate-v4, but this is not supported in vcrpy. As a solution for these tests, I have removed the vcr markers, and added a command line flag ( |
cac8137
to
64df7f3
Compare
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 @tibor-reiss! This looks overall good, 2 small comments:
- Can you fix the lint issues?
- Can you remove the cassettes that are no longer used?
@@ -1,40 +1,37 @@ | |||
# Tested with weaviate-client==3.26.0 | |||
# Weaviate instrumentation with opentelemetry-instrumentation-weaviate==0.19.0 |
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.
this can be removed, right? cause we support both
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.
Are there any cassettes left which are not used? I removed both "fetch" cassettes, the rest is needed afais.
Related to #1123