-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow address
instead of redis_address
#5412
Conversation
Test PASSed. |
else: | ||
ray.init( | ||
redis_address=args.redis_address, | ||
address=args.ray_address, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also change all of the examples in tune
and rllib
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm purposely not changing them since I don't want users to start using it on old versions of Ray. We can add it to the docs in a couple months.
I think the exception you want to raise can also be an importerror just
with a modified message :)
…On Fri, Aug 9, 2019 at 4:43 PM Robert Nishihara ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In python/ray/scripts/scripts.py
<#5412 (comment)>:
> plasma_store_socket_name, raylet_socket_name, temp_dir, include_java,
java_worker_options, load_code_from_local, internal_config):
# Convert hostnames to numerical IP address.
if node_ip_address is not None:
node_ip_address = services.address_to_ip(node_ip_address)
if redis_address is not None:
redis_address = services.address_to_ip(redis_address)
+ if address:
+ if redis_address:
+ logger.warning(
+ "You should specify address instead of redis_address.")
This can just be an exception instead of a warning, right? Doing so won't
break any existing workloads.
------------------------------
In python/ray/services.py
<#5412 (comment)>:
> @@ -93,6 +93,34 @@ def include_java_from_redis(redis_client):
return redis_client.get("INCLUDE_JAVA") == b"1"
+def find_redis_address_or_die():
+ import psutil
since psutil is not a hard dependency for Ray, can we add something like
try:
import psutilexcept ImportError:
# What time of exception should this be??????????????????????????
raise Exception("In order to automatically detect the Ray cluster, you "
"must install psutil. You can do this with 'pip install psutil'. "
"If that doesn't work, try using Anaconda Python and doing "
"'conda install psutil'.")
------------------------------
In python/ray/worker.py
<#5412 (comment)>:
> @@ -1376,6 +1378,14 @@ def init(redis_address=None,
arguments is passed in.
"""
+ if address:
+ if redis_address:
+ logger.warning(
+ "You should specify address instead of redis_address.")
can be exception
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5412?email_source=notifications&email_token=ABCRZZKXBU5C2TMJI2FDE5TQDX6JPA5CNFSM4IKPSAKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBF2EAQ#pullrequestreview-273392130>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABCRZZODBRXLPG5YDDFGUYLQDX6JPANCNFSM4IKPSAKA>
.
|
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me assuming tests pass.
Test PASSed. |
In anticipation of deprecating redis_address, add address which is just an alias of redis_address. In a couple releases, we can remove uses of redis_address from the documentation as well.
This also allows --address=auto which will auto-find any local redises.