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.

Sign up for GitHub

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

Jump to bottom

ARROW-2657: [Python] Import TensorFlow python extension before pyarrow to avoid segfault #2210

Closed
wants to merge 11 commits into from
Closed

ARROW-2657: [Python] Import TensorFlow python extension before pyarrow to avoid segfault #2210

wants to merge 11 commits into from

Conversation

pcmoritz
Copy link
Contributor

@pcmoritz pcmoritz commented Jul 2, 2018

No description provided.

@pcmoritz
Copy link
Contributor Author

pcmoritz commented Jul 3, 2018

This is now ready for review! This is only tested on linux, where are the mac wheels being built?

@codecov-io
Copy link

codecov-io commented Jul 3, 2018 •
edited
Loading

Codecov Report

Merging #2210 into master will decrease coverage by 2.44%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2210      +/-   ##
==========================================
- Coverage   86.52%   84.07%   -2.45%     
==========================================
  Files         244      288      +44     
  Lines       41893    43771    +1878     
==========================================
+ Hits        36246    36801     +555     
- Misses       5647     6939    +1292     
- Partials        0       31      +31
Impacted Files Coverage Δ
python/pyarrow/__init__.py 58.62% <100%> (+1.47%) ⬆️
python/pyarrow/compat.py 74.59% <70%> (-0.91%) ⬇️
python/pyarrow/tests/test_types.py 100% <0%> (ø) ⬆️
python/pyarrow/tests/test_schema.py 100% <0%> (ø) ⬆️
go/arrow/memory/util.go 100% <0%> (ø)
go/arrow/array/boolean.go 84.61% <0%> (ø)
go/arrow/array/bufferbuilder_byte.go 50% <0%> (ø)
go/arrow/array/booleanbuilder.go 58.2% <0%> (ø)
go/arrow/memory/memory_sse4_amd64.go 100% <0%> (ø)
go/arrow/math/uint64_avx2_amd64.go 0% <0%> (ø)
... and 39 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 18f31e2...92aef7a. Read the comment docs.

wesm
wesm reviewed Jul 3, 2018
View reviewed changes
python/pyarrow/__init__.py Outdated
SITE_PATH, = site.getsitepackages()
ctypes.CDLL(os.path.join(SITE_PATH, "tensorflow",
"libtensorflow_framework.so"))
except:
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to make this less restrictive. You could even get away without a try/except and only attempt to load the shared lib if the TF folder is there. How about putting this in a function in pyarrow.compat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

robertnishihara
robertnishihara reviewed Jul 3, 2018
View reviewed changes
ci/travis_script_manylinux.sh

# Testing for https://issues.apache.org/jira/browse/ARROW-2657
# These tests cannot be run inside of the docker container, since TensorFlow
# does not run on manylinux1
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this test fail before this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried it on an ec2 instance before putting it in.

@robertnishihara
Copy link
Contributor

Do you anticipate this fix breaking in any cases?

@pcmoritz
Copy link
Contributor Author

pcmoritz commented Jul 4, 2018

@robertnishihara So I think the current version should be pretty robust and cover the corner cases (there might be some corner cases where it might incur the full TensorFlow loading time, but in most cases it will be fast)

@pcmoritz
Copy link
Contributor Author

pcmoritz commented Jul 4, 2018

This should now be ready to merge, let me know if there are other comments! I plan to merge it later tonight/early tomorrow morning if there are none.

@robertnishihara
Copy link
Contributor

Yeah, the primary failure mode that I'm thinking of is somebody modifies sys.path to include tensorflow after pyarrow as been imported. There will probably be other issues, but we'll see.

wesm
wesm reviewed Jul 4, 2018
View reviewed changes
python/pyarrow/__init__.py Outdated


# Workaround for https://issues.apache.org/jira/browse/ARROW-2657
import_tensorflow_extension()
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this exposes import_tensorflow_extension in the pyarrow.* namespace. I'll fix

@wesm
Use compat namespace to avoid adding import_tensorflow_extension to p…
92aef7a 
…yarrow.* namespace

Change-Id: I94bd60d98b64cab6105fbdda793bc82050dc640e
@wesm
Copy link
Member

wesm commented Jul 4, 2018

+1. Will merge once the builds complete

@pcmoritz pcmoritz closed this in e7aaf7b Jul 4, 2018
@pcmoritz pcmoritz deleted the try-fixing-tf-crash branch July 4, 2018 20:09
@robertnishihara robertnishihara mentioned this pull request Jul 4, 2018
Closed
pitrou
pitrou reviewed Jul 4, 2018
View reviewed changes
python/pyarrow/__init__.py


# Workaround for https://issues.apache.org/jira/browse/ARROW-2657
compat.import_tensorflow_extension()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be run only on Linux?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry I missed that (it was in the prior iteration of the patch but I didn't look closely enough at this function): https://issues.apache.org/jira/browse/ARROW-2795

@wesm wesm mentioned this pull request Jul 5, 2018
Closed
wesm added a commit that referenced this pull request Jul 5, 2018
@wesm
ARROW-2795: [Python] Run TensorFlow import workaround only on Linux p…
0175167 
…latforms

Per comments in #2210

Author: Wes McKinney <wesm+git@apache.org>

Closes #2218 from wesm/ARROW-2795 and squashes the following commits:

9825fbf <Wes McKinney> Run TensorFlow import workaround only on Linux platforms
@pcmoritz pcmoritz mentioned this pull request Jul 26, 2018
Closed
@diegoferigo diegoferigo mentioned this pull request Feb 7, 2021
Open
@diegoferigo diegoferigo mentioned this pull request Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesm wesm wesm left review comments

@pitrou pitrou pitrou left review comments

@robertnishihara robertnishihara robertnishihara approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pcmoritz @codecov-io @robertnishihara @wesm @pitrou