Add experimental API for ray.get and ray.wait with additional argument types #2071

kunalgosar · 2018-05-16T09:50:32Z

What do these changes do?

This adds in an experimental API to allow ray.experimental.get to take in lists, tuples, ndarrays or dicts. It also allows ray.experimental.wait to take in lists, tuples or ndarrays.

AmplabJenkins · 2018-05-16T11:02:08Z

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

robertnishihara · 2018-05-16T23:23:02Z

This seems like a good idea, some comments/questions:

Are iterables really the right abstraction? E.g., strings and dicts are iterable, but it doesn't make sense to call ray.get on them (definitely not on a string, and dicts would require more special handling). If we leave it this way, we may want to specifically check for iterables that don't work and raise exceptions or at least make sure we get a good error message. It make more sense to just specifically handle certain kinds of iterables, e.g., just lists and tuples.
Can you add some tests for this (in test/runtest.py, wherever we are already testing ray.get)?
We may want to respect the type, that is, if you call ray.get on a list, then you get a list back, and if you call it on a tuple, then you get a tuple back, though maybe always returning a list is ok.

AmplabJenkins · 2018-05-17T00:13:09Z

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

kunalgosar · 2018-05-17T08:20:13Z

I agree that Iterable alone is not the right abstraction. My original reasoning was that, passing in something weird (string) would get caught in the isinstance(object_id, ray.ObjectID) check. As you said, this can instead handle certain types (lists, tuples, dicts, np.ndarray and pd.Series). If this sounds right to you, I can update the PR to specifically handle these cases.
Once the specific cases are finalized, I'll update and add tests.
If each iterable type is handled differently, it can also ensure that the return type matches the input type.

I will also update other methods to support this once the types are finalized (e.g. ray.wait).

Additional question: Does it make sense to handle Iterables passed into ray.put? This is not supported currently.

robertnishihara · 2018-05-20T04:44:31Z

@kunalgosar some questions. What are the types that you think are important to support? Lists and tuples make a lot of sense. Maybe numpy arrays? Does anything else make sense?

I'm still unsure about whether the return container type should match the input container type..

We could make this change for ray.wait but there are more constraints. E.g., ray.wait should probably continue to return lists (I think).

Let's not change ray.put because it's hard to distinguish between if you pass a list to ray.put that may just mean that you want to put a list in the object store (as opposed to calling ray.put on each element of the list).

kunalgosar · 2018-05-22T07:39:50Z

I agree it makes sense to support lists and tuples. I would also add in numpy arrays and dictionaries. For the return type, I think that all of these could return just lists, except dictionaries, where is might make sense to return a dict back as well.

The types for ray.wait should probably follow the same as ray.get.

It would definitely be hard to change ray.put, without adding a parameter for num_vals or something similar. I won't change this here.

kunalgosar · 2018-05-23T04:38:04Z

python/ray/experimental/worker.py

+                        .format(threading.current_thread().getName()))
+
+
+def get(object_ids, worker=None):


@robertnishihara: The function names here can be changed, if needed. I was unsure what to call them so have kept it as get and wait for now.

AmplabJenkins · 2018-05-23T05:47:53Z

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

AmplabJenkins · 2018-05-23T06:41:14Z

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

kunalgosar · 2018-05-23T06:59:09Z

This passes on Private travis.

robertnishihara · 2018-05-25T18:19:10Z

@kunalgosar, instead of reimplementing get/wait, let's just implement these by calling ray.get and ray.wait under the hood. All we should need to do is to cast to a list and then call the original ray.get.

We shouldn't need to duplicate check_main_thread.

AmplabJenkins · 2018-05-29T19:56:14Z

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

AmplabJenkins · 2018-05-29T20:04: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/5690/
Test PASSed.

kunalgosar · 2018-05-30T00:17:42Z

@robertnishihara I've addressed the above comment and the test failures are unrelated.

robertnishihara

Thanks! This looks pretty good. Left some comments.

robertnishihara · 2018-05-31T20:30:16Z

python/ray/experimental/worker.py

+
+def get(object_ids, worker=None):
+    """Get a remote object or a collection of remote objects from the object
+    store.


the first doc line should fit on a single line

robertnishihara · 2018-05-31T20:37:03Z

python/ray/experimental/worker.py

+def get(object_ids, worker=None):
+    """Get a remote object or a collection of remote objects from the object
+    store.
+


Could you add a note explaining how this differs from ray.get. The block below is probably not necessary, since you can assume people know how ray.get works.

robertnishihara · 2018-05-31T20:47:03Z

python/ray/experimental/worker.py

+    if isinstance(object_ids, (list, tuple, np.ndarray)):
+        # There is a dependency on ray.worker which prevents importing
+        # global_worker at the top of this file
+        return ray.get(list(object_ids)) if worker is None else \


this worker is None appears a number of times, can we just do

worker = ray.worker.global_worker if worker is None else worker

once at the beginning of this function?

robertnishihara · 2018-05-31T20:52:38Z

python/ray/experimental/worker.py

+        return ray.get(list(object_ids)) if worker is None else \
+            ray.get(list(object_ids), worker)
+    elif isinstance(object_ids, dict):
+        to_get = [


I think this can be simplified, maybe something like

keys_to_get = [k for k, v in object_ids.items() if isinstance(v, ray.ObjectID)] ids_to_get = [v for k, v in object_ids.items() if isinstance(v, ray.ObjectID)] values = ray.get(ids_to_get) result = object_ids.copy() for key, value in zip(keys_to_get, values): result[key] = value

robertnishihara · 2018-05-31T20:53:21Z

python/ray/experimental/worker.py

+            result[key] = val
+        return result
+    else:
+        return ray.get(object_ids) if worker is None else \


In this file, let's avoid using \. If multiline things are necessary, let's do

return (ray.get(object_ids) if worker is None else ray.get(object_ids, worker))

robertnishihara · 2018-05-31T20:54:06Z

python/ray/experimental/worker.py

+def wait(object_ids, num_returns=1, timeout=None, worker=None):
+    """Return a list of IDs that are ready and a list of IDs that are not.
+
+    If timeout is set, the function returns either when the requested number of


I'd suggest explaining documenting this function primarily by explaining the difference from ray.wait.

robertnishihara · 2018-05-31T20:55:26Z

python/ray/experimental/worker.py

@@ -0,0 +1,78 @@
+from __future__ import absolute_import


Let's change this filename to be api.py

kunalgosar · 2018-05-31T22:54:40Z

@robertnishihara thanks for taking a look! I've resolved the above comments.

AmplabJenkins · 2018-05-31T23:27:21Z

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

kunalgosar · 2018-05-31T23:27:59Z

Jenkins, retest this please

AmplabJenkins · 2018-05-31T23:57:52Z

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

kunalgosar · 2018-06-01T07:25:02Z

Jenkins, retest this please

AmplabJenkins · 2018-06-01T08:34:03Z

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

robertnishihara · 2018-06-01T23:42:20Z

Looks good to me, thanks!

alok · 2018-06-03T23:13:51Z

@robertnishihara I think the "right sort of iterable" for this problem is actually pretty hard to express in Python. We want an iterable that contains only concrete types that is not itself a concrete type (which rules out strings). Dicts would require special handling to support, but this covers all the "flat" types like ndarrays, lists, sets (though order would be a sticking point there), and tuples.

kunalgosar force-pushed the allow_iterable branch from 6b7126d to 062eb48 Compare May 16, 2018 22:54

kunalgosar force-pushed the allow_iterable branch from 062eb48 to a0332cc Compare May 23, 2018 04:35

kunalgosar commented May 23, 2018

View reviewed changes

kunalgosar changed the title ~~Allow any Iterable type to ray.get~~ Add experimental API for ray.get and ray.wait additional argument types May 23, 2018

kunalgosar changed the title ~~Add experimental API for ray.get and ray.wait additional argument types~~ Add experimental API for ray.get and ray.wait with additional argument types May 23, 2018

kunalgosar mentioned this pull request May 23, 2018

Allow ray.get and ray.wait to take in additional argument types #2126

Open

kunalgosar force-pushed the allow_iterable branch from 92e476f to 5e0e15e Compare May 29, 2018 18:44

robertnishihara reviewed May 31, 2018

View reviewed changes

kunalgosar added 6 commits May 31, 2018 15:54

allow any iterable to ray.get

73c1705

Adding experimental get/put API

be59f6e

fix lint

16957bf

addressing comments

fbe2987

fix lint

ec91d0b

resolving comments

b38f8ad

kunalgosar force-pushed the allow_iterable branch from 55036f9 to b38f8ad Compare May 31, 2018 22:54

robertnishihara approved these changes Jun 1, 2018

View reviewed changes

robertnishihara merged commit 317d0da into ray-project:master Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental API for ray.get and ray.wait with additional argument types #2071

Add experimental API for ray.get and ray.wait with additional argument types #2071

kunalgosar commented May 16, 2018 •

edited

Loading

AmplabJenkins commented May 16, 2018

robertnishihara commented May 16, 2018

AmplabJenkins commented May 17, 2018

kunalgosar commented May 17, 2018

robertnishihara commented May 20, 2018

kunalgosar commented May 22, 2018

kunalgosar May 23, 2018

AmplabJenkins commented May 23, 2018

AmplabJenkins commented May 23, 2018

kunalgosar commented May 23, 2018

robertnishihara commented May 25, 2018

AmplabJenkins commented May 29, 2018

AmplabJenkins commented May 29, 2018

kunalgosar commented May 30, 2018

robertnishihara left a comment

robertnishihara May 31, 2018

robertnishihara May 31, 2018

robertnishihara May 31, 2018

robertnishihara May 31, 2018

robertnishihara May 31, 2018

robertnishihara May 31, 2018

robertnishihara May 31, 2018

kunalgosar commented May 31, 2018

AmplabJenkins commented May 31, 2018

kunalgosar commented May 31, 2018

AmplabJenkins commented May 31, 2018

kunalgosar commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

robertnishihara commented Jun 1, 2018

alok commented Jun 3, 2018

		.format(threading.current_thread().getName()))


		def get(object_ids, worker=None):

Add experimental API for ray.get and ray.wait with additional argument types #2071

Add experimental API for ray.get and ray.wait with additional argument types #2071

Conversation

kunalgosar commented May 16, 2018 • edited Loading

What do these changes do?

AmplabJenkins commented May 16, 2018

robertnishihara commented May 16, 2018

AmplabJenkins commented May 17, 2018

kunalgosar commented May 17, 2018

robertnishihara commented May 20, 2018

kunalgosar commented May 22, 2018

Choose a reason for hiding this comment

AmplabJenkins commented May 23, 2018

AmplabJenkins commented May 23, 2018

kunalgosar commented May 23, 2018

robertnishihara commented May 25, 2018

AmplabJenkins commented May 29, 2018

AmplabJenkins commented May 29, 2018

kunalgosar commented May 30, 2018

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

kunalgosar commented May 31, 2018

AmplabJenkins commented May 31, 2018

kunalgosar commented May 31, 2018

AmplabJenkins commented May 31, 2018

kunalgosar commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

robertnishihara commented Jun 1, 2018

alok commented Jun 3, 2018

kunalgosar commented May 16, 2018 •

edited

Loading