Pyarrow Segfault Fix by ijrsvt · Pull Request #7568 · ray-project/ray

ijrsvt · 2020-03-11T22:24:12Z

Why are these changes needed?

Ray will segfault if pyarrow is imported before it because exported symbol are colliding . This fixes that bug and adds a test to ensure that future changes do not reintroduce it.

Related issue number

Fixes Issue #7393

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

AmplabJenkins · 2020-03-11T22:29:58Z

Can one of the admins verify this patch?

pcmoritz · 2020-03-11T22:40:05Z

The test script might be a little bit too clever (calling itself), have you considered doing python -c "import ray; import pyarrow" and vice versa?

We should also change the comment that contain RTLD_GLOBAL so they are in sync with the code.

Other than that it looks good to me!

@raulchen Can you check if there is any impact on the streaming system?

edoakes

Nice!

edoakes · 2020-03-11T22:56:35Z

python/ray/__init__.py

    import ctypes
    from ctypes import CDLL
-    CDLL(so_path, ctypes.RTLD_GLOBAL)
+    CDLL(so_path, ctypes.RTLD_LOCAL)


Should probably leave a comment here pointing to the issue this fixed

Definitely will add!

@ijrsvt Could you give an example which symbols conflict? We used linker script ray_exported_symbols.lds and ray_version_script.lds to limit exported symbols. And using RTLD_GLOBAL by purpose so that _streaming.so can using symbols in _raylet.so

The conflicting symbols were in the GRPC library. The problem only showed up on Linux, and when pyarrow was imported before ray. The specific symbol that was segfaulting was: google::protobuf::internal::AssignDescriptors.

Can you give more complete detailed log for this symbol segfault. I tink we didn't expose protobuf symbols in ray_exported_symbols.lds/ray_version_script.lds. And this import order issue exists before RTLD_GLOBAL.

It is here It appears to be some issue where there is a conflicting symbol from gRPC. A similar issue is here.

@chaokunyang Can you look more into this and why the symbols are conflicting despite the version linker script? This is blocking the current release I believe.

@pcmoritz I'm looking into it. There may be some symbols exported unexpectedly despite of the version linker script, like template. If that's the reason, maybe we can use __attribute__((visibility(....))). If not, we'll need more time for this.

ci/travis/check_symbol_collisions.py

AmplabJenkins · 2020-03-11T23:24:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23049/
Test FAILed.

AmplabJenkins · 2020-03-11T23:35:42Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23050/
Test FAILed.

AmplabJenkins · 2020-03-12T06:00:03Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23064/
Test FAILed.

AmplabJenkins · 2020-03-12T20:54:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23106/
Test FAILed.

AmplabJenkins · 2020-03-12T23:45:20Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23111/
Test FAILed.

AmplabJenkins · 2020-03-13T01:50:37Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23117/
Test PASSed.

AmplabJenkins · 2020-03-13T04:51:23Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23119/
Test FAILed.

chaokunyang · 2020-03-25T04:23:49Z

Hi @ijrsvt , I used Python 3.7.5, ray 0.8.2, pyarrow 0.16.0 in a Ubuntu 19.10 docker container. I can't reproduce your issue.

Could you recheck again that it's not a problem caused by other lib?

I also searched symbols in _raylet.so using nm -g _raylet.so | grep AssignDescriptors. But didn't find symbols related to AssignDescriptors.

I test ray 0.82 with pyarrow 0.16.0/0.15.0. Both of all works well. But when I use pyarrow 0.14.0, here is a segment fault. I think it's a issue of pyarrow?

ijrsvt · 2020-03-27T21:46:38Z

@chaokunyang I can't reproduce it either (and nor can the original bug reporter). I reverted the RTLD_LOCAL change.

AmplabJenkins · 2020-03-27T22:53:27Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23816/
Test PASSed.

pcmoritz · 2020-03-30T02:03:17Z

@ijrsvt After merging the PR it is causing an error, can you look at this?

This reverts commit 57599f0.

robertnishihara · 2020-03-30T03:49:57Z

python/ray/tests/test_symbol_collisions.py

+"""
+import subprocess
+
+TESTED_LIBRARIES = ["pyarrow"]


I'm mildly surprised that this test passes in our CI. Is pyarrow installed in our CI?

This doesn't fail if pyarrow is installed, only if there is some strange symbol collision. There was initially a fix to limit the scope of exported symbols, but that was removed after it became un-reproducable.

Right, I agree it doesn't fail if pyarrow is installed. I just thought that pyarrow isn't installed in our Travis tests.

Ohh okay! Let me check that.

I added Pyarrow to the dependencies. It looks like the error is a segfault again :/
https://travis-ci.com/github/ray-project/ray/jobs/308220125

Is this pyarrow version 0.14?

No, it is 0.16. I added the version to the debug info, so it shows up in this test.

This reverts commit 57599f0.

robertnishihara · 2020-03-30T04:04:03Z

python/ray/tests/test_symbol_collisions.py

+
+
+def test_imports():
+    def try_imports(library1, library2):


@ijrsvt slightly simpler way to do this (and I think will give a better error message)

subprocess.check_output(["python", "-c", "import {}; import {}".format(library1, library2)]) subprocess.check_output(["python", "-c", "import {}; import {}".format(library2, library1)])

Ahh okay. I'll switch to this!

…ay-project#7805)" This reverts commit eb61036.

Fixed segfault and added tests

59336d1

ijrsvt added 2 commits March 11, 2020 15:31

Added error messages

3fb7d1b

Formatting

1812c0e

ijrsvt requested a review from pcmoritz March 11, 2020 22:33

edoakes approved these changes Mar 11, 2020

View reviewed changes

ijrsvt added 4 commits March 11, 2020 22:04

Modifying script and adding docs

d530e97

formatting fixes

d9fd5f8

removing "python3"

0e6f2d9

modifying logic to give better errors

1314bb8

ijrsvt added 2 commits March 12, 2020 13:14

Moving to a standard pytest

6438fcd

linting

ee3ae85

Fixing pytest

c4b9a4d

ijrsvt added 2 commits March 12, 2020 17:29

missing quotes

7ad0fcc

comma add

0e516e2

Reverting RTLD_LOCAL

17670ae

pcmoritz approved these changes Mar 29, 2020

View reviewed changes

pcmoritz merged commit 57599f0 into ray-project:master Mar 29, 2020

pcmoritz added a commit that referenced this pull request Mar 30, 2020

Revert "Pyarrow Segfault Regression Test (#7568)"

a1c5ae0

This reverts commit 57599f0.

pcmoritz mentioned this pull request Mar 30, 2020

Revert "Pyarrow Segfault Fix" #7805

Merged

robertnishihara reviewed Mar 30, 2020

View reviewed changes

robertnishihara pushed a commit that referenced this pull request Mar 30, 2020

Revert "Pyarrow Segfault Regression Test (#7568)" (#7805)

eb61036

This reverts commit 57599f0.

robertnishihara reviewed Mar 30, 2020

View reviewed changes

ijrsvt added a commit to ijrsvt/ray that referenced this pull request Mar 30, 2020

Revert "Revert "Pyarrow Segfault Regression Test (ray-project#7568)" (r…

263997e

…ay-project#7805)" This reverts commit eb61036.

Conversation

ijrsvt commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

AmplabJenkins commented Mar 11, 2020

Uh oh!

pcmoritz commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chaokunyang Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijrsvt Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijrsvt Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AmplabJenkins commented Mar 11, 2020

Uh oh!

AmplabJenkins commented Mar 11, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

AmplabJenkins commented Mar 13, 2020

Uh oh!

AmplabJenkins commented Mar 13, 2020

Uh oh!

chaokunyang commented Mar 25, 2020

Uh oh!

ijrsvt commented Mar 27, 2020

Uh oh!

AmplabJenkins commented Mar 27, 2020

Uh oh!

pcmoritz commented Mar 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

ijrsvt commented Mar 11, 2020 •

edited

Loading

pcmoritz commented Mar 11, 2020 •

edited

Loading

chaokunyang Mar 13, 2020 •

edited

Loading

ijrsvt Mar 13, 2020 •

edited

Loading

ijrsvt Mar 16, 2020 •

edited

Loading