Create RemoteFunction class, remove FunctionProperties, simplify worker Python code. by robertnishihara · Pull Request #2052 · ray-project/ray

robertnishihara · 2018-05-13T23:27:37Z

Related to #614 and #2016. Some initial cleanups.

richardliaw · 2018-05-13T23:37:58Z

Would it make sense for @ray.method to be renamed as @ray.split?
This would resolve this issue for singletons (and clean up sharded parameter server impls): #1665

AmplabJenkins · 2018-05-14T00:34:41Z

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

robertnishihara · 2018-05-14T00:36:24Z

The reasoning for ray.method was that we may want to add additional arguments into the decorator later.

Your suggestion would definitely clean up certain applications, although it might be confusing for @ray.method(num_return_vals=1) to be different from no decorator.

robertnishihara · 2018-05-14T00:37:49Z

Though it sounds like the current behavior is also confusing as described in #1665.

If we do that, then we should probably also change the behavior of @ray.remote(num_return_vals=1).

AmplabJenkins · 2018-05-14T01:17:27Z

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

AmplabJenkins · 2018-05-14T01:27:16Z

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

AmplabJenkins · 2018-05-14T02:18:00Z

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

richardliaw · 2018-05-14T06:28:45Z

I think having a list of return values would be better (whether it be num_return_vals or some better naming)

…ve FunctionProperties object.

robertnishihara · 2018-05-14T06:56:17Z

@richardliaw you're talking about the case where num_return_vals is specified, right? But not the default case, right? That might make sense, I'm curious what others think. That said, I'd prefer not to introduce any API changes in this PR.

robertnishihara · 2018-05-14T06:57:06Z

@suquark, this PR doesn't fix the issues you bring up in #2016, but some of these refactorings are necessary in order to address those issues.

richardliaw · 2018-05-14T07:02:14Z

yeah, for the specified case. Subsequent PR would be fine if others also approve.

AmplabJenkins · 2018-05-14T08:05:15Z

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

richardliaw

nits

richardliaw · 2018-05-14T08:15:31Z

python/ray/remote_function.py

+        function_id_hash.update(inspect.getsource(function).encode("ascii"))
+    # Compute the function ID.
+    function_id = function_id_hash.digest()
+    assert len(function_id) == 20


is 20 a ray constant?

Yeah, that's the size of our IDs. I started fixing this, by replacing it with a variable ray._ID_SIZE. However, this touches many many files so I think I'll submit a separate PR for that (the 20 isn't introduced here, it is just moved around from somewhere else).

richardliaw · 2018-05-14T08:19:11Z

python/ray/remote_function.py

+                num_cpus=None,
+                num_gpus=None,
+                resources=None):
+        """An experimental alternate way to submit remote functions."""


given that this is marked experimental, is this used?

It's used in Pandas on Ray (to change the number of return values), and I think it will be used more in the future (e.g., to specify different resource requirements at runtime).

richardliaw · 2018-05-14T08:24:22Z

python/ray/worker.py

         ) += 1

        reached_max_executions = (self.num_task_executions[task.driver_id().id(
-        )][function_id.id()] == self.function_properties[task.driver_id().id()]


task.driver_id(), task.task_id() and function_id.id() seem to be used multiple times - can consider abstracting into variable to decrease verbosity.

Fixed for task.driver_id(). It didn't obviously make sense to me for the others, but I do want to do a subsequent PR cleaning up some of this stuff (but not super sure what the scope of that is yet).

richardliaw · 2018-05-14T08:25:56Z

python/ray/worker.py


 def remote(*args, **kwargs):
-    """This decorator is used to define remote functions and to define actors.
+    worker = ray.worker.global_worker


get_global_worker()?

AmplabJenkins · 2018-05-14T20:34: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/5380/
Test PASSed.

AmplabJenkins · 2018-05-14T20:50:30Z

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

* master: Create RemoteFunction class, remove FunctionProperties, simplify worker Python code. (ray-project#2052) Don't crash on duplicate actor notifications (ray-project#2043) Fixed attribute name in code example (ray-project#2054) [xray] Add Travis build for testing xray on Linux. (ray-project#2047) Added missing comma to code example (ray-project#2050) Use more CPUs for testMultipleWaitsAndGets. (ray-project#2051) use jobid_nil (ray-project#2044) Fix typo in tune. (ray-project#2046) Fix error in api.rst. (ray-project#2048) Improve shared_ptr usage (ray-project#2030)

* fix-a3c-torch: Fix shape error in conv nets Ensure that values are flat list rm all use of torch Variables Create RemoteFunction class, remove FunctionProperties, simplify worker Python code. (ray-project#2052) Don't crash on duplicate actor notifications (ray-project#2043) rm unnecessary Variable wrapper Fixed attribute name in code example (ray-project#2054) [xray] Add Travis build for testing xray on Linux. (ray-project#2047) Added missing comma to code example (ray-project#2050) Use more CPUs for testMultipleWaitsAndGets. (ray-project#2051) use jobid_nil (ray-project#2044) Fix typo in tune. (ray-project#2046) Fix error in api.rst. (ray-project#2048) Improve shared_ptr usage (ray-project#2030) replace deprecated function Fmt Fix shapes of tensors Rename argument name to out_size Use correct pytorch functions Use F.softmax instead of a pointless network layer

robertnishihara added 8 commits May 13, 2018 23:49

Cleaning up worker and actor code. Create remote function class. Remo…

07f4156

…ve FunctionProperties object.

Remove register_actor_signatures function.

de2fb22

Small cleanups.

a7789e1

Fix linting.

af4a578

Support @ray.method syntax for actor methods.

adf47aa

Fix pickling bug.

1a3ba71

Fix linting.

85c5e9a

Shorten testBlockingTasks.

4ae6c10

richardliaw reviewed May 14, 2018

View reviewed changes

robertnishihara added 2 commits May 14, 2018 11:31

Small fixes.

5fd6007

Call get_global_worker().

75c89c5

pcmoritz approved these changes May 14, 2018

View reviewed changes

pcmoritz merged commit 8fbb884 into ray-project:master May 14, 2018

pcmoritz deleted the workercleanups branch May 14, 2018 21:35

stephanie-wang mentioned this pull request May 31, 2018

[xray] Terminated actors with queued tasks crashes raylet #2041

Closed

Conversation

robertnishihara commented May 13, 2018

Uh oh!

richardliaw commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

robertnishihara commented May 14, 2018

Uh oh!

robertnishihara commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

richardliaw commented May 14, 2018

Uh oh!

robertnishihara commented May 14, 2018

Uh oh!

robertnishihara commented May 14, 2018

Uh oh!

richardliaw commented May 14, 2018

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

richardliaw 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!

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!

AmplabJenkins commented May 14, 2018

Uh oh!

AmplabJenkins commented May 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

richardliaw commented May 13, 2018 •

edited

Loading

robertnishihara commented May 14, 2018 •

edited

Loading