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

[ONNX 1.10] Fix for compilation error on older compilers #3683

Merged
merged 5 commits into from Sep 3, 2021

Conversation

postrational
Copy link
Contributor

@postrational postrational commented Aug 26, 2021

Description

ONNX 1.10 fails to compile when using older versions of gcc, for example on CentOS, with the following error:

In file included from /onnx/onnx/shape_inference/implementation.cc:5:0:
/onnx/onnx/shape_inference/implementation.h: In constructor 'onnx::shape_inference::DataPropagationContextImpl::DataPropagationContextImpl(onnx::NodeProto&, const std::unordered_map<std::basic_string<char>, onnx::TypeProto*>&, const std::unordered_map<std::basic_string<char>, const onnx::TensorProto*>&, std::unordered_map<std::basic_string<char>, onnx::TensorShapeProto>&)':
/onnx/onnx/shape_inference/implementation.h:276:47: error: invalid initialization of non-const reference of type 'std::unordered_map<std::basic_string<char>, onnx::TensorShapeProto>&' from an rvalue of type '<brace-enclosed initializer list>'
       : generatedShapeData_{generatedShapeData} {
                                               ^
gmake[2]: *** [CMakeFiles/onnx.dir/onnx/shape_inference/implementation.cc.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[1]: *** [CMakeFiles/onnx.dir/all] Error 2
gmake: *** [all] Error 2  

Steps to reproduce

Build the following Dockerfile to observe the issue:

FROM centos:7.6.1810

RUN yum -y update
RUN yum -y install gcc gcc-c++ make openssl-devel
RUN yum -y install python3 python3-devel
RUN yum -y install git wget

WORKDIR /root/

# Build and install Cmake
RUN wget https://cmake.org/files/v3.21/cmake-3.21.1.tar.gz
RUN tar zxvf cmake-3.21.1.tar.gz
RUN cd cmake-3.21.1 && ./bootstrap --prefix=/usr/local && make -j$(nproc) && make install

# Build and install protobuf
RUN git clone https://github.com/protocolbuffers/protobuf.git
RUN mkdir /root/protobuf/build
RUN cd protobuf/build && cmake ../cmake \
    -Dprotobuf_BUILD_SHARED_LIBS=OFF \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DCMAKE_INSTALL_SYSCONFDIR=/etc \
    -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
    -Dprotobuf_BUILD_TESTS=OFF \
    -DCMAKE_BUILD_TYPE=Release
RUN cd protobuf/build && make -j$(nproc) && make install

# Create and activate virtualenv
RUN python3 -m pip install virtualenv
RUN python3 -m virtualenv /root/venv
ENV PATH=/root/venv/bin:$PATH

RUN git clone https://github.com/onnx/onnx.git 
WORKDIR /root/onnx
RUN git checkout v1.10.1
RUN git submodule update --init --recursive
RUN pip install -U pip
RUN pip install -r requirements-release.txt

RUN python setup.py install

Motivation and Context

This small change fixes the issue.

It may be a reason to make another point-release of ONNX 1.10

@postrational postrational added this to the 1.10 milestone Aug 26, 2021
@postrational postrational requested a review from jcwchen Aug 26, 2021
@postrational postrational requested a review from a team as a code owner Aug 26, 2021
Signed-off-by: Michal Karzynski <michal.karzynski@intel.com>
Copy link
Member

@jcwchen jcwchen left a comment

LGTM. It's good to make it compatible with older C++. Thanks for catching this.

@askhade
Copy link
Contributor

askhade commented Aug 30, 2021

This seems to have been identified as a bug in C++11 and was later corrected.
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1288 (Search for 1095. List-initialization of references) and gcc 4.9 onwards the compiler supports it too.

We can make this change to support older compilers but given gcc 4.8 is pretty old wondering whether we should request users to update gcc .

How urgent is this change? Can we discuss this in this week's arch-infra op-sig meeting?

@gramalingam
Copy link
Contributor

gramalingam commented Sep 1, 2021

The change itself makes sense to me, since the fix/benefit comes at no cost/downside. We can decouple the question of whether we need another release from this PR, I think.

@askhade askhade enabled auto-merge (squash) Sep 3, 2021
@askhade askhade merged commit 780188f into onnx:master Sep 3, 2021
46 of 47 checks passed
postrational added a commit that referenced this issue Oct 12, 2021
Signed-off-by: Michal Karzynski <michal.karzynski@intel.com>

Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com>
Co-authored-by: Ashwini Khade <askhade@microsoft.com>
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.

None yet

4 participants