kylebgorman / pynini

Read-only mirror of Pynini
http://pynini.opengrm.org
Apache License 2.0
120 stars 26 forks source link

Move from C++11 to C++17 incompatible with latest OpenFst (i.e., 1.7.5) #19

Closed PierreAndreNoel closed 4 years ago

PierreAndreNoel commented 4 years ago

Version 2.0.9.post2 changed one of gcc's hard-coded flags from -std=c++11 to -std=c++17. However, the latest OpenFst (i.e., 1.7.5) still uses the hard-coded -std=c++11 flag, which induces incompatible symbols in the *.so.

Example:

FROM python:3.7-buster

# OpenFst

ENV FST_VERSION "1.7.5"
ENV FST_DOWNLOAD_PREFIX "http://www.openfst.org/twiki/pub/FST/FstDownload"

RUN cd /tmp \
    && wget -q ${FST_DOWNLOAD_PREFIX}/openfst-${FST_VERSION}.tar.gz \
    && tar -zxf openfst-${FST_VERSION}.tar.gz \
    && rm openfst-${FST_VERSION}.tar.gz

RUN cd /tmp/openfst-${FST_VERSION} \
    # && mv configure configure.bad \  # Uncomment this and following for workaround
    # && cat configure.bad | sed 's/-std=c++11/-std=c++17/' > configure \
    # && rm configure.bad \
    # && chmod +x configure \
    && ./configure  --enable-grm \
    && make --jobs=4 \
    && make install \
    && rm -rd /tmp/openfst-${FST_VERSION}

RUN ldconfig

# Pynini

ENV PYNINI_VERSION "2.0.9.post2"
ENV PYNINI_DOWNLOAD_PREFIX "http://www.opengrm.org/twiki/pub/GRM/PyniniDownload"

RUN cd /tmp \
    && wget -q ${PYNINI_DOWNLOAD_PREFIX}/pynini-${PYNINI_VERSION}.tar.gz \
    && tar -zxf pynini-${PYNINI_VERSION}.tar.gz \
    && rm pynini-${PYNINI_VERSION}.tar.gz

RUN mkdir /tools \
    && mv /tmp/pynini-${PYNINI_VERSION} /tools/pynini \
    && cd /tools/pynini \
    && python setup.py install

# Import test
RUN python -c "import pynini"

From a folder containing the above Dockerfile, calling docker build . fails on the last line with the error message:

Step 11/11 : RUN python -c "import pynini"
 ---> Running in 369a30cc34d8
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: /usr/local/lib/python3.7/site-packages/pynini-2.0.9.post2-py3.7-linux-x86_64.egg/pynini.cpython-37m-x86_64-linux-gnu.so: undefined symbol: _ZNK3fst8internal14DenseSymbolMap4FindESt17basic_string_viewIcSt11char_traitsIcEE
The command '/bin/sh -c python -c "import pynini"' returned a non-zero code: 1

In short, Pynini expects the symbol fst::internal::DenseSymbolMap::Find(std::basic_string_view<char, std::char_traits<char> >) const but the OpenFst .so instead has fst::internal::DenseSymbolMap::Find(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const.

kylebgorman commented 4 years ago

Thanks for the report.

I can fix the compilation flags used for OpenFst upstream (not Pynini though), but cannot replicate the issue locally. (I didn't try the Docker version.)

On Wed, Jan 8, 2020 at 11:23 AM Pierre-Andre Noel notifications@github.com wrote:

Version 2.0.9.post2 changed https://github.com/kylebgorman/pynini/commit/76178c3e25d6570b425b2a30f188cf835f7a6f23#diff-2eeaed663bd0d25b7e608891384b7298L24-R24 one of gcc's hard-coded flags from -std=c++11 to -std=c++17. However, the latest OpenFst (i.e., 1.7.5) still uses the hard-coded -std=c++11 flag, which induces incompatible symbols in the *.so.

Example:

FROM python:3.7-buster

OpenFst

ENV FST_VERSION "1.7.5"ENV FST_DOWNLOAD_PREFIX "http://www.openfst.org/twiki/pub/FST/FstDownload" RUN cd /tmp \ && wget -q ${FST_DOWNLOAD_PREFIX}/openfst-${FST_VERSION}.tar.gz \ && tar -zxf openfst-${FST_VERSION}.tar.gz \ && rm openfst-${FST_VERSION}.tar.gz RUN cd /tmp/openfst-${FST_VERSION} \

&& mv configure configure.bad \ # Uncomment this and following for workaround

# && cat configure.bad | sed 's/-std=c++11/-std=c++17/' > configure \
# && rm configure.bad \
# && chmod +x configure \
&& ./configure  --enable-grm \
&& make --jobs=4 \
&& make install \
&& rm -rd /tmp/openfst-${FST_VERSION}

RUN ldconfig

Pynini

ENV PYNINI_VERSION "2.0.9.post2"ENV PYNINI_DOWNLOAD_PREFIX "http://www.opengrm.org/twiki/pub/GRM/PyniniDownload" RUN cd /tmp \ && wget -q ${PYNINI_DOWNLOAD_PREFIX}/pynini-${PYNINI_VERSION}.tar.gz \ && tar -zxf pynini-${PYNINI_VERSION}.tar.gz \ && rm pynini-${PYNINI_VERSION}.tar.gz RUN mkdir /tools \ && mv /tmp/pynini-${PYNINI_VERSION} /tools/pynini \ && cd /tools/pynini \ && python setup.py install

Import testRUN python -c "import pynini"

From a folder containing the above Dockerfile, calling docker build . fails on the last line with the error message:

Step 11/11 : RUN python -c "import pynini" ---> Running in 369a30cc34d8 Traceback (most recent call last): File "", line 1, in ImportError: /usr/local/lib/python3.7/site-packages/pynini-2.0.9.post2-py3.7-linux-x86_64.egg/pynini.cpython-37m-x86_64-linux-gnu.so: undefined symbol: _ZNK3fst8internal14DenseSymbolMap4FindESt17basic_string_viewIcSt11char_traitsIcEE The command '/bin/sh -c python -c "import pynini"' returned a non-zero code: 1

In short, Pynini expects the symbol fst::internal::DenseSymbolMap::Find(std::basic_string_view<char, std::char_traits >) const but the OpenFst .so instead has fst::internal::DenseSymbolMap::Find(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&) const.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/19?email_source=notifications&email_token=AABG4OOJOAGJWZHILQTL4WTQ4X4ZVA5CNFSM4KEK5RBKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IE2A5XQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OMRVPHUE2PBEZWJKGLQ4X4ZVANCNFSM4KEK5RBA .

PierreAndreNoel commented 4 years ago

Are you on mac? I believe that OSX's gcc links to clang, hence the different behavior.

kylebgorman commented 4 years ago

Nope, I don't have any easy way to test on Mac, except by writing recipes for Conda-Forge. (That said the current state of things builds for Mac on conda-forge and in fact unless you have an allergy to conda---some people don't care for it, I'm sure---I'd strongly recommend using it to install this.)

On Wed, Jan 8, 2020 at 11:49 AM Pierre-Andre Noel notifications@github.com wrote:

Are you on mac? I believe that OSX's gcc links to clang, hence the different behavior.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/19?email_source=notifications&email_token=AABG4OONVKIOYPYX4WKOJP3Q4X7YRA5CNFSM4KEK5RBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEING2AY#issuecomment-572157187, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OPAPB6KNXXFJG6UXS3Q4X7YRANCNFSM4KEK5RBA .

PierreAndreNoel commented 4 years ago

To clarify, I'm not on mac. My Dockerfile is based on Debian Buster, and I reproduced the problem on Ubuntu Bionic (outside docker).

kylebgorman commented 4 years ago

No worries, I don't doubt you, just can't reproduce myself from Bionic.

On Wed, Jan 8, 2020 at 12:08 PM Pierre-Andre Noel notifications@github.com wrote:

To clarify, I'm not on mac. My Dockerfile is based on Debian Buster, and I reproduced the problem on Ubuntu Bionic (outside docker).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/19?email_source=notifications&email_token=AABG4ONKZU24MWBQ3DHQJV3Q4YB7ZA5CNFSM4KEK5RBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINIYYQ#issuecomment-572165218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OK26LBP6DHVUNFJOCLQ4YB7ZANCNFSM4KEK5RBA .

lumpidu commented 4 years ago

I have run the above Dockerfile with updated release-versions: pynini 2.1.1 & openfst 1.7.7 and I get different errors:

First, I need to add an additional build step so that Cython is found. But even then, I get the same error as the issue author.

Here is my complete Dockerfile:

FROM python:3.7-buster

# OpenFst

ENV FST_VERSION "1.7.7"
ENV FST_DOWNLOAD_PREFIX "http://www.openfst.org/twiki/pub/FST/FstDownload"

RUN cd /tmp \
    && wget -q ${FST_DOWNLOAD_PREFIX}/openfst-${FST_VERSION}.tar.gz \
    && tar -zxf openfst-${FST_VERSION}.tar.gz \
    && rm openfst-${FST_VERSION}.tar.gz

RUN cd /tmp/openfst-${FST_VERSION} \
    # && mv configure configure.bad \  # Uncomment this and following for workaround
    # && cat configure.bad | sed 's/-std=c++11/-std=c++17/' > configure \
    # && rm configure.bad \
    # && chmod +x configure \
    && ./configure  --enable-grm \
    && make --jobs=4 \
    && make install \
    && rm -rd /tmp/openfst-${FST_VERSION}

RUN ldconfig

# Pynini

ENV PYNINI_VERSION "2.1.1"
ENV PYNINI_DOWNLOAD_PREFIX "http://www.opengrm.org/twiki/pub/GRM/PyniniDownload"

RUN cd /tmp \
    && wget -q ${PYNINI_DOWNLOAD_PREFIX}/pynini-${PYNINI_VERSION}.tar.gz \
    && tar -zxf pynini-${PYNINI_VERSION}.tar.gz \
    && rm pynini-${PYNINI_VERSION}.tar.gz

RUN mkdir /tools \
    && mv /tmp/pynini-${PYNINI_VERSION} /tools/pynini \
    && cd /tools/pynini \
    && python3 -m pip install --upgrade cython \
    && python3 setup.py install

# Import test
RUN python -c "import pynini"

it would be really helpful, if this project would add a github actions process, so that one could exactly reproduce the build steps necessary. For me, neither Mac OS X build nor the Docker-based approach work out of the box.

PierreAndreNoel commented 4 years ago

@lumpidu It works for me if you uncomment my fix.

FROM python:3.7-buster

# OpenFst

ENV FST_VERSION "1.7.7"
ENV FST_DOWNLOAD_PREFIX "http://www.openfst.org/twiki/pub/FST/FstDownload"

RUN cd /tmp \
    && wget -q ${FST_DOWNLOAD_PREFIX}/openfst-${FST_VERSION}.tar.gz \
    && tar -zxf openfst-${FST_VERSION}.tar.gz \
    && rm openfst-${FST_VERSION}.tar.gz

RUN cd /tmp/openfst-${FST_VERSION} \
    && mv configure configure.bad \
    && cat configure.bad | sed 's/-std=c++11/-std=c++17/' > configure \
    && rm configure.bad \
    && chmod +x configure \
    && ./configure  --enable-grm \
    && make --jobs=4 \
    && make install \
    && rm -rd /tmp/openfst-${FST_VERSION}

RUN ldconfig

# Pynini

ENV PYNINI_VERSION "2.1.1"
ENV PYNINI_DOWNLOAD_PREFIX "http://www.opengrm.org/twiki/pub/GRM/PyniniDownload"

RUN cd /tmp \
    && wget -q ${PYNINI_DOWNLOAD_PREFIX}/pynini-${PYNINI_VERSION}.tar.gz \
    && tar -zxf pynini-${PYNINI_VERSION}.tar.gz \
    && rm pynini-${PYNINI_VERSION}.tar.gz

RUN mkdir /tools \
    && mv /tmp/pynini-${PYNINI_VERSION} /tools/pynini \
    && cd /tools/pynini \
    && python3 -m pip install --upgrade cython \
    && python3 setup.py install

# Run pynini's tests
RUN cd /tools/pynini && python pynini_test.py -v -f

What we ended up doing is that I made manylinux2014 Python wheels that I pushed to our local PyPI. OpenFst's .so are all bundled in the wheel, so a simple pip/poetry install gets you a working pynini. @kylebgorman please let me know if this would have some interest to you.

kylebgorman commented 4 years ago

Yes, I am not knowledgeable here about how that works but I would love to incorporate it if you can assist.

I also should note these assets are all available from Conda-Forge nowadays: ‘conda install -c conda-forge pynini’ should work out of the box.

On Fri, May 8, 2020 at 9:14 AM Pierre-Andre Noel notifications@github.com wrote:

@lumpidu https://github.com/lumpidu It works for me if you uncomment my fix.

FROM python:3.7-buster

OpenFst

ENV FST_VERSION "1.7.7" ENV FST_DOWNLOAD_PREFIX "http://www.openfst.org/twiki/pub/FST/FstDownload"

RUN cd /tmp \ && wget -q ${FST_DOWNLOAD_PREFIX}/openfst-${FST_VERSION}.tar.gz \ && tar -zxf openfst-${FST_VERSION}.tar.gz \ && rm openfst-${FST_VERSION}.tar.gz

RUN cd /tmp/openfst-${FST_VERSION} \ && mv configure configure.bad \ && cat configure.bad | sed 's/-std=c++11/-std=c++17/' > configure \ && rm configure.bad \ && chmod +x configure \ && ./configure --enable-grm \ && make --jobs=4 \ && make install \ && rm -rd /tmp/openfst-${FST_VERSION}

RUN ldconfig

Pynini

ENV PYNINI_VERSION "2.1.1" ENV PYNINI_DOWNLOAD_PREFIX "http://www.opengrm.org/twiki/pub/GRM/PyniniDownload"

RUN cd /tmp \ && wget -q ${PYNINI_DOWNLOAD_PREFIX}/pynini-${PYNINI_VERSION}.tar.gz \ && tar -zxf pynini-${PYNINI_VERSION}.tar.gz \ && rm pynini-${PYNINI_VERSION}.tar.gz

RUN mkdir /tools \ && mv /tmp/pynini-${PYNINI_VERSION} /tools/pynini \ && cd /tools/pynini \ && python3 -m pip install --upgrade cython \ && python3 setup.py install

Run pynini's tests

RUN cd /tools/pynini && python pynini_test.py -v -f

What we ended up doing is that I made manylinux2014 Python wheels that I pushed to our local PyPI. OpenFst's .so are all bundled in the wheel, so a simple pip/poetry install gets you a working pynini. @kylebgorman https://github.com/kylebgorman please let me know if this would have some interest to you.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/19#issuecomment-625809537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OLA5U4EVN7A27D3CGDRQQASBANCNFSM4KEK5RBA .

PierreAndreNoel commented 4 years ago

Yes, I am not knowledgeable here about how that works but I would love to incorporate it if you can assist.

I've got the OK to share our code for that. I will adapt it a bit and come back to you soon.

I also should note these assets are all available from Conda-Forge nowadays: ‘conda install -c conda-forge pynini’ should work out of the box.

Not everyone can use Conda-Forge. In the status quo, if the policy is "all dependencies must be pip/poetry-installable", then I either can't use pynini, or I must provide my own wheels for it.

kylebgorman commented 4 years ago

Understood re: Conda. It's not something I'm religious about, just the first packaging solution that worked for me when there are non-Python dependencies afoot. We'd welcome additional packaging solutions.

On Mon, May 11, 2020 at 12:35 PM Pierre-Andre Noel notifications@github.com wrote:

Yes, I am not knowledgeable here about how that works but I would love to incorporate it if you can assist.

I've got the OK to share our code for that. I will adapt it a bit and come back to you soon.

I also should note these assets are all available from Conda-Forge nowadays: ‘conda install -c conda-forge pynini’ should work out of the box.

Not everyone can use Conda-Forge. In the status quo, if the policy is "all dependencies must be pip/poetry-installable", then I either can't use pynini, or I must provide my own wheels for it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/19#issuecomment-626813218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OLRYWX5PWQDFASIJULRRASLHANCNFSM4KEK5RBA .