Add Redis port option to startup script #232
Conversation
| @@ -69,6 +78,8 @@ def check_no_existing_redis_clients(node_ip_address, redis_address): | |||
| address_info["redis_address"])) | |||
| else: | |||
| # Start Ray on a non-head node. | |||
| if args.redis_port: | |||
There was a problem hiding this comment.
Can we use if args.redis_port is not None for consistency?
| @@ -69,6 +78,8 @@ def check_no_existing_redis_clients(node_ip_address, redis_address): | |||
| address_info["redis_address"])) | |||
| else: | |||
| # Start Ray on a non-head node. | |||
| if args.redis_port: | |||
There was a problem hiding this comment.
Can we use if args.redis_port is not None for consistency?
| @@ -200,10 +200,14 @@ def start_redis(node_ip_address, num_retries=20, cleanup=True, redirect_output=F | |||
| assert os.path.isfile(redis_filepath) | |||
| assert os.path.isfile(redis_module) | |||
| counter = 0 | |||
| if port: | |||
There was a problem hiding this comment.
Can we use if port is not None for consistency?
| @@ -519,7 +537,7 @@ def start_ray_node(node_ip_address, | |||
| cleanup=cleanup, | |||
| redirect_output=redirect_output) | |||
|
|
|||
| def start_ray_local(address_info=None, | |||
| def start_ray_head(address_info=None, | |||
There was a problem hiding this comment.
the indentation in the lines below needs to be fixed
| @@ -519,7 +537,7 @@ def start_ray_node(node_ip_address, | |||
| cleanup=cleanup, | |||
| redirect_output=redirect_output) | |||
|
|
|||
| def start_ray_local(address_info=None, | |||
There was a problem hiding this comment.
This is also used when starting Ray on a single machine.
There was a problem hiding this comment.
Have updated worker.py
| @@ -370,7 +375,8 @@ def start_ray_processes(address_info=None, | |||
| worker_path=None, | |||
| cleanup=True, | |||
| redirect_output=False, | |||
| include_global_scheduler=False): | |||
| include_global_scheduler=False, | |||
| include_redis=False): | |||
There was a problem hiding this comment.
document this argument
| @@ -178,7 +178,7 @@ def wait_for_redis_to_start(redis_host, redis_port, num_retries=5): | |||
| if counter == num_retries: | |||
| raise Exception("Unable to connect to Redis. If the Redis instance is on a different machine, check that your firewall is configured properly.") | |||
|
|
|||
| def start_redis(node_ip_address, num_retries=20, cleanup=True, redirect_output=False): | |||
| def start_redis(node_ip_address, port=None, num_retries=20, cleanup=True, redirect_output=False): | |||
There was a problem hiding this comment.
for completeness, lets add documentation for "port"
| address_info["redis_address"] = redis_address | ||
| time.sleep(0.1) | ||
| else: | ||
| redis_host, redis_port = redis_address.split(":") |
There was a problem hiding this comment.
the current terminology is
blah_ip_address -> ip_address of blah without port,
blah_port -> port of blah
blah_address -> ip address and port of blah
I like blah_host better instead of blah_ip_address, but we should make it the same everywhere
| redirect_output=True) | ||
|
|
||
| if args.redis_port is not None: | ||
| address_info_in = {"redis_address": "{}:{}".format(node_ip_address, |
There was a problem hiding this comment.
I'd prefer to call it address_info instead of address_info_in. I'm aware the name is also used below but it is not a problem, as it is explicitly updated like so:
address_info = function(address_info)
robertnishihara
force-pushed
the
redisport
branch
3 times, most recently
from
1d1ee06
to
4c505a6
Compare
robertnishihara
force-pushed
the
redisport
branch
from
4c505a6
to
beabd4f
Compare
When scripting Ray startup we want to specify the port on which Redis will be started. This change introduced the
--redis-portoption to the Ray startup script.