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

optimizer_test.py failure on litprotobuf ERROR #38

Open
jiafatom opened this issue Apr 9, 2021 · 13 comments
Open

optimizer_test.py failure on litprotobuf ERROR #38

jiafatom opened this issue Apr 9, 2021 · 13 comments

Comments

@jiafatom
Copy link

jiafatom commented Apr 9, 2021

I installed onnx/optimizer from source as mentioned.
Then I run python onnxoptimizer/test/optimizer_test.py, I got the failure below:

[libprotobuf ERROR google/protobuf/descriptor_database.cc:644] File already exists in database: onnx/onnx-ml.proto
[libprotobuf FATAL google/protobuf/descriptor.cc:1371] CHECK failed: GeneratedDatabase()->Add(encoded_file_descriptor, size): 
terminate called after throwing an instance of 'google::protobuf::FatalException'
  what():  CHECK failed: GeneratedDatabase()->Add(encoded_file_descriptor, size): 
Aborted (core dumped)

Any suggestions? Thanks!

@yan12125
Copy link

The underlying issue appears the same as microsoft/onnxruntime#5035. Using protobuf-lite is a working workaround for me:

CMAKE_ARGS="-DONNX_USE_LITE_PROTO=ON" python setup.py build

Other related discussions: onnx/onnx#3030, protocolbuffers/protobuf#1941

@BowenBao
Copy link

Thanks for the suggestion @yan12125, that helped!

I'm now getting the below failure

onnxoptimizer/examples/onnx_optimizer_exec.cpp:15:24: error: ‘class onnx::ModelProto’ has no member named ‘ParseFromIstream’; did you mean ‘ParseFromString’?
   bool success = model.ParseFromIstream(&ifs);
                        ^~~~~~~~~~~~~~~~
                        ParseFromString
onnxoptimizer/examples/onnx_optimizer_exec.cpp:25:23: error: ‘const class onnx::ModelProto’ has no member named ‘SerializePartialToOstream’; did you mean ‘SerializePartialToString’?
   success = new_model.SerializePartialToOstream(&ofs);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~
                       SerializePartialToString

Looks like a protobuf version issue. I can proceed to build the project and install the whl package if I remove the code associated with ParseFromIstream and SerializePartialToOstream in this file.
Perhaps the example script should be modified to allow building under a wider range of protobuf version?

@yan12125
Copy link

Hmm, ParseFromIstream and SerializePartialToOstream are available from full protobuf but not protobuf-lite before 3.9.0 [1].

Perhaps the example script should be modified to allow building under a wider range of protobuf version?

I think that is a good way. Let's see what @daquexian thinks.

[1] protocolbuffers/protobuf@1d4e959

@daquexian
Copy link
Member

daquexian commented Apr 18, 2021

Sorry for the late reply.

@yan12125 @jiafatom @BowenBao Could you please try linking to protobuf static lib? I found that if onnx and onnxoptimizer are both built from source and link to protobuf shared lib, the "File already exists in database: onnx/onnx-ml.proto" error will happen. If protobuf is statically linked instead (i.e. linking libprotobuf.a), the error will not happen.

@yan12125
Copy link

Thanks a lot for the hint. I managed to build onnxoptimzer with a static protobuf library and it indeed works. I got some issues during the build, so I'd like to share my steps in hope of helping others:

  1. First uninstall the system protobuf. Seems libprotobuf.so interferes with libprotobuf.a
  2. Download and extract protobuf sources. I use https://github.com/protocolbuffers/protobuf/releases/download/v3.15.8/protobuf-cpp-3.15.8.tar.gz
  3. Configure and build protobuf with CXXFLAGS="-fPIC" ./configure --disable-shared; make; make install DESTDIR=$HOME/protobuf-install
  4. Build onnxoptimizer with PATH="$PATH:$HOME/protobuf-install/usr/local/bin" python setup.py build

I feel the process more complex than using protobuf-lite provided by Linux distributions. Also, I'm maintaining a unofficial python-onnxoptimizer package for Arch Linux, and packaging guidelines there recommend against using static libraries for concerns around security and package sizes - so I use protobuf-lite for that package.

@daquexian
Copy link
Member

daquexian commented Apr 26, 2021

@yan12125 wow! I had been an arch linux user until one year ago 😂 (I'm now using ChromeOS/Windows and most of my daily work is to ssh to server) Thanks for your great contribution to archlinuxcn and aur!

But I'm afraid it is a "trick" to use libprotobuf-lite. The reason that it solves the problem is it makes onnx optimizer link to libprotobuf-lite.so while onnx links to another library, libprotobuf.so. Though it works, it relies on the "implementation" of how onnx is packaged, which is fragile.

@yan12125
Copy link

Thanks for your kind words! I'm glad that my efforts are also helpful for others :)

Yep, using protobuf-lite is a trick and it is indeed a little fragile. I share my findings in hope of helping others. Building onnxoptimizer against static protobuf does prevent the conflict between onnx-ml.proto in onnxoptimizer and onnx. However, two copies of protobuf in one process may cause issues if those protobuf copies are built from different versions. I believe the long term solution wil be exposing onnx-ml.proto via shared libraries as suggested by a protobuf contributor (protocolbuffers/protobuf#1941 (comment)), so that all libraries can share the same version of protobuf and onnx-ml.proto. Some patches are necessary for ONNX to build working shared libraries. I will do cleanups and submit a pull request when I find time.

yan12125 pushed a commit to EMCLab-Sinica/Stateful-CNN that referenced this issue Oct 10, 2021
Looks like multiple onnx-ml.proto copies are allowed in protobuf-lite.
See also [1] for the protobuf issue.

[1] onnx/optimizer#38
yan12125 pushed a commit to EMCLab-Sinica/Stateful-CNN that referenced this issue Oct 12, 2021
Looks like multiple onnx-ml.proto copies are allowed in protobuf-lite.
See also [1] for the protobuf issue.

[1] onnx/optimizer#38
yan12125 pushed a commit to EMCLab-Sinica/Stateful-CNN that referenced this issue Oct 24, 2021
* Add an external MAML implementation as a submodule
* Load omniglot
* Model rewriting in transform.py
  - Change inputs for KWS to eliminate unsupported operators from the
    final ONNX model.
  - Save models at each step of rewriting for debugging
  - Inference dynamic shapes to get rid of a complex graph from forward
    steps like `x = x.view(x.size(0), -1)`
  - Constant folding for Squeeze and Reshape nodes with known new shape
    and constant input
  - Reduce global variables so that the latest model is always used
    after model rewriting
* Other changes in transform.py
  - Move more ONNX helpers to utils.py
  - Make transformation of input samples more robust in terms of input
    shape
  - Don't use a default batch size; make that argument required.
* Implement BatchNormalization
* Use libc_nano instead of libc as the model omniglot-maml is too large

[1] onnx/optimizer#38
[2] microsoft/onnxruntime#5577
@lucasjinreal
Copy link

@yan12125 @daquexian got into same issue in 202222..

Why it still an issue, uninstall system protobuf seems broken everything I rely on, any better suggestion here? optimizer become standalone but shared a onnx-ml.proto with onnx, looks like not a good way, why not just using onnx as included lib remove onnx-ml.proto from this repo?

@yan12125
Copy link

Why it still an issue, uninstall system protobuf seems broken everything I rely on

You don't need to uninstall system protobuf if your system protobuf is 3.9.0 or newer. Using the command at #38 (comment) to build onnxoptimizer should be enough.

why not just using onnx as included lib remove onnx-ml.proto from this repo?

That is exactly what I wanted to do, but that requires non-trivial changes discussed in onnx/onnx#3030 as you already know. I'm not a developer of onnx or onnxoptimizer and I'm OK with the status quo, so I don't put more efforts into this issue.

@lucasjinreal
Copy link

@yan12125 I believe this error will happen then:

#38 (comment)

@yan12125
Copy link

I saw you mentioned M1 Mac in another issue (onnx/onnx#4013). Do you use a package manager like Homebrew or MacPorts? Both provides protobuf 3.19.x [1,2] and CMAKE_ARGS="-DONNX_USE_LITE_PROTO=ON" should be enough. If there are still failures, please share complete logs.

[1] https://formulae.brew.sh/formula/protobuf
[2] https://ports.macports.org/port/protobuf3-cpp/

yan12125 pushed a commit to EMCLab-Sinica/Stateful-CNN that referenced this issue Apr 5, 2022
* Add an external MAML implementation as a submodule
* Load omniglot
* Model rewriting in transform.py
  - Change inputs for KWS to eliminate unsupported operators from the
    final ONNX model.
  - Save models at each step of rewriting for debugging
  - Inference dynamic shapes to get rid of a complex graph from forward
    steps like `x = x.view(x.size(0), -1)`
  - Constant folding for Squeeze and Reshape nodes with known new shape
    and constant input
  - Reduce global variables so that the latest model is always used
    after model rewriting
* Other changes in transform.py
  - Move more ONNX helpers to utils.py
  - Make transformation of input samples more robust in terms of input
    shape
  - Don't use a default batch size; make that argument required.
* Implement BatchNormalization
* Use libc_nano instead of libc as the model omniglot-maml is too large

[1] onnx/optimizer#38
[2] microsoft/onnxruntime#5577
@lucasjinreal
Copy link

I think is not a protobuf lib issue. It's a prootbuf schema conflict issue

@yurivict
Copy link

This is still a problem now, in 2024: pyg-team/pytorch_geometric#9220

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

No branches or pull requests

6 participants