Add RayParams to refactor the parameters used by ray python. #3558

guoyuhong · 2018-12-17T15:45:07Z

What do these changes do?

Current ray parameters appears in multiple places. Therefore, many same parameters are passed through many functions. If we need to add a single parameter, we need to add this parameter to all the functions and document this parameter every time. It's verbose and error-prone.

In this PR, all the parameters are stored in one class. The developer can add one parameter once and use it in the ray function call stack deeply.

Related issue number

N/A

AmplabJenkins · 2018-12-17T17:22: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/10092/
Test FAILed.

AmplabJenkins · 2018-12-17T19:08:14Z

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

ericl · 2018-12-18T09:22:55Z

This is great. We can merge this as is, but should it actually be a proto so it can be used in c++ as well?

guoyuhong · 2018-12-18T10:03:44Z

@ericl I don't really understand how do we use it c++.

AmplabJenkins · 2018-12-18T11:34:12Z

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

raulchen

Thanks, I left a few small comments. Will take a closer take later.

python/ray/params.py

AmplabJenkins · 2018-12-19T20:10:12Z

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

richardliaw · 2018-12-20T05:08:16Z

python/ray/params.py

+                 temp_dir=None,
+                 include_log_monitor=None,
+                 _internal_config=None,
+                 autoscaling_config=None):


_internal_config should come last?

richardliaw · 2018-12-20T05:09:54Z

python/ray/params.py

+            if (hasattr(self, arg)):
+                setattr(self, arg, kwargs[arg])
+            else:
+                raise Exception("Invalid RayParams parameter in"


raise ValueError?

richardliaw · 2018-12-20T05:10:21Z

python/ray/scripts/scripts.py

-            temp_dir=temp_dir,
-            _internal_config=internal_config)
+                    .format(ray_params.node_ip_address))
+        ray_params.redis_port = redis_port


should we be using the apply_config method here?

richardliaw · 2018-12-20T05:39:52Z

python/ray/services.py

+            following parameters could be checked: redis_address,
+            node_ip_address, worker_path, resources, object_manager_ports,
+            node_manager_ports, redis_password
+        index (int): the index used in resources, object_manager_ports,


what is the index of specifically? does it indicate Raylet index? Would be ideal to have this documented.

richardliaw · 2018-12-20T05:50:46Z

python/ray/services.py

-        worker_path (str): The path of the source code that will be run by the
-            worker.
+        ray_params (ray.params.RayParams): The RayParams instance. The
+            following parameters could be checked: address_info,


hm I feel like these docs aren't particularly helpful. If I wanted to know which attributes were going to be used, I can just check the code.

One thing that would be good is to note specifically how the params instance is updated (which default values are being set, etc).

richardliaw · 2018-12-20T05:53:22Z

python/ray/services.py

+    config = json.loads(
+        ray_params._internal_config) if ray_params._internal_config else None
+
+    if ray_params.include_log_monitor is None:


this pattern appears pretty often; would it makes sense to have some method that batches this?

I add a function called apply_when_none.

richardliaw · 2018-12-20T05:54:17Z

python/ray/services.py


    # Start any raylets that do not exist yet.
-    for i in range(len(raylet_socket_names), num_local_schedulers):
+    for i in range(len(raylet_socket_names), ray_params.num_local_schedulers):


can we use a better variable here? like worker index or something

richardliaw · 2018-12-20T06:01:49Z

python/ray/test/cluster_utils.py

            "object_store_memory": 100 * (2**20)  # 100 MB
        }
        node_kwargs.update(override_kwargs)
+        ray_params = RayParams()


why not just (

ray_params = RayParams( node_ip_address=services.get_node_ip_address(), **node_kwargs)

guoyuhong · 2018-12-25T03:02:06Z

@ericl Thanks for the approve. I am working on other 2 PRs. I will come back and fix the comments soon.

…ythonParam

AmplabJenkins · 2018-12-25T18:22: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/10397/
Test FAILed.

AmplabJenkins · 2018-12-25T19:15:38Z

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

AmplabJenkins · 2018-12-27T09:47:13Z

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

guoyuhong · 2018-12-27T10:03:13Z

For mnist_example.py failure, it is not caused by this PR. I have submitted another PR: #3648 . However, there is still failure in multi_node_docker_test.py. I will take a look later.

AmplabJenkins · 2018-12-27T11:00:57Z

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

AmplabJenkins · 2018-12-27T12:39:05Z

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

AmplabJenkins · 2018-12-27T12:52:26Z

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

AmplabJenkins · 2018-12-27T13:16: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/10451/
Test FAILed.

AmplabJenkins · 2018-12-27T15:31:53Z

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

AmplabJenkins · 2018-12-27T15:35:30Z

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

AmplabJenkins · 2018-12-27T17:49:35Z

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

AmplabJenkins · 2018-12-27T18:32:00Z

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

guoyuhong · 2018-12-28T03:31:12Z

This Jenkins test passed, so I reverted the temporary Jenkins fix and kept the related commit.

AmplabJenkins · 2018-12-28T04:38:27Z

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

robertnishihara · 2018-12-29T06:38:04Z

python/ray/parameter.py

+    def update_if_absent(self, **kwargs):
+        """Update the settings when the target fields are None.
+
+        Attributes:


Should be Args:

robertnishihara · 2018-12-29T06:38:17Z

python/ray/parameter.py

+    def update(self, **kwargs):
+        """Update the settings according to the keyword arguments.
+
+        Attributes:


robertnishihara · 2018-12-29T06:45:25Z

@guoyuhong this looks great, thanks for this PR!

robertnishihara · 2018-12-29T06:51:18Z

Looking over these changes made me think:

We should really deprecate this ray.worker._init stuff and instead only use ray.test.cluster_utils to simulate multiple nodes on a single machine.
Instead of having ray.services.start_ray_head and ray.services.start_ray_node, we should just have start_ray_node and initialize_cluster (not sure if that's the right term), where initialize_cluster just starts all of the stuff that only gets created once per cluster (e.g., redis shards, monitor.py, monitor.cc, the web UI). There's no real reason for these processes to all be tied to a "head" node. They could live on different machines or multiple machines.

In subsequent PRs..

AmplabJenkins · 2018-12-29T09:10:00Z

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

guoyuhong force-pushed the rayPythonParam branch from 52171be to 97d1527 Compare December 18, 2018 09:21

Yuhong Guo added 3 commits December 19, 2018 16:02

Add RayParams

c85312e

Lint

7697778

Use RayParams in start_raylet

78d0ad0

guoyuhong requested review from ericl, raulchen, richardliaw and robertnishihara December 19, 2018 08:43

Remove unused parameters.

2dc01d6

raulchen reviewed Dec 19, 2018

View reviewed changes

python/ray/params.py Outdated Show resolved Hide resolved

python/ray/params.py Outdated Show resolved Hide resolved

Yuhong Guo added 2 commits December 19, 2018 17:53

Change connect to use RayParams

f5eaae8

Fix Raul's comment

b3848d1

guoyuhong force-pushed the rayPythonParam branch from 97d1527 to b3848d1 Compare December 19, 2018 09:57

Fix Travis failure.

a244ed1

richardliaw reviewed Dec 20, 2018

View reviewed changes

ericl approved these changes Dec 25, 2018

View reviewed changes

ericl mentioned this pull request Dec 25, 2018

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

Closed

Yuhong Guo added 4 commits December 25, 2018 23:46

Address comment

d6ae2e6

Merge branch 'master' of https://github.com/ray-project/ray into rayP…

aa619fb

…ythonParam

Move params.py to parameter.py

4090ea6

refine the document

c38de33

guoyuhong force-pushed the rayPythonParam branch from 13d4dd7 to 82dd26a Compare December 27, 2018 11:43

guoyuhong force-pushed the rayPythonParam branch 2 times, most recently from 7bb3870 to 12f444f Compare December 27, 2018 15:08

guoyuhong force-pushed the rayPythonParam branch from 12f444f to 2facd3f Compare December 27, 2018 15:41

Fix start none-head node from ray start

414d07d

guoyuhong changed the title ~~Add RayParams to refactor the parameters used by ray python.~~ [WIP] Add RayParams to refactor the parameters used by ray python. Dec 27, 2018

guoyuhong force-pushed the rayPythonParam branch from 2facd3f to d0347ec Compare December 27, 2018 16:19

guoyuhong force-pushed the rayPythonParam branch from d0347ec to 414d07d Compare December 28, 2018 01:33

guoyuhong changed the title ~~[WIP] Add RayParams to refactor the parameters used by ray python.~~ Add RayParams to refactor the parameters used by ray python. Dec 28, 2018

robertnishihara reviewed Dec 29, 2018

View reviewed changes

Update parameter.py

325d4a1

robertnishihara approved these changes Dec 29, 2018

View reviewed changes

raulchen merged commit c9b8ecc into ray-project:master Dec 29, 2018

guoyuhong deleted the rayPythonParam branch January 25, 2019 10:08

Add RayParams to refactor the parameters used by ray python. #3558

Add RayParams to refactor the parameters used by ray python. #3558

Uh oh!

Conversation

guoyuhong commented Dec 17, 2018

What do these changes do?

Related issue number

Uh oh!

AmplabJenkins commented Dec 17, 2018

Uh oh!

AmplabJenkins commented Dec 17, 2018

Uh oh!

ericl commented Dec 18, 2018

Uh oh!

guoyuhong commented Dec 18, 2018

Uh oh!

AmplabJenkins commented Dec 18, 2018

Uh oh!

raulchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AmplabJenkins commented Dec 19, 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!

guoyuhong commented Dec 25, 2018

Uh oh!

AmplabJenkins commented Dec 25, 2018

Uh oh!

AmplabJenkins commented Dec 25, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

guoyuhong commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

AmplabJenkins commented Dec 27, 2018

Uh oh!

guoyuhong commented Dec 28, 2018

Uh oh!

AmplabJenkins commented Dec 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara commented Dec 29, 2018

Uh oh!

robertnishihara commented Dec 29, 2018