Skip to content

[rllib] Remove need to pass around registry#2250

Merged
ericl merged 11 commits intoray-project:masterfrom
ericl:remove-registry
Jun 20, 2018
Merged

[rllib] Remove need to pass around registry#2250
ericl merged 11 commits intoray-project:masterfrom
ericl:remove-registry

Conversation

@ericl
Copy link
Contributor

@ericl ericl commented Jun 13, 2018

What do these changes do?

Avoid the need to pass the dict of registered objects around by fetching them from global redis.

@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/6056/
Test FAILed.

@robertnishihara
Copy link
Collaborator

I'm very skeptical of this change. RLlib should stick to using Ray APIs. Relying on implementation details like this will make it hard to change the implementation later on.

Also, using out-of-band mechanisms like this will compromise properties like fault tolerance.

@ericl
Copy link
Contributor Author

ericl commented Jun 14, 2018

We're not tied to any particular implementation here -- the requirement is that we support a global registry. Right now it uses redis, like named actors, but we can easily switch it to use the object store or some other mechanism in the future.

@ericl ericl changed the title [rllib] Remove need to pass around registry by using global redis [rllib] Remove need to pass around registry Jun 16, 2018
@ericl
Copy link
Contributor Author

ericl commented Jun 16, 2018

I updated this to refactor both this and named actors to use an internal KV interface.

@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/6081/
Test FAILed.

@ray-project ray-project deleted a comment Jun 18, 2018
@robertnishihara
Copy link
Collaborator

@ericl that seems like a good approach for now.

Can you say why you introduced the post_init_hooks? What is that needed for and how were you solving that problem before?

@ericl
Copy link
Contributor Author

ericl commented Jun 18, 2018 via email

Copy link
Collaborator

@robertnishihara robertnishihara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me assuming tests pass.

@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/6106/
Test FAILed.

@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/6109/
Test FAILed.

@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/6116/
Test FAILed.


from types import FunctionType

import cloudpickle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloudpickle technically isn't a dependency of Ray (e.g., it is not in our setup.py), so users may not have it installed.

We have our own cloudpickle which you can import with e.g., import ray.cloudpickle as pickle. The point of doing this is that it's very easy to have a cloudpickle version mismatch on different machines, but if we ship our own then we don't run into this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@richardliaw
Copy link
Contributor

Lint and a couple tests are failing on Travis...

@AmplabJenkins
Copy link

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

@ericl ericl merged commit 30f7c08 into ray-project:master Jun 20, 2018
royf added a commit to royf/ray that referenced this pull request Jun 22, 2018
* 'master' of https://github.com/ray-project/ray: (157 commits)
  Fix build failure while using make -j1. Issue 2257 (ray-project#2279)
  Cast locator with index type (ray-project#2274)
  fixing zero length partitions (ray-project#2237)
  Make actor handles work in Python mode. (ray-project#2283)
  [xray] Add error table and push error messages to driver through node manager. (ray-project#2256)
  addressing comments (ray-project#2210)
  Re-enable some actor tests. (ray-project#2276)
  Experimental: enable automatic GCS flushing with configurable policy. (ray-project#2266)
  [xray] Sets good object manager defaults. (ray-project#2255)
  [tune] Update Trainable doc to expose interface (ray-project#2272)
  [rllib] Add a simple REST policy server and client example (ray-project#2232)
  [asv] Pushing to s3 (ray-project#2246)
  [rllib] Remove need to pass around registry (ray-project#2250)
  Support multiple availability zones in AWS (fix ray-project#2177) (ray-project#2254)
  [rllib] Add squash_to_range model option (ray-project#2239)
  Mitigate randomly building failure: adding gen_local_scheduler_fbs to raylet lib. (ray-project#2271)
  [rllib] Refactor Multi-GPU for PPO (ray-project#1646)
  [rllib] Envs for vectorized execution, async execution, and policy serving (ray-project#2170)
  [Dataframe] Change pandas and ray.dataframe imports (ray-project#1942)
  [Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) (ray-project#2245)
  ...
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