Add units to argument names in ray.init and ray.wait. #3666

robertnishihara · 2018-12-29T21:47:39Z

This replaces #3629.

This makes the following changes:

object_store_memory -> object_store_memory_bytes
redis_max_memory -> redis_max_memory_bytes
timeout -> timeout_milliseconds

Addresses #3411.

…y -> object_store_memory_bytes, timeout -> timeout_seconds.

robertnishihara · 2018-12-29T21:53:29Z

Following up on comments from #3629.

@ericl I agree megabytes are more intuitive here, but I felt that object_store_memory_mb and redis_max_memory_mb would be too ambiguous and people would still need to look at the documentation to check, whereas bytes are very clear. On the other hand writing out megabytes would be acceptable but starts to get long. On the other hand, for the timeout, seconds are probably the right unit, and timeout_seconds is perfectly clear.

@atumanov Consistency with SI unit prefixes is an interesting idea, I guess that means using Mb instead of mb. However, I can't quite bring myself to name a variable object_store_memory_Mb, and unfortunately it still seems less clear than bytes.

ericl · 2018-12-29T22:49:20Z

@robertnishihara I believe memory_mb is common terminology, imo the gains of removing six zeros is worth it even if it wasn't...

ericl · 2018-12-29T22:49:49Z

https://docs.oracle.com/database/nosql-12.1.3.1/AdminGuide/independent_commands.html

E.g., oracle cli uses memory_mb

atumanov · 2018-12-30T00:22:47Z

@robertnishihara , actually, it would be object_store_memory_MB for megabytes. Mb would be megabits. And MiB if we want to go with million bytes. But I agree that it's awkward, given our naming conventions. One possibility is to allow specifying human readable values like 10M, 1G. It's fairly standard to enable that for command line tools. I think there might be a readily available python library.

atumanov · 2018-12-30T00:30:55Z

something like this :
https://pypi.org/project/humanfriendly/

AmplabJenkins · 2018-12-30T00:48:08Z

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

robertnishihara · 2018-12-30T04:26:39Z

python/ray/ray_constants.py

 INFINITE_RECONSTRUCTION = 2**30
+
+# Max bytes to allocate to plasma unless overriden by the user
+DEFAULT_MAX_MEMORY_MB = 20 * 1000


moved to this file from services.py

robertnishihara · 2018-12-30T04:49:26Z

Thanks @ericl @atumanov Let's give _mb a try. I agree it is more intuitive. Hopefully people understand the abbreviation..

I really dislike the "humanfriendly" approach. It's more difficult to work with programmatically and it's so confusing that I always have to check the documentation or other examples every time I use it (e.g., for docker, is it --memory=1G or --memory=1g or --memory=1gb or any number of other possibilities).

AmplabJenkins · 2018-12-30T05:34:17Z

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

AmplabJenkins · 2018-12-30T05:37:29Z

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

AmplabJenkins · 2018-12-30T06:01:06Z

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

AmplabJenkins · 2018-12-30T06:04:08Z

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

AmplabJenkins · 2018-12-30T06:21:35Z

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

AmplabJenkins · 2018-12-30T06:32:19Z

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

AmplabJenkins · 2018-12-30T07:16:34Z

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

atumanov · 2018-12-30T16:05:02Z

to clarify my proposal was to try object_memory_size filtered through humanfriendly (or equiv) only at the I/O boundary with the user. It would then get immediately translated and all internal APIs could then use integral _bytes.
Note that with my proposal you can still specify the number of bytes, and it's supported as a special case. This is strictly more expressive.
The interface supported by humanfriendly is a Unix standard.
I still think that "mb" is confusing. Technically it means "millibits". Practically, it will leave some ambiguity as to whether it's megabytes or megabits. Only contextually it will be clear that we're talking about megabytes (because it's memory). But if this same approach is used, say, for network bandwidth, then the confusion will be very real. Your call.

atumanov · 2018-12-30T16:14:32Z

python/ray/services.py

            # Compare the requested memory size to the memory available in
            # /dev/shm.
-            if shm_avail > object_store_memory:
+            if shm_avail > object_store_memory_mb:


are you comparing bytes to megabytes here?

Thanks a lot! that was a bug!

atumanov · 2018-12-30T16:16:17Z

python/ray/services.py

    # Print the object store memory using two decimal places.
-    object_store_memory_str = (object_store_memory / 10**7) / 10**2
-    logger.info("Starting the Plasma object store with {} GB memory "
+    object_store_memory_str = (object_store_memory_mb // 10) / 10**2


this is a great place where a library could "pretty-print" the object store memory for you into a human readable format.

atumanov · 2018-12-30T16:25:19Z

python/ray/worker.py

            raise DeprecationWarning("The use_raylet argument is deprecated. "
                                     "Please remove it.")

+    if object_store_memory is not None:


object_store_memory_mb ?

This is intentional. It is used to print a deprecation warning.

atumanov · 2018-12-30T16:25:36Z

python/ray/worker.py

+                       "deprecated. Please use 'object_store_memory_mb'.")
+        object_store_memory_mb = object_store_memory / 10**6
+
+    if redis_max_memory is not None:


redis_max_memory_mb ?

This is intentional. It is used to print a deprecation warning.

atumanov · 2018-12-30T16:32:31Z

python/ray/worker.py

+def wait(object_ids,
+         num_returns=1,
+         timeout_seconds=None,
+         timeout=None,


I see you're trying to make it backward compatible. However, what I'm worried about is existing applications using positional arguments instead of keyword arguments. This change will break them, unfortunately, due to the mismatch of time units between timeout and timeout_seconds! I propose explicitly deprecating the previous interface. See below.

we could consider using @deprecated on the old interface, and make it call the new interface. What I'm worried about here is that the current approach is changing the API twice, once to add timeout_seconds and again to remove timeout. This is a noop for keyword argument users, but is a breaking change for positional argument users. Why not:

@deprecated(version='0.6.2', reason='timeout deprecated in favor of timeout_seconds') def ray.wait(object_ids, num_returns=1, timeout=None, worker=global_worker): return ray.wait(object_ids = object_ids, num_returns = num_returns, timeout_seconds = timeout, global_worker = global_worker)

Hopefully that won't happen much, since using positional arguments for keyword arguments is a bad practice.

That said, I'm not sure I understand your suggestion as Python functions can't be overloaded.

That said, I am ok with immediately invalidating the old API instead of trying to do it gracefully.

sorry, I meant something like ray.future.wait(object_ids, num_returns, timeout_seconds, global_worker).

I see, thanks. I prefer to update the ray.wait API right away.

atumanov · 2018-12-30T16:49:04Z

src/ray/raylet/node_manager.cc

            << "by the redis LRU configuration. Consider increasing the memory "
               "allocation via "
-            << "ray.init(redis_max_memory=<max_memory_bytes>).";
+            << "ray.init(redis_max_memory_mb=<max_memory_bytes>).";


nit: =<max_memory_megabytes>

thanks, fixed

atumanov · 2018-12-30T16:54:18Z

test/actor_test.py

    for _ in range(num_objects):
-        obj = a.create_object.remote(object_store_memory // num_objects)
+        obj = a.create_object.remote(
+            10**6 * object_store_memory_mb // num_objects)


minor: This is correct, but I would put parens around (10**6 * object_store_memory_mb) to ensure/express correct/desired order of operations.

robertnishihara · 2018-12-31T23:13:09Z

Thanks @atumanov, I understand your proposal, however I still think the "human-readable" approach is too difficult to use.

AmplabJenkins · 2019-01-01T00:20:31Z

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

robertnishihara · 2019-01-01T00:47:22Z

Jenkins, retest this please.

AmplabJenkins · 2019-01-01T02:47: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/10520/
Test PASSed.

pcmoritz · 2019-01-01T21:44:32Z

Timeouts are in seconds everywhere in python, and the argument is called timeout, so we should adhere to this standard.

For the transition I suggest the following: Till 1.0, give a deprecation warning if timeout is given as an int; if it is given as a float, it is just interpreted as in seconds. Fortunately not too many people are using ray.wait with a timeout at the moment I think (it was a mistake to make it milliseconds initially).

atumanov

Looks good to me, esp. combined with #3706. The net effect of #3666 + #3706 will be unmodified ray.wait API, but with a breaking semantics change for the timeout parameter (seconds instead of milliseconds). The latter is done for consistency with Python unit conventions for timeouts.

atumanov · 2019-01-09T01:30:08Z

python/ray/experimental/api.py



-def wait(object_ids, num_returns=1, timeout=None, worker=None):
+def wait(object_ids, num_returns=1, timeout_seconds=None, worker=None):


we've agreed offline that this will go back to timeout in #3706 . Note that this will preserve the API, but will change the semantics (units) for the timeout parameter in the 0.6.2 release.

atumanov · 2019-01-09T01:39:09Z

python/ray/services.py

-        if object_store_memory > MAX_DEFAULT_MEM:
+        if object_store_memory_mb > ray.ray_constants.DEFAULT_MAX_MEMORY_MB:
            logger.warning(
                "Warning: Capping object memory store to {}GB. ".format(


nit: "capping object memory store" --> "capping object store memory"

atumanov · 2019-01-09T01:48:11Z

python/ray/services.py


        # Do some sanity checks.
-        if object_store_memory > system_memory:
+        if object_store_memory_mb > system_memory_mb:


nit: I would make this sanity check immediately after if plasma_directory is None
line: https://github.com/ray-project/ray/pull/3666/files#diff-54ac27010a06993004bec4677c7e583eR1048

atumanov · 2019-01-09T01:51:45Z

python/ray/test/cluster_utils.py

            cleanup=True,
            resources={"CPU": 1},
-            object_store_memory=100 * (2**20) # 100 MB
+            object_store_memory_mb=100  # 100 MB


We might want to document somewhere that we interpret MB as 10e6, not 2^20

robertnishihara · 2019-01-12T06:11:50Z

Closing for now due to #3706.

Rename redis_max_memory -> redis_max_memory_bytes, object_store_memor…

7584073

…y -> object_store_memory_bytes, timeout -> timeout_seconds.

robertnishihara mentioned this pull request Dec 29, 2018

Add units to argument names in ray.init and ray.wait. #3629

Closed

robertnishihara added 2 commits December 29, 2018 20:14

Change bytes to mb.

bccdd17

Small fix.

616ae98

robertnishihara commented Dec 30, 2018

View reviewed changes

robertnishihara added 2 commits December 29, 2018 20:37

Small fixes.

4bcbec7

Fix tests.

e6e3471

robertnishihara added 2 commits December 29, 2018 21:01

Fix

335d83f

Fix actor test.

ee74c58

Fix

631efc8

atumanov requested changes Dec 30, 2018

View reviewed changes

Fixes

71cb3c9

richardliaw assigned atumanov Jan 4, 2019

robertnishihara mentioned this pull request Jan 7, 2019

Change timeout from milliseconds to seconds in ray.wait. #3706

Merged

atumanov approved these changes Jan 9, 2019

View reviewed changes

atumanov added the api label Jan 9, 2019

robertnishihara closed this Jan 12, 2019

robertnishihara deleted the apiunits2 branch January 12, 2019 06:12



		def wait(object_ids, num_returns=1, timeout=None, worker=None):
		def wait(object_ids, num_returns=1, timeout_seconds=None, worker=None):

Add units to argument names in ray.init and ray.wait. #3666

Add units to argument names in ray.init and ray.wait. #3666

Uh oh!

Conversation

robertnishihara commented Dec 29, 2018

Uh oh!

robertnishihara commented Dec 29, 2018

Uh oh!

ericl commented Dec 29, 2018

Uh oh!

ericl commented Dec 29, 2018

Uh oh!

atumanov commented Dec 30, 2018

Uh oh!

atumanov commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

AmplabJenkins commented Dec 30, 2018

Uh oh!

atumanov commented Dec 30, 2018

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!

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!

robertnishihara commented Dec 31, 2018

Uh oh!

AmplabJenkins commented Jan 1, 2019

Uh oh!

robertnishihara commented Jan 1, 2019

Uh oh!

AmplabJenkins commented Jan 1, 2019

Uh oh!