Skip to content
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

Fast protobuf #67

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Fast protobuf #67

merged 2 commits into from
Sep 16, 2022

Conversation

generall
Copy link
Member

@generall generall commented Sep 1, 2022

Replace betterproto with default google grpcio, as it is 20x faster serialization

@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit 7871de6
🔍 Latest deploy log https://app.netlify.com/sites/poetic-froyo-8baba7/deploys/6324ca67209c690008be945b
😎 Deploy Preview https://deploy-preview-67--poetic-froyo-8baba7.netlify.app/qdrant_client.grpc
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@agourlay
Copy link
Member

agourlay commented Sep 5, 2022

Do you remember what was the rationale for using betterproto in first place?
Just want to make sure we have enough context captured here.

@generall
Copy link
Member Author

generall commented Sep 5, 2022

Do you remember what was the rationale for using betterproto in first place?

The reason was, that betterproto generated structures looks like this:

@dataclass(eq=False, repr=False)
class UpdateCollection(betterproto.Message):
    collection_name: str = betterproto.string_field(1)
    optimizers_config: Optional["OptimizersConfigDiff"] = betterproto.message_field(
        2, optional=True, group="_optimizers_config"
    )
    timeout: Optional[int] = betterproto.uint64_field(
        3, optional=True, group="_timeout"
    )

not like this

UpdateCollection = _reflection.GeneratedProtocolMessageType('UpdateCollection', (_message.Message,), {
  'DESCRIPTOR' : _UPDATECOLLECTION,
  '__module__' : 'collections_pb2'
  # @@protoc_insertion_point(class_scope:qdrant.UpdateCollection)
  })
_sym_db.RegisterMessage(UpdateCollection)

@agourlay
Copy link
Member

agourlay commented Sep 7, 2022

How much impact is there for the users of the client? Do they need to change some call sites or is it completely transparent?

@generall
Copy link
Member Author

generall commented Sep 7, 2022

How much impact is there for the users of the client? Do they need to change some call sites or is it completely transparent?

I would say, it is 95% same, buy there are corner cases

@generall generall force-pushed the fast-protobuf branch 2 times, most recently from 9a758e5 to fbaff7d Compare September 16, 2022 17:17
use new grpc client in qdrant methods

fix grpc parallel issue

remove betterproto

remove betterproto

remove betterproto type annotation

extend test coverage

upd protobuf to master
@generall generall changed the base branch from master to v0.10.0 September 16, 2022 19:11
@generall generall merged commit 0970f02 into v0.10.0 Sep 16, 2022
generall added a commit that referenced this pull request Sep 19, 2022
* generate gRPC client

* generate REST client

* draft

* wip

* fix conversion test

* old api compatible

* clean deprecated

* batch search + reco tests

* multi-vector test

* full-text filtering

* Fast protobuf (#67)

* generate grpc client

use new grpc client in qdrant methods

fix grpc parallel issue

remove betterproto

remove betterproto

remove betterproto type annotation

extend test coverage

upd protobuf to master

* rm eventloop

* upd version

* shortcut for models

* v0.10.1 compatibility hotfix: make deprecated ram_data_size optional

Co-authored-by: Andrey Vasnetsov <[email protected]>
@generall generall deleted the fast-protobuf branch May 3, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants