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

feat: support proto 3 #1078

Merged
merged 4 commits into from Feb 3, 2023
Merged

feat: support proto 3 #1078

merged 4 commits into from Feb 3, 2023

Conversation

samsja
Copy link
Member

@samsja samsja commented Feb 3, 2023

Context

the goal of this pr is to support both proto 3 and proto 4. Atm we only support proto 3.

details.

  • First, we relax the proto requirement. protobuf>= 3.19.0 instead of protobuf>=4.21

protoc 3 and 4 does not generate the same python code. Therefore if we use proto 3 to generate the docarray_pb2.py it will break when using proto 4 and vice versa.

The solution to this problem is to generate the docarray_pb2.py from the proto two times. One with protoc3 and one with protoc4. Then the trick is to check which version of protobuf is installed during import of the proto file. And we serve the correct version

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>
Signed-off-by: samsja <sami.jaghouar@hotmail.fr>
Signed-off-by: samsja <sami.jaghouar@hotmail.fr>
Signed-off-by: samsja <sami.jaghouar@hotmail.fr>
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

📝 Docs are deployed on https://ft-feat-support-proto-3--jina-docs.netlify.app 🎉

Comment on lines +3 to +20
if __pb__version__.startswith('4'):
from docarray.proto.pb.docarray_pb2 import (
DocumentArrayProto,
DocumentArrayStackedProto,
DocumentProto,
NdArrayProto,
NodeProto,
UnionArrayProto,
)
else:
from docarray.proto.pb2.docarray_pb2 import (
DocumentArrayProto,
DocumentArrayStackedProto,
DocumentProto,
NdArrayProto,
NodeProto,
UnionArrayProto,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't these two exactly the same? or am I blind? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh i see the difference now, nevermind

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR should be named "spot the difference"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nerd version of "where is waldo?"

@samsja samsja merged commit ebe0ede into feat-rewrite-v2 Feb 3, 2023
@samsja samsja deleted the feat-support-proto-3 branch February 3, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants