-
Notifications
You must be signed in to change notification settings - Fork 88
Refactor to separate sync and async connections #1594
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
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
Great to see you again! Thanks for the contribution. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1594 +/- ##
==========================================
- Coverage 88.39% 87.82% -0.57%
==========================================
Files 187 219 +32
Lines 16015 17492 +1477
==========================================
+ Hits 14156 15362 +1206
- Misses 1859 2130 +271 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
…iate/weaviate-python-client into poc-separate-sync-and-async
…n-client into poc-separate-sync-and-async
…n-client into poc-separate-sync-and-async
…n-client into poc-separate-sync-and-async
…ient into poc-separate-sync-and-async
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Infrastructure as Code | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
SAST | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Vulnerabilities | ![]() ![]() ![]() ![]() |
View in Orca |
…ient into poc-separate-sync-and-async
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Infrastructure as Code | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
SAST | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Vulnerabilities | ![]() ![]() ![]() ![]() |
View in Orca |
🛡️ The following SAST misconfigurations have been detected
NAME | FILE | ||
---|---|---|---|
![]() |
Flask app configured to run with host 0.0.0.0, exposing it publicly | ...sts/gunicorn/wsgi.py | View in code |
This change broke unit tests for me, because suddenly the |
Hi @eavanvalkenburg, thanks for raising! I'll look into how we can work around that and release a fix |
was able to work around it, by doing this: # previous code:
async_mock = AsyncMock(spec=WeaviateAsyncClient)
async_mock.collections = AsyncMock(spec=_CollectionsAsync)
# new additions:
async_mock.collections.create = AsyncMock()
async_mock.collections.delete = AsyncMock()
async_mock.collections.exists = AsyncMock() |
This PR constitutes a complete overhaul of the codebase structure with respect to the underlying connections of the
WeaviateClient
andWeaviateAsyncClient
classes and how they are provided to them. Currently, theWeaviateClient
works by using the sameConnectionV4
(which isasync
) object under-the-hood that theWeaviateAsyncClient
uses only with the calls scheduled in a side-car event loop thread.With this PR, this is has been refactored so that the connection can be dependency injected into the business logic layer of the client from the top-down so that all namespaces in
WeaviateClient
now explicitly depend onConnectionSync
, which itself depends onhttpx.Client
andgrpc.Channel
, withConnectionV4
renamed toConnectionAsync
that still depends onhttpx.AsyncClient
andgrpc.aio.Channel
.This is achieved using the command pattern whereby all business logic of the client is encapsulated into Executor classes, e.g.
_DataExecutor
, which expose methods that exactly match those of the public API plus an additional argument:connection: ConnectionSync | ConnectionAsync
. Then, at runtime, the connection is injected into the executor call by the decorator@weaviate.connect.executor.wrap("sync" | "async")
so that we can have a single implementation of the inner business logic in the executor that is agnostic to exactly which connection is passed to it. The executor methods work with the union of connections by utilising the logic inweaviate/connect/executor.py
, which is effectively an ad hoc implementation of callbacks.This PR introduces the fundamental structure of the overall pattern that the client should use moving forward. However, there is a large amount of boilerplate associated with the stitching together of the sync/async clients with the executors. This should be automated in the future through custom-built auto-generation tools that translate the API surfaces of the executors into the
async_.pyi
andsync.pyi
stub files of each namespace. Until then effort should also be spent developing a custom flake8 linter to validate that the namespaceasync_.pyi
andsync.pyi
files are identical.