Skip to content

Conversation

@robertnishihara
Copy link
Collaborator

This makes the following changes:

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

@robertnishihara
Copy link
Collaborator Author

There was some disagreement about whether we should do this or not, so we should discuss this. cc @atumanov @ericl @pcmoritz

@pcmoritz pointed out that it's more common to use timeout in Python and for it to refer to seconds. However, that forces uses to look at the documentation to figure out how to use ray.wait, whereas timeout_milliseconds is much clearer. I'd also be ok with switching to timeout_seconds.

@ericl
Copy link
Contributor

ericl commented Dec 25, 2018

This is a breaking change right? I think we can also switch to mb and s as they are more natural units, i.e. redis_max_memory_mb=5000 is much more readable. Or even gb.

@ericl
Copy link
Contributor

ericl commented Dec 25, 2018

Let's make sure this is merged after #3558 since they will hugely conflict.

@AmplabJenkins
Copy link

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

@atumanov
Copy link
Contributor

atumanov commented Dec 25, 2018

Yes, I would very much like units to be part of all argument names to make it explicit. Regarding the exact unit abbreviations, there's a standard for this: SI units (le Système International):
https://en.wikipedia.org/wiki/International_System_of_Units
I think we should make our unit abbreviations compatible with the "unit symbols" chosen for SI units. For example, s (seconds), ms (milliseconds). They standardize power of 10 prefixes as well:
https://en.wikipedia.org/wiki/International_System_of_Units#Prefixes
Regarding the comment about following the Python convention -- I think an international standard should supersede any de facto convention. We can lead that convention by example :-)

@robertnishihara
Copy link
Collaborator Author

Replacing with #3666.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants