Skip to content

feat: support proto 3#1078

Merged
samsja merged 4 commits intofeat-rewrite-v2from
feat-support-proto-3
Feb 3, 2023
Merged

feat: support proto 3#1078
samsja merged 4 commits intofeat-rewrite-v2from
feat-support-proto-3

Conversation

@samsja
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants