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

Compatible workaround to support both protobuf <=3.20 and >=4.21 #2954

Closed
wants to merge 1 commit into from

Conversation

gen-xu
Copy link
Contributor

@gen-xu gen-xu commented Sep 29, 2022

Description

This is a work around for #2880
By checking the api implementation before define anything in the generated code, it is able to have both versions of generated protobuf and so it allows the opentelemetry-proto to work with both protobuf<=3.20 and protobuf>=4.21

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested with both protobuf~=3.19.4 and protobuf==4.21.6 by importing any generated modules and it should not throw any error

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR: Update dependency fo open-telemetry/opentelemetry-python#2954 opentelemetry-python-contrib#1363

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@gen-xu gen-xu requested a review from a team September 29, 2022 19:28
@alaeddine-13
Copy link

This PR would be helpful for us at Jina AI, since open telemetry version cap in protobuf restricts our protobuf version.
(we also did a similar hack to support both <=3.20 and >=4.21)

@aabmass
Copy link
Member

aabmass commented Dec 1, 2022

@gen-xu thank you for the PR. Does the fix mentioned in #2880 (comment) (along with loosening the dependencies) solve this issue as well? It seems like a good middle ground until we are ready to drop Protobuf 3.

@gen-xu
Copy link
Contributor Author

gen-xu commented Dec 1, 2022

@gen-xu thank you for the PR. Does the fix mentioned in #2880 (comment) (along with loosening the dependencies) solve this issue as well? It seems like a good middle ground until we are ready to drop Protobuf 3.

Yes this should support both protobuf 3 and 4, but afaik there are some other protos (jaeger etc.) in the contrib repo might also need to get regenerated?

@aabmass
Copy link
Member

aabmass commented Dec 2, 2022

@gen-xu I saw your comment here #2880 (comment), where did you see that "old protobuf library version won't be able to use newer generated protobuf code"? Which version is the cutoff? I've opened protocolbuffers/protobuf#11123 to get some clarity on this as I couldn't find it documented.

Yes this should support both protobuf 3 and 4, but afaik there are some other protos (jaeger etc.) in the contrib repo might also need to get regenerated?

👍

@gen-xu
Copy link
Contributor Author

gen-xu commented Dec 2, 2022

@gen-xu I saw your comment here #2880 (comment), where did you see that "old protobuf library version won't be able to use newer generated protobuf code"? Which version is the cutoff? I've opened protocolbuffers/protobuf#11123 to get some clarity on this as I couldn't find it documented.

Basically this line in the generated code, the builder is introduced in 3.20, and code generated after that version uses the builder, and I believe the upb implementation is officially introduced as replacement for cpp for python >=3.21 (a.k.a protobuf 4.21)

from google.protobuf.internal import builder as _builder

@aabmass
Copy link
Member

aabmass commented Dec 2, 2022

@gen-xu right I know we need newer protoc to work with upb. I'm a bit confused because I generated the protobufs with latest grpcio-tools (which provides libprotoc 3.21.6 protoc generator) and all of our tests are passing.

Is there a case you've seen where the latest version of protoc does not work with protobuf~=3.19?

edit see protocolbuffers/protobuf#11123 (comment), I think latest version should work OK for now.

@gen-xu
Copy link
Contributor Author

gen-xu commented Dec 2, 2022

@gen-xu right I know we need newer protoc to work with upb. I'm a bit confused because I generated the protobufs with latest grpcio-tools (which provides libprotoc 3.21.6 protoc generator) and all of our tests are passing.

Is there a case you've seen where the latest version of protoc does not work with protobuf~=3.19?

if you use protoc 3.21.6, it generates code that uses the builder, which doesn't really exsit in protobuf 3.19.x.
you can try pip install protobuf==3.19.6 when the newer generated code it will faill on this line

from google.protobuf.internal import builder as _builder

I believe the reason why it passes all tests is that you are using protobuf~=3.19 where pip actually will install protobuf==3.20.x for you (<4.x), and the builder is introduced in protobuf>=3.20.

@aabmass
Copy link
Member

aabmass commented Dec 2, 2022

I believe the reason why it passes all tests is that you are using protobuf~=3.19 where pip actually will install protobuf==3.20.x for you (<4.x), and the builder is introduced in protobuf>=3.20.

You're right, I update my PR to actually test with ~=3.19.0 which resolves to 3.19.6. It works now with protos generated with grpcio-tools==1.48.1 (which provides protoc 3.19.4).

Thanks for talking this through. Does #3070 look good to you now as a solution for now?

@gen-xu
Copy link
Contributor Author

gen-xu commented Dec 3, 2022

I believe the reason why it passes all tests is that you are using protobuf~=3.19 where pip actually will install protobuf==3.20.x for you (<4.x), and the builder is introduced in protobuf>=3.20.

You're right, I update my PR to actually test with ~=3.19.0 which resolves to 3.19.6. It works now with protos generated with grpcio-tools==1.48.1 (which provides protoc 3.19.4).

Thanks for talking this through. Does #3070 look good to you now as a solution for now?

Yup that looks good to me! I am closing this PR since #3070 works.

@gen-xu gen-xu closed this Dec 3, 2022
@gen-xu gen-xu deleted the fix/2880 branch December 19, 2022 12:32
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.

3 participants