[sgd] Add file lock to protect compilation of sgd op #3486

pcmoritz · 2018-12-06T19:12:55Z

What do these changes do?

Related issue number

robertnishihara

Nice! Thanks for doing this!

robertnishihara · 2018-12-06T19:16:42Z

python/ray/experimental/sgd/sgd_worker.py

@@ -113,7 +115,11 @@ def __init__(self,
            manager_socket = (
                ray.worker.global_worker.plasma_client.manager_socket_name)
            if not plasma.tf_plasma_op:
+                lock_path = os.path.join(pyarrow.__path__[0], "tensorflow", "compile_op.lock")
+                lock = SoftFileLock(lock_path)
+                lock.acquire()


Let's make this work with a with statement?

If not, use try/finally, but I think with is better.

robertnishihara · 2018-12-06T19:17:17Z

python/ray/experimental/sgd/util.py

+# Soft file lock -- behavior depends on the underlying filesystem.
+# For a proper lock with additional dependencies see the "filelock"
+# package.
+class SoftFileLock(object):


document

Is this portable across OS's?

robertnishihara · 2018-12-06T19:19:27Z

python/ray/experimental/sgd/util.py

+
+    import threading
+    import random
+    print("Testing file lock, this will take a few seconds...")


Use logger instead of print

robertnishihara · 2018-12-06T19:19:46Z

python/ray/experimental/sgd/util.py

+    print("Testing file lock, this will take a few seconds...")
+    lock_id = random.randint(0, 100000)
+    lock_path = "/tmp/test" + str(lock_id) + ".lock"
+    lock1 = SoftFileLock(lock_path)


Does this test actually ever get run?

robertnishihara · 2018-12-06T19:19:54Z

python/ray/experimental/sgd/util.py

+
+    threading.Thread(target=try_hold_lock).start()
+    lock1.acquire()
+    print("Locking test successful!")


robertnishihara · 2018-12-06T19:21:19Z

python/ray/experimental/sgd/util.py

+        time.sleep(10.0)
+        lock2.release()
+
+    threading.Thread(target=try_hold_lock).start()


For the test, maybe run 100 of them in separate threads (where each one sleeps for 0.1s) and make sure the whole thing takes at least 10s or something like that?

robertnishihara · 2018-12-06T19:22:15Z

python/ray/experimental/sgd/sgd_worker.py

                plasma.build_plasma_tensorflow_op()
+                lock.release()


Instead of calling plasma.build_plasma_tensorflow_op() here, maybe we should provide a wrapped version that includes the filelock and call that here.

robertnishihara · 2018-12-06T19:47:44Z

python/setup.py

@@ -136,6 +136,7 @@ def find_version(*filepath):

 requires = [
    "numpy",
+    "filelock",


Can you check that this works in Python 2?

pcmoritz · 2018-12-06T19:48:33Z

If we want to be fancy here it makes more sense to depend on the filelock package directly (which I wanted to avoid but it is also used in the local version of the autoscaler). I made the appropriate changes.

AmplabJenkins · 2018-12-06T21:03:47Z

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

robertnishihara · 2018-12-06T21:06:22Z

@pcmoritz this doesn't quite protect against the scenario where we call tf.load_op_library(TF_PLASMA_OP_PATH) from import pyarrow and the file is only half written, right?

https://github.com/apache/arrow/blob/b731b583e870726121d0adeebad014981e05b36c/python/pyarrow/plasma.py#L37-L44

AmplabJenkins · 2018-12-06T21:14:43Z

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

AmplabJenkins · 2018-12-06T21:16:24Z

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

pcmoritz · 2018-12-06T21:32:04Z

Yeah I think you are right. This needs apache/arrow#3117

AmplabJenkins · 2018-12-06T22:17:58Z

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

AmplabJenkins · 2018-12-06T22:33:51Z

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

AmplabJenkins · 2018-12-06T23:15:45Z

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

richardliaw · 2018-12-06T23:22:03Z

cmake/Modules/ArrowExternalProject.cmake

@@ -14,11 +14,11 @@
 #  - PLASMA_STATIC_LIB
 #  - PLASMA_SHARED_LIB

-set(arrow_URL https://github.com/apache/arrow.git)
+set(arrow_URL https://github.com/pcmoritz/arrow.git)


is this temporary or are we're officially building from @pcmoritz's arrow?

We will not merge this until it's changed back to apache/arrow. This is always just temporary for testing.

AmplabJenkins · 2018-12-08T02:42:26Z

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

AmplabJenkins · 2018-12-08T03:59:11Z

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

pcmoritz · 2018-12-08T04:54:54Z

jenkins retest this please

AmplabJenkins · 2018-12-08T06:24:09Z

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

robertnishihara · 2018-12-08T17:06:53Z

Jenkins, retest this please.

AmplabJenkins · 2018-12-08T18:35:47Z

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

richardliaw

Looks good aside from lint errors

richardliaw · 2018-12-09T19:15:18Z

python/ray/experimental/sgd/sgd_worker.py

@@ -2,16 +2,18 @@
 from __future__ import division
 from __future__ import print_function

+import filelock


not needed?

AmplabJenkins · 2018-12-09T21:36: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/9903/
Test FAILed.

pcmoritz added 2 commits December 6, 2018 11:11

add file lock to protect compilation of sgd op

836b217

lint

2f79695

robertnishihara reviewed Dec 6, 2018

View reviewed changes

pcmoritz added 3 commits December 6, 2018 11:31

update

1d70f71

fix

4b12c98

fix

db29bb7

robertnishihara reviewed Dec 6, 2018

View reviewed changes

lint

99cdb22

update

f986bec

richardliaw reviewed Dec 6, 2018

View reviewed changes

pcmoritz added 2 commits December 7, 2018 18:30

Merge branch 'master' into fix-sgd-compiling

6d11f9f

rebase on arrow

aec97ee

richardliaw approved these changes Dec 9, 2018

View reviewed changes

Update sgd_worker.py

68c7079

pcmoritz merged commit 87c0d24 into ray-project:master Dec 9, 2018

pcmoritz deleted the fix-sgd-compiling branch December 9, 2018 21:52

richardliaw mentioned this pull request Dec 11, 2018

[sgd] File Too Short Error #3405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sgd] Add file lock to protect compilation of sgd op #3486

[sgd] Add file lock to protect compilation of sgd op #3486

pcmoritz commented Dec 6, 2018

robertnishihara left a comment

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

robertnishihara Dec 6, 2018

pcmoritz Dec 6, 2018

pcmoritz commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

robertnishihara commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

pcmoritz commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

richardliaw Dec 6, 2018

robertnishihara Dec 6, 2018 •

edited

Loading

AmplabJenkins commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

pcmoritz commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

robertnishihara commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

richardliaw left a comment

richardliaw Dec 9, 2018

AmplabJenkins commented Dec 9, 2018

[sgd] Add file lock to protect compilation of sgd op #3486

[sgd] Add file lock to protect compilation of sgd op #3486

Conversation

pcmoritz commented Dec 6, 2018

What do these changes do?

Related issue number

robertnishihara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

robertnishihara commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

pcmoritz commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

Choose a reason for hiding this comment

robertnishihara Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

pcmoritz commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

robertnishihara commented Dec 8, 2018

AmplabJenkins commented Dec 8, 2018

richardliaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Dec 9, 2018

robertnishihara Dec 6, 2018 •

edited

Loading