Skip to content

Conversation

@alsrgv
Copy link

@alsrgv alsrgv commented May 1, 2019

This patch addresses the incompatibility of pyarrow and tensorflow wheels provided by Google.

It has been tested with:

import pyarrow
import tensorflow

As well as more complicated examples such as https://github.com/horovod/horovod/blob/master/examples/keras_spark_rossmann.py which uses PyArrow + Petastorm to read the data.

Fixes https://issues.apache.org/jira/browse/ARROW-5130

@alsrgv
Copy link
Author

alsrgv commented May 1, 2019

@pitrou, I saw in #1953 (comment) that std::__once* include was added to make Plasma & Arrow use the same symbols.

What's a good way to test whether this change allows them to co-exist without such reuse (in a hope that they both use symbols from libstdc++)?

@wesm
Copy link
Member

wesm commented May 1, 2019

Thanks for working on this. This is obviously a complex issue that we've invested a lot of time in trying to fix and haven't succeeded yet. There are a lot of build and system configurations we'll have to test out to see whether this causes issues

Would you mind using our preferred PR titling style (which is needed for our changelog management tools)? "ARROW-5130: [C++][Python] ..."

@alsrgv alsrgv changed the title Limit exporting of std::* symbols ARROW-5130: [C++][Python] Limit exporting of std::* symbols May 1, 2019
@codecov-io
Copy link

codecov-io commented May 1, 2019

Codecov Report

Merging #4232 into master will increase coverage by 1.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4232      +/-   ##
=========================================
+ Coverage   88.04%   89.2%   +1.16%     
=========================================
  Files         758     617     -141     
  Lines       92803   82345   -10458     
  Branches     1251       0    -1251     
=========================================
- Hits        81708   73457    -8251     
+ Misses      10982    8888    -2094     
+ Partials      113       0     -113
Impacted Files Coverage Δ
cpp/src/arrow/util/thread-pool-test.cc 97.66% <0%> (-0.94%) ⬇️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/ipc/file_reader.go
js/src/enum.ts
go/arrow/array/builder.go
js/src/Arrow.node.ts
js/src/schema.ts
go/arrow/type_traits_boolean.go
js/src/ipc/node/writer.ts
... and 132 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a41288...b82e320. Read the comment docs.

@alsrgv
Copy link
Author

alsrgv commented May 1, 2019

@wesm, I've updated the title and the PR. Now it passes both Travis integration tests & my internal tests mentioned in the description. What would be a good next step?

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM

Thanks for working on this.
@wesm @kszucs Could we add a test for this somewhere in our CI setup? (I'm not sure about the best location, maybe the manylinux1 nightlies?)

@pitrou
Copy link
Member

pitrou commented May 1, 2019

@alsrgv The symptom was crashes in Plasma. If there are no crashes on CI it should be good.

# a system with an older libstdc++ which doesn't include the necessary
# c++11 symbols.
std::*;
*std::__once_call*;
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why the leading asterisk?

Copy link
Author

Choose a reason for hiding this comment

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

There's this symbol that was causing issues, and it starts with void std::...:

void std::__once_call_impl<std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_base::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>&, bool&)> (std::__future_base::_State_base*, std::reference_wrapper<std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()> >, std::reference_wrapper<bool>)> >()

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks.

@wesm wesm closed this in 0289af2 May 2, 2019
@alsrgv alsrgv deleted the simple_symbols branch May 2, 2019 00:31
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

Successfully merging this pull request may close these issues.

5 participants