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

Allow support for protobuf5 #3931

Closed
wants to merge 7 commits into from

Conversation

Sovietaced
Copy link

@Sovietaced Sovietaced commented May 24, 2024

Description

I was unable to use a more recent version of grpc-io due to this library's upper restriction on the version of protobuf. Protobuf 5 was released in March and newer versions of grpc rely on it so it would be nice if this library were more flexible.

How Has This Been Tested?

I added environments to fox in order to ensure the library builds in protobuf 5.

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:

  • No.

Checklist:

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

@Sovietaced Sovietaced marked this pull request as ready for review May 24, 2024 23:01
@Sovietaced Sovietaced requested a review from a team May 24, 2024 23:01
tox.ini Outdated Show resolved Hide resolved
Copy link

@torarvid torarvid left a comment

Choose a reason for hiding this comment

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

I'm just a guy drive-by-reviewing (not familiar with the repo — just waiting for v5 support)

tox.ini Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Sovietaced
Copy link
Author

Looks like this is currently blocked on a bad googleapis-common-protos wheel: googleapis/python-api-common-protos@28fc17a

It should support protobuf > 5 but doesn't...

Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
@aabmass
Copy link
Member

aabmass commented May 29, 2024

I was unable to use a more recent version of grpc-io due to this library's upper restriction on the version of protobuf. Protobuf 5 was released in March and newer versions of grpc rely on it so it would be nice if this library were more flexible.

I'm a little dubious this will work correctly. Protobuf versions make no guarantees that the generated code will work with even different minor versions of the protobuf library: protocolbuffers/protobuf#11123.

We had a bit of a debacle when protobuf 4 came out with a partition in the community on which version to support. IIRC we found some magic version protoc compiler that works with both versions. I guess that is still the case here if the tests are passing, but do you have any insight on how the generated code is working across protobuf 3, 4, and 5?

@Sovietaced
Copy link
Author

Sovietaced commented May 29, 2024

I guess that is still the case here if the tests are passing, but do you have any insight on how the generated code is working across protobuf 3, 4, and 5?

Admittedly I have no idea yet. I'm just a user of a library that has this in a long list of transitive dependency that can no longer upgrade to grpc-io > 1.62.0. Unfortunately the tests are unable to even pass yet because google's own common protos library release is botched. Once I can get CI running I think I'll cross that bridge and look deeper into the generated code. I appreciate the context around the changes though.

@aabmass
Copy link
Member

aabmass commented May 29, 2024

@Sovietaced gotcha I appreciate the PR. I reached out internally regarding googleapis/python-api-common-protos#221 and there is apparently more work needed to support protobuf 5 and should have a fix in the next couple of weeks.

@aabmass
Copy link
Member

aabmass commented Jun 3, 2024

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

@Sovietaced
Copy link
Author

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

Thanks for the ping. I wasn't subscribed to the releases. Will take a look today or tomorrow.

@Sovietaced
Copy link
Author

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

Updated. Ran tox locally and it seems to work. Just need approval for CI to run.

@aabmass
Copy link
Member

aabmass commented Jun 6, 2024

Related: #3932 (comment)

I'm going to bring this up in the SIG call but I'm nervous to move forward with this change even with CI passing without first re-generating the code and only supporting a single major version. From the protobuf website

Protobuf cross-version usages outside the guarantees are error-prone and not supported. Version skews can lead to flakes and undefined behaviors that are hard to diagnose, even if it can often seem to work as long as nothing has changed in a source-incompatible way. For Protobuf, the proliferation of tools and services that rely on using unsupported Protobuf language bindings prevents the protobuf team from updating the protobuf implementation in response to bug reports or security vulnerabilities.]

@aabmass aabmass mentioned this pull request Jun 6, 2024
2 tasks
@aabmass aabmass linked an issue Jun 6, 2024 that may be closed by this pull request
2 tasks
@Sovietaced
Copy link
Author

I'm going to bring this up in the SIG call but I'm nervous to move forward with this change even with CI passing without first re-generating the code and only supporting a single major version. From the protobuf website

Makes sense. That does seem like a tricky problem to manage. And seems like the only way to safely handle that is to distribute different versions of these packages, one per protobuf major, which does not seem fun :/

@aabmass
Copy link
Member

aabmass commented Jun 6, 2024

We didn't really come to a conclusion. I opened #3958 for discussion

@aabmass
Copy link
Member

aabmass commented Sep 27, 2024

@Sovietaced thanks again for your patience, do you still have time to work on this? I think we have reached a decision to move forward with only supporting protobuf 5+ and get this into the next release.

@Sovietaced
Copy link
Author

@Sovietaced thanks again for your patience, do you still have time to work on this? I think we have reached a decision to move forward with only supporting protobuf 5+ and get this into the next release.

I can rework the build to support that. If it requires more than that I probably wouldn't be a good candidate since I'm a Python hobbyist. I can at least and fix up this pull request.

@Sovietaced
Copy link
Author

I can rework the build to support that. If it requires more than that I probably wouldn't be a good candidate since I'm a Python hobbyist. I can at least and fix up this pull request.

I took a look a things have changed a bit. I'm not sure I'll have to time to get it fully working so feel free to have someone else take a look.

@aabmass
Copy link
Member

aabmass commented Sep 30, 2024

No worries @Sovietaced appreciate your work on this so far. This PR helped move the discussion forward. Since we are scoping down, I will probably open a separate PR

@emdneto
Copy link
Member

emdneto commented Oct 14, 2024

Closing as support for protobuf5 was merged 🚀 #4206

@emdneto emdneto closed this Oct 14, 2024
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.

Add support for Protobuf 5
5 participants