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

Fix compilation with gcc < 5 #2956

Merged
merged 2 commits into from
Apr 19, 2021
Merged

Conversation

mvoelkle-cern
Copy link
Contributor

@mvoelkle-cern mvoelkle-cern commented Apr 16, 2021

Description

When the user defines _GLIBCXX_USE_CXX11_ABI=0 to force the pre-c++11 ABI, numpy.h assumes that is_trivially_copyable is available.
It is not necessarily the case. This patch uses clang's feature detection instead.

Suggested changelog entry:

Fixed compilation with GCC < 5 when the user defines ``_GLIBCXX_USE_CXX11_ABI``.

When the user defines _GLIBCXX_USE_CXX11_ABI=0 to force the pre-c++11 ABI, numpy.h assumes that is_trivially_copyable is available.
It is not necessarily the case. This patch uses clang's feature detection instead.
@EricCousineau-TRI
Copy link
Collaborator

Howdy @mvoelkle-cern! Do you have a theory as to why our present CI coverage doesn't already check this case?
From a naive read at present master, it seems like we should have?

# Testing on CentOS (manylinux uses a centos base, and this is an easy way
# to get GCC 4.8, which is the manylinux1 compiler).
centos:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
centos:
- 7 # GCC 4.8
- 8

Also, I believe these CI errors are caused by your PR - can you fix those?

The workaround is for certain libstdc++ versions, so the test should target these particular versions.
@mvoelkle-cern
Copy link
Contributor Author

Hi, sorry my fix didn't compile outside of clang, I had wrongly assumed that preprocessor boolean expressions would short-circuit. Here is an other approach that targets libstdc++ specifically.

@mvoelkle-cern
Copy link
Contributor Author

Howdy @mvoelkle-cern! Do you have a theory as to why our present CI coverage doesn't already check this case?
From a naive read at present master, it seems like we should have?

# Testing on CentOS (manylinux uses a centos base, and this is an easy way
# to get GCC 4.8, which is the manylinux1 compiler).
centos:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
centos:
- 7 # GCC 4.8
- 8

The issue is when using 4.8 AND the user defines _GLIBCXX_USE_CXX11_ABI=0. The CI coverage doesn't define it.

@henryiii henryiii added the bug label Apr 19, 2021
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

I can't state w/ 100% confidence that I can verify, but will trust testing on your side as well as our CI (and of course, Henry's review as well). Thanks!

@EricCousineau-TRI EricCousineau-TRI merged commit e08a581 into pybind:master Apr 19, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 19, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
daquexian added a commit to Oneflow-Inc/oneflow that referenced this pull request Jul 26, 2021
Signed-off-by: daquexian <daquexian566@gmail.com>
oneflow-ci-bot added a commit to Oneflow-Inc/oneflow that referenced this pull request Jul 28, 2021
* move tensor to c++

Signed-off-by: daquexian <daquexian566@gmail.com>

* align with master and fix export error

Signed-off-by: daquexian <daquexian566@gmail.com>

* reformat

Signed-off-by: daquexian <daquexian566@gmail.com>

* rename some functions

Signed-off-by: daquexian <daquexian566@gmail.com>

* refine

Signed-off-by: daquexian <daquexian566@gmail.com>

* remove unused header

Signed-off-by: daquexian <daquexian566@gmail.com>

* restore device repr and str

Signed-off-by: daquexian <daquexian566@gmail.com>

* upgrade pybind11 to 2.7.0 for pybind/pybind11#2956

Signed-off-by: daquexian <daquexian566@gmail.com>

* impl parameter in c++, remove determine() call

Signed-off-by: daquexian <daquexian566@gmail.com>

* let parameter derive from TensorIf to own a separate grad_fn_node, add missing get_device method, remove out-dated methods in docs

Signed-off-by: daquexian <daquexian566@gmail.com>

* add test_parameter.py, fix requires_grad_ and get_device

Signed-off-by: daquexian <daquexian566@gmail.com>

* use oneflow cast instead of numpy cast

Signed-off-by: daquexian <daquexian566@gmail.com>

* restore format changes

Signed-off-by: daquexian <daquexian566@gmail.com>

* auto format by CI

* fix tests involving _local_or_consistent_tensor

Signed-off-by: daquexian <daquexian566@gmail.com>

Co-authored-by: oneflow-ci-bot <ci-bot@oneflow.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants