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

[Core worker] Add metadata support in object interface #5031

Merged
merged 8 commits into from
Jun 28, 2019

Conversation

kfstorm
Copy link
Member

@kfstorm kfstorm commented Jun 25, 2019

What do these changes do?

Plasma store supports store both metadata and data for an object ID. But current core worker object interface can only put and get data. Put operation inserts null metadata, and get operation drops metadata. This PR modified the interfaces and wrapped both metadata and data into a struct.

Related issue number

#4850

Linter

  • I've run scripts/format.sh to lint the changes in this PR.

@kfstorm kfstorm changed the title Add metadata support in core worker object interface [Core worker] Add metadata support in core worker object interface Jun 25, 2019
@kfstorm kfstorm changed the title [Core worker] Add metadata support in core worker object interface [Core worker] Add metadata support in object interface Jun 25, 2019
@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/14861/
Test FAILed.

unready.erase(object_id);
// TODO (kfstorm): metadata should be structured.
std::string metadata = object_buffers[i].metadata->ToString();
for (auto &error_type : EnumValuesErrorType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing the definition for EnumValuesErrorType?

Copy link
Member Author

Choose a reason for hiding this comment

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

EnumValuesErrorType is generated by flatbuffer. PS: This part of code should not be in this PR. It will come with Java worker changes. I'll remove it.

@@ -8,6 +8,30 @@

namespace ray {

/// Object value in ray system.
class RayObjectValue {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about RayObject? or ObjectValue might be also ok

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also considering store object reference in this class in the future, like store OBJECT_ID in metadata field and raw bytes of object ID in data field. Then TaskArg could be totally replaced by this structure. So *Value is not appropriate. Maybe RayObject is good enough. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

RayObject sounds good to me.
For storing object ID into this class, is there any requirement for this? In ray we have object IDs and objects, I'm thinking RayObject or ObjectValue is still talking about objects, not the ids.

Copy link
Member Author

Choose a reason for hiding this comment

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

One scenario for storing ID into this class is, cross-language invocation against a remote function which returns an object ID.

Suppose we have a Java remote function with signature RayObject<Integer> foo(), and we want to call foo in Java like this: RayObject<RayObject<Integer>> result = Ray.call(SomeClass::foo); Integer intResult = result.get().get();. This works because the return value of foo is serialized by Java and stored in object store, and the caller can deserialize it and get the wrapped Integer type result.

However, if we define foo in Python, then the return value of foo will be serialized by Python. When we try to get the result of foo from Java, deserialization fails because Java can't understand the binary generated by Python.

By encoding object ID into this class, with metadata of OJBECT_ID and data of bytes of the object ID, we have a language-independent way about representing and storing of object ID. We want to deal with actor handle and exception exactly like this to make them language-independent too.

In this PR, this class is just a handful data structure to make the object interface more clean. But it can also be used to help solve cross-language limitations. Now for cross-language we can only call Python from Java, not reversed. And the only acceptable parameter and return type of Python function is byte array. We want to change that.

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

@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/14934/
Test PASSed.

@pcmoritz pcmoritz self-assigned this Jun 27, 2019
@pcmoritz
Copy link
Contributor

LGTM, can be merged after the RayObjectValue -> RayObject change

@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-Perf-Integration-PRB/1388/
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/14950/
Test FAILed.

@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/14961/
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/14963/
Test PASSed.

@pcmoritz pcmoritz merged commit 4ccb7b0 into ray-project:master Jun 28, 2019
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