Fix bug in which remote function redefinition doesn't happen. #6175

robertnishihara · 2019-11-16T02:24:18Z

This should fix Function definitions are not always updated when redefined. #6130.
If a user defines the same remote function many (100) times (e.g., from within 100 separate tasks), then a warning will be printed.
We also warn for actors.
Spurious warnings are possible (e.g., if the something in the remote functions closure changes so the remote function is not actually the same).
This only works for Python 3 because it uses the dis module.
The behavior is slightly different for Python 3.7+ (hopefully better), but I didn't actually try it out yet.
It's possible that dis.dis is sufficiently nondeterministic that we don't always get the warning when we should. However, I haven't noticed this happening.

AmplabJenkins · 2019-11-16T02:30:02Z

Can one of the admins verify this patch?

python/ray/function_manager.py

AmplabJenkins · 2019-11-16T07:55:28Z

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

robertnishihara · 2019-11-16T07:59:35Z

@edoakes I just tried the following on 3840 cores.

@ray.remote                                                    
def f():
    @ray.remote
    def g():
        return 1
    return ray.get(g.remote())

%time results = ray.get([g.remote() for _ in range(3840 // 2)])

It took 1 minute, which of course is absurdly slow because of the N^2 effect.

One possible way to help (though certainly not the perfect fix) is to detect the scenario where the N^2 effect is happening (e.g., by detecting that many of the remote functions defined have the same source code) and give a good warning to the user (e.g., tell them to pull the remote function definition back to the driver instead of having it defined in the workers.

AmplabJenkins · 2019-11-16T10:19:19Z

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

AmplabJenkins · 2019-11-16T10:33:06Z

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

AmplabJenkins · 2019-11-16T14:02:16Z

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

AmplabJenkins · 2019-11-16T14:29:13Z

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

edoakes

As we discussed this looks good, let's just be sure to give a good warning when there are many redefinitions and fix it by pulling from workers in the near future.

AmplabJenkins · 2019-11-24T07:57:11Z

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

python/ray/tests/test_failure.py

python/ray/function_manager.py

AmplabJenkins · 2019-11-25T12:43:54Z

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

edoakes · 2019-11-25T16:13:04Z

python/ray/function_manager.py

- # Source code may not be available:
- # e.g. Cython or Python interpreter.
- function_source_hash = b""
+ hasher = hashlib.sha1()


Just do hashlib.sha1(pickled_function).digest()

edoakes · 2019-11-25T16:13:23Z

python/ray/function_manager.py

+ """The identifier is used to detect excessive duplicate exports.
+
+ The identifier is used to determine when the same function or class is
+ exported many times. This can yielf false positives.


edoakes · 2019-11-25T16:15:21Z

python/ray/function_manager.py

+ collision_identifier = function_or_class.__name__
+
+ # Return a hash of the identifier in case it is too large.
+ hasher = hashlib.sha1()


same as above

edoakes · 2019-11-25T16:15:40Z

python/ray/import_thread.py

@@ -30,13 +32,19 @@ class ImportThread(object):
 redis_client: the redis client used to query exports.
 threads_stopped (threading.Event): A threading event used to signal to
 the thread that it should exit.
+ imported_collision_identifiers: This is a dicitonary mapping strings


Suggested change

imported_collision_identifiers: This is a dicitonary mapping strings

imported_collision_identifiers: This is a dictionary mapping strings

edoakes · 2019-11-25T16:17:03Z

python/ray/import_thread.py

@@ -30,13 +32,19 @@ class ImportThread(object):
 redis_client: the redis client used to query exports.
 threads_stopped (threading.Event): A threading event used to signal to
 the thread that it should exit.
+ imported_collision_identifiers: This is a dicitonary mapping strings
+ containing the collision identifier of the remote functions that


strings containing the collision identifier of the remote functions that have been exported -> collision identifiers for exported remote functions

edoakes · 2019-11-25T16:17:20Z

python/ray/import_thread.py

+ imported_collision_identifiers: This is a dicitonary mapping strings
+ containing the collision identifier of the remote functions that
+ have been exported to the number of times each remote function has
+ been imported. this is used to provide good error messages when the


Suggested change

been imported. this is used to provide good error messages when the

been imported. This is used to provide good error messages when the

edoakes · 2019-11-25T16:17:27Z

python/ray/import_thread.py

+ containing the collision identifier of the remote functions that
+ have been exported to the number of times each remote function has
+ been imported. this is used to provide good error messages when the
+ same function is exported many many times.


Suggested change

same function is exported many many times.

same function is exported many times.

edoakes · 2019-11-25T16:23:13Z

python/ray/import_thread.py

+ if key.startswith(b"RemoteFunction"):
+ collision_identifier, function_name = (
+ self.redis_client.hmget(
+ key, ["collision_identifier", "name"]))


Suggested change

key, ["collision_identifier", "name"]))

key, ["collision_identifier", "function_name"]))

This would be more clear and matches class_name used for actors. If this requires making many changes, don't worry about it.

edoakes · 2019-11-25T16:24:58Z

python/ray/import_thread.py

+ "https://github.com/ray-project/ray/issues/6240 for more "
+ "discussion.")
+
+ if key.startswith(b"RemoteFunction"):


Can we abstract this whole if/else block into a helper function that returns a collision identifier, type, and name? Better to separate out the redis implementation details from the actual logic.

AmplabJenkins · 2019-11-25T23:12:52Z

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

AmplabJenkins · 2019-11-25T23:32:31Z

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

AmplabJenkins · 2019-11-26T04:45:49Z

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

robertnishihara changed the title ~~[WIP] Fix bug in which remote function redefinition doesn't happen.~~ Fix bug in which remote function redefinition doesn't happen. Nov 16, 2019

robertnishihara commented Nov 16, 2019

View reviewed changes

python/ray/function_manager.py Outdated Show resolved Hide resolved

edoakes requested changes Nov 16, 2019

View reviewed changes

python/ray/function_manager.py Outdated Show resolved Hide resolved

edoakes approved these changes Nov 20, 2019

View reviewed changes

This was referenced Nov 22, 2019

Function definition collision for functions with duplicate names and bodies #5493

Closed

Performance issues with defining remote functions and actor classes from within tasks. #6240

Open

robertnishihara force-pushed the fixredefinition branch from a6fd9a5 to dfe9eba Compare November 24, 2019 02:36

robertnishihara force-pushed the fixredefinition branch from dfe9eba to 8ef02fe Compare November 25, 2019 07:07

robertnishihara commented Nov 25, 2019

View reviewed changes

python/ray/tests/test_failure.py Show resolved Hide resolved

robertnishihara commented Nov 25, 2019

View reviewed changes

python/ray/function_manager.py Show resolved Hide resolved

edoakes reviewed Nov 25, 2019

View reviewed changes

robertnishihara added 8 commits November 25, 2019 17:01

Fix bug in which remote function redefinition doesn't happen.

a8a8eb9

Add test.

790b4c3

Another more complex test.

a5ed188

Reorganize test a little.

1416ee3

Small fix.

e3ec5df

Simplify

7f8e165

Only pickle the function once.

17f0bad

Print warning and add test.

23d1452

robertnishihara added 3 commits November 25, 2019 17:01

Warn for actors as well.

3ea4739

Address comments.

678ab90

Slight reorganization.

8cf3e5a

robertnishihara force-pushed the fixredefinition branch from 76085d9 to 8cf3e5a Compare November 26, 2019 01:01

edoakes merged commit ffb9c0e into ray-project:master Nov 26, 2019

robertnishihara deleted the fixredefinition branch November 26, 2019 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in which remote function redefinition doesn't happen. #6175

Fix bug in which remote function redefinition doesn't happen. #6175

robertnishihara commented Nov 16, 2019 •

edited

Loading

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

robertnishihara commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

edoakes left a comment

AmplabJenkins commented Nov 24, 2019

AmplabJenkins commented Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

edoakes Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 26, 2019

	imported_collision_identifiers: This is a dicitonary mapping strings
	imported_collision_identifiers: This is a dictionary mapping strings

	been imported. this is used to provide good error messages when the
	been imported. This is used to provide good error messages when the

	same function is exported many many times.
	same function is exported many times.

	key, ["collision_identifier", "name"]))
	key, ["collision_identifier", "function_name"]))

Fix bug in which remote function redefinition doesn't happen. #6175

Fix bug in which remote function redefinition doesn't happen. #6175

Conversation

robertnishihara commented Nov 16, 2019 • edited Loading

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

robertnishihara commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 16, 2019

edoakes left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 24, 2019

AmplabJenkins commented Nov 25, 2019

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

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 26, 2019

robertnishihara commented Nov 16, 2019 •

edited

Loading