8000 Minimum protobuf version is upgraded to v25.1 · Issue #6718 · onnx/onnx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Minimum protobuf version is upgraded to v25.1 #6718

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.

Sign up for GitHub

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

Jump to bottom
Open
justinchuby opened this issue Feb 20, 2025 · 32 comments
Open

Minimum protobuf version is upgraded to v25.1 #6718

justinchuby opened this issue Feb 20, 2025 · 32 comments
Labels
protobuf topic: build Issues related to ONNX builds and packages

Comments

@justinchuby
Copy link
Contributor
justinchuby commented Feb 20, 2025 •
edited
Loading

I recently found that builds created with pip install . requires protobuf>5 to function. Should we double check the project's requirement?

cc @andife
@justinchuby justinchuby added this to the 1.18 milestone Feb 20, 2025
@andife
Copy link
Member
andife commented Feb 21, 2025

yes we should check this.

Maybe we should also change:

Image

For a step independent of pypi, as a general test for the preview build.

@cyyever
Copy link
Contributor
cyyever commented Feb 21, 2025 •
edited
Loading

grep -i protobuf requirements*.txt lists

requirements-dev.txt:protobuf
requirements-dev_freethreading.txt:protobuf
requirements-lintrunner.txt:types-protobuf==5.29.1.20241207
requirements-min.txt:protobuf==3.20.2; python_version<"3.12"
requirements-min.txt:protobuf==4.25.1; python_version>="3.12"
requirements-release.txt:protobuf==4.25.1
requirements.txt:protobuf>=3.20.2

protobuf in requirements-min.txt is newer than in requirements.txt for python 3.12.

grep -i protobuf pyproject.toml lists

requires = ["setuptools>=64", "protobuf>=3.20.2"]

A simple fix is to change the two lines in requirements-min.txt to protobuf>=3.20.2

@andife
Copy link
Member
andife commented Feb 21, 2025

@justinchuby which python version did you have? for python 3.12 could be the correct behavior
#5743

@cyyever
Copy link
Contributor
cyyever commented Feb 21, 2025 •
edited
Loading

If he used 'pip install .' to install, pip should install a protobuf that satisfies ">=3.20.2" according to pyproject.toml, which would be the latest protobuf. We can try to change the protobuf version in pyproject.toml to a different value locally, then the corresponding protobuf should be installed.

@andife
Copy link
Member
andife commented Feb 21, 2025

well that should work, because for newer python versions e.g. python 3.12 there are no corresponding old wheels at pypi? I'll have to look again at what the reason was for splitting it up and look at the old PR again to see what we might be missing

@andife
Copy link
Member
andife commented Feb 21, 2025

I think we first have to clarify whether there is a problem at all and what the possible bug is?

@justinchuby
Copy link
Contributor Author

https://github.com/onnx/onnx/actions/runs/13444959796/job/37568087432?pr=6717 this is the error log

@cyyever
Copy link
Contributor
cyyever commented Feb 21, 2025 •
edited
Loading

IMO, the CI job installed protobuf 4.25.1 as specified requirements-release.txt but downloaded and built protobuf 5.29.2 from CMake, thus there is major version mismatch. Protobuf may not guarantees backward compatibility in this case.
One way to fix in the CI job is to add

sudo apt-get install libprotobuf-dev

to it? So that CMake will link to the system (which is usually older) protobuf.

Or we update requirements-release.txt to use protobuf 5.29.2?

@andife
Copy link
Member
andife commented Feb 21, 2025 •
edited
Loading

I prefer not to update protobuf in requirements-release.txt, as it is not backwards compatible?

Or, to formulate it differently, I would have liked to have announced this in advance, to have specified it. #6298

Adding "sudo apt-get install libprotobuf-dev" would reflect our description here: https://github.com/onnx/onnx/blob/main/INSTALL.md

Currently we have https://github.com/protocolbuffers/protobuf/releases/download/v21.12/protobuf-cpp-3.21.12.tar.gz in
https://github.com/onnx/onnx/blob/main/workflow_scripts/protobuf/build_protobuf_unix.sh
https://github.com/protocolbuffers/protobuf/releases/tag/v21.12 In my opinion, this should be suitable for the release file?

@justinchuby
Copy link
Contributor Author

I prefer not to update protobuf in requirements-release.txt, as it is not backwards compatible?

I agree. I think we should match the cmake version with that in requirements-release

@cyyever
Copy link
Contributor
cyyever commented Feb 22, 2025 •
edited
Loading

The problem is that the protobuf version from requirements-release.txt should be compatible with the protobuf dev libs in mainstream Linux so that the user doesn't need to build protobuf from source when doing pip install .
For example, ubuntu 24.04 uses 3.21:
https://launchpad.net/ubuntu/+source/protobuf
But other distributions like Fedora may use newer dev libs and it's quite difficult to choose a suitable release version.

@justinchuby
Copy link
Contributor Author

Why is building from source not a good idea?

@cyyever
Copy link
Contributor
cyyever commented Feb 22, 2025

They have to learn concepts about building, which is extra burden.

@justinchuby
Copy link
Contributor Author

I think they can install from pypi right? Those who need to install from source are assumed to have special requirements for the dependencies or the building process.

@andife
Copy link
Member
andife commented Feb 23, 2025 •
edited
Loading

I think we need to decompose the problem now:

a) do we have a stopper for 1.18 ? Do we need to change anything now before there is a release branch? => My current assessment would be no.

b) Should we adjust the auto-update-doc: => I believe yes 1) that it works. What do we have to change? 2) that it is run regularly and we don't miss any issues due to updates from us or others

c) Should we generally work on improving the workflow, including people compiling the project themselves. => Always, but I think we need to carefully consider the implications of the changes.

@justinchuby @xadupre @cyyever @ramkrishna2910

@justinchuby
Copy link
Contributor Author

a) do we have a stopper for 1.18 ? Do we need to change anything now before there is a release branch? => My current assessment would be no.

Looks like the release picks another proto version to compile onnx, not the default one in cmake right? Aka it is compatible with the min protobuf versions we have set. That’s fine with me.

However the onnx package built from source is incompatible with protobuf 4.x. So it seems like this violates the min support version we set.

@andife
Copy link
Member
andife commented Feb 23, 2025
8000

@jcwchen Does #4242 mean our req min has to be changed?

@cyyever
Copy link
Contributor
cyyever commented Feb 24, 2025

The simplest fix is to revert to "protobuf == 3.16.0" in requirements-release.txt.

@justinchuby
Copy link
Contributor Author
justinchuby commented Feb 24, 2025 •
edited
Loading

There are two protobuf versions, the one we use to build the c++ extensions & the onnx python modules, and the python package we install in the release workflow. I think what we need for the c++ version is the newest version that is compatible with the latest python protobuf version (5.x), and the min version we declare to support (3.x). So as new as we can get with max compatiblity.

If whatever protobuf c++ version we use to build the pipeline to support 3.x protobuf python is too old to also work with the 5.x protobuf python, I vote that we update the min support protobuf python version, so that the onnx package we build works with 5.x.

@justinchuby
Copy link
Contributor Author

Also we need to use a version that does not contain any of the vulnerabilities https://security.snyk.io/package/pip/protobuf

@cyyever
Copy link
Contributor
cyyever commented Feb 24, 2025 •
edited
Loading

3.16.0 is actually very old, which was release in 2021. Our current minimum version is acceptable to me. Vulnerabilities are unavoidable, the only way to minimise them is to follow Protobuf's deprecation policy and life the minimum version for each ONNX major release.

@andife
Copy link
Member
andife commented Feb 24, 2025 •
edited
Loading

What do we miss if we make the following changes: #6722 , i.e. define 4.25.1 as min protobuf version? If I understand correctly, we have not been compatible with protobuf 3 for some time now?

@justinchuby
Copy link
Contributor Author

I think #6722 is quite reasonable

@justinchuby
Copy link
Contributor Author

Also, from https://pypi.org/project/protobuf/4.25.1 I realized it is possible to compile agaist a strict set of python abi to support multiple python versions with a single wheel. I wonder if we can do this with onnx?

@justinchuby justinchuby mentioned this issue Feb 24, 2025
Open
@andife andife mentioned this issue Feb 25, 2025
Merged
@andife
Copy link
Member
andife commented Feb 26, 2025

My current tendency is to merge #6722 and #6723 first to get a better overview. I think the changes are safe by now.

I still can't get a complete overview of the changes to #6725?
There are also a few additional errors in other workflows.

@cyyever
Copy link
Contributor
cyyever commented Feb 26, 2025

@andife I agree, it's better to merge safe changes first.

github-merge-queue bot pushed a commit that referenced this issue Feb 26, 2025
@andife
Set version 4.25.1 as min protobuf version (#6722)
a952ab0 
### Description
According to the discussion at #6718
our minimal protobuf is wrong.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

---------

Signed-off-by: Andreas Fehlner <fehlner@arcor.de>
@jcwchen
Copy link
Member
jcwchen commented Feb 28, 2025

@jcwchen Does #4242 mean our req min has to be changed?

To provide some context I know when upgrading onnx protobuf version in the past -- sometimes newer Protobuf version will bring breaking changes and lower version of protobuf/protoc version might not work at all. To make onnx compatible with newer Protobuf, we may need to

  • Upgrade protoc which uses in pipelines
  • Upgrade minimum required version of python protobuf in requirements.txt

However, upgrading minimum required version of python protobuf might be a concern for some users and so please ensure we notify this upgrade in ONNX community in advance. Thank you guys!

@andife
Copy link
Member
andife commented Mar 1, 2025

@jcwchen Thank your for comments.

As I understand it, we weren't exactly clean before, so presumably the min version given for the last release was no longer correct in terms of building and executing. From this perspective, the minimal version would not change.

I also think that we need to communicate clearly which core components we plan to raise and when. See also the discussion here: See also the discussion here: #6298

Image

@cyyever
Copy link
Contributor
cyyever commented Mar 1, 2025

And if we life the minimum protobuf version, the instructions of INSTALL.md also need to be updated.

@justinchuby justinchuby added the topic: build Issues related to ONNX builds and packages label Mar 3, 2025
@andife andife added the protobuf label Mar 5, 2025
@andife andife changed the title Verify protobuf support status Minimum protobuf version is upgraded to v25.1 Mar 5, 2025
@andife andife pinned this issue Mar 5, 2025
@AdrianSosic
Copy link

Hi, just wanted to ask if someone has an estimate how long it will take to resolve this issue. As far as I can see, this is the last blocker for the 1.18 release shipping Python 3.13, which I'm waiting for 🙃 Thanks!

@justinchuby justinchuby removed this from the 1.18 milestone Apr 9, 2025
@andife
Copy link
Member
andife commented Apr 9, 2025

@AdrianSosic You can test the rc1 at https://test.pypi.org/project/onnx/1.18.0rc1/

@andife
Copy link
Member
andife commented May 2, 2025

@AdrianSosic You can test the rc2 at https://test.pypi.org/project/onnx/1.18.0rc2/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
protobuf topic: build Issues related to ONNX builds and packages
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@justinchuby @jcwchen @cyyever @andife @AdrianSosic
0