Skip to content
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

[Client] Provide common ObjectRefType #14042

Closed
devin-petersohn opened this issue Feb 10, 2021 · 9 comments · Fixed by #16110
Closed

[Client] Provide common ObjectRefType #14042

devin-petersohn opened this issue Feb 10, 2021 · 9 comments · Fixed by #16110
Labels
enhancement Request for new feature and/or capability P1 Issue that should be fixed within a few weeks
Milestone

Comments

@devin-petersohn
Copy link
Member

Describe your feature request

We do a lot of defensive type checking in Modin when using Ray to make sure that things are working. It would be very convenient if ClientObjectRef extended ObjectRef so that assertions like assert(isinstance(obj, ray.ObjectRef)) would still work without needing to handle the Client object separately.

@devin-petersohn devin-petersohn added the enhancement Request for new feature and/or capability label Feb 10, 2021
@devin-petersohn
Copy link
Member Author

Related to modin-project/modin#2688

@edoakes edoakes added the triage Needs triage (eg: priority, bug/not-bug, and owning component) label Feb 10, 2021
@edoakes edoakes added this to the Ray Client milestone Feb 10, 2021
@ericl ericl added P2 Important issue, but not time-critical and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Feb 10, 2021
@Bhavya6187
Copy link

Bhavya6187 commented Mar 4, 2021

Hey guys, can you verify if this example works with current nightlies - https://docs.ray.io/en/master/modin/index.html#modin-with-the-ray-client?

I run into this issue when I try to load Modin csv's or parquets using ray client. Here's the corresponding issue on Modin's side - modin-project/modin#2688

@ericl ericl assigned AmeerHajAli and ericl and unassigned barakmich and AmeerHajAli Mar 8, 2021
@ericl ericl added P1 Issue that should be fixed within a few weeks and removed P2 Important issue, but not time-critical labels Mar 8, 2021
@ericl
Copy link
Contributor

ericl commented Mar 8, 2021

This turns out to be quite tricky to do, since ObjectRef is a native cdef that depends on the C++ runtime. Would the following workaround work?

ObjectRefType = Union[ray.ObjectRef, ClientObjectRef]

Then you can type check around ObjectRefType instead of ObjectRef directly.

@devin-petersohn
Copy link
Member Author

devin-petersohn commented Mar 9, 2021

Yes, that will work for our purposes. Something like:

from ray import ObjectRefType

assert isinstance(myobj, ObjectRefType)

edit: Did you mean from Ray or within our code to add this line?

@ericl
Copy link
Contributor

ericl commented Mar 9, 2021

@devin-petersohn it seems there are some cdef limitations on inheriting from an abstract base class. Would one of the following options work for you in Modin instead?

assert is_object_ref_type(myobj)
assert isinstance(myobj, ObjectRefType.__args__)

In the short term, I'd recommend implementing a custom is_object_ref_type function in Modin that checks if it's either ObjectRef or ClientObjectRef to unblock Ray client users.

In Ray 1.3 we can add either is_object_ref_type as a native API in Ray or the ObjectRefType Union type as above. We could also probably refactor the ObjectRef implementation so a real abstract base class is possible, but this would be a quite large change.

@devin-petersohn
Copy link
Member Author

Sounds good @ericl, we'll do that.

@ericl ericl changed the title [Client] Make ClientObjectRef extend ObjectRef for the purposes of type checking [Client] Provide common ObjectRefType Mar 10, 2021
@ericl ericl removed their assignment Mar 10, 2021
@ericl ericl modified the milestones: Ray Client, Core Bugs Mar 10, 2021
@ericl
Copy link
Contributor

ericl commented Mar 10, 2021

Moving to core bugs for tracking

@pcmoritz
Copy link
Contributor

Would it be possible to only have a single ObjectRef type for both client and the rest of Ray? Certainly from the API side that would be the simplest (no need for is_object_ref_type etc)?

If the implementation requires it, the ObjectRef could have an internal distinction if it is a client ref or a Ray ref.

@ericl
Copy link
Contributor

ericl commented Mar 10, 2021

It's a possibility, we'd have to look into how much refactoring is needed, since ObjectRef is a Cython type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for new feature and/or capability P1 Issue that should be fixed within a few weeks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants