Support metadata for passing by value task arguments by kfstorm · Pull Request #5527 · ray-project/ray

kfstorm · 2019-08-24T08:51:51Z

Why are these changes needed?

This feature is needed by direct actor call, as it only accepts passing by value arguments. When passing byte array between Java and Python, we need metadata in task arguments to make passing byte array as value possible. Hence, this PR is a prerequisite of #5504.

What do these changes do?

Add matadata field for TaskArg in common.proto.

Update the object serialization code of both Java and Python.

Extract serialize and deserialzie methods of ObjectStore to a new class ObjectSerializer.

Related issue number

#5029

Linter

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

…adata

AmplabJenkins · 2019-08-24T12:15:52Z

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

AmplabJenkins · 2019-08-24T12:22:16Z

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

AmplabJenkins · 2019-08-24T12:36:06Z

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

AmplabJenkins · 2019-08-24T14:34:28Z

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

AmplabJenkins · 2019-08-24T15:08:50Z

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

zhijunfu

Thanks, I left a few comments.

zhijunfu · 2019-08-25T06:53:52Z

src/ray/core_worker/common.h

  /// Data of the argument, if passed by value, otherwise nullptr.
  const std::shared_ptr<Buffer> data_;
+  /// Metadata of the argument, if passed by value, otherwise nullptr.
+  const std::shared_ptr<Buffer> metadata_;


Is it possible to reuse the existing RayObject to present the by-value param in the TaskArg? That way it should be more clear and more consistent.

also for the data_ and metadata_, could we maintain the following invariants to make the code simpler?

one of them must be non-null;

if it's not null, then the buffer must be valid, that is, the size of the buffer must be greater than 0. Thus can only reply on the validity of the pointer itself, without need to further check its size.

I'm not sure whether forbidding empty buffers is a better choice. Maybe we can add two more functions, HasData and HasMetadata.

Do you have any concerns about processing empty buffers?

Just found that RayObject already has the HasMetadata function. I think it's better to ensure that data_ cannot be nullptr yet is allowed to be empty. As for metadata_, we don't set any restrictions.

Could you explain a bit more on why it's better to ensure data_ cannot be nullptr?

I think for error conditions data_ could nullptr?

I preferred to not have empty buffers because:

it's more intuitive and more clear conceptually. When it's not a nullptr, it's always valid.

in most cases we only have data_ but not metadata_, and we can save a memory allocation/free in this case.

I'm a bit concerned that having empty buffers might be error-prone. e.g. it's possible that people might only check the validity of the pointer, but don't check if buffer size is empty, then it might cause some bugs.

I've changed my mind. I think we should ensure that the same set of rules apply to both data_ and metadata_ to avoid confusion. I tried to add checks in the constructor of RayObject to avoid empty buffers, but it just made it worse. Tests failed due to the checks and I can't find out the source of empty buffers. And it cost me hours.

I suggest to keep it as is. We can add more checks after we have an agreement on nullptr vs. empty buffer.

I see. Then how about just keep it for now, and leave a todo that we can revisit this later.

Todo added.

I've added checks to avoid empty buffers when constructing a RayObject instance.

AmplabJenkins · 2019-08-26T10:17:44Z

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

AmplabJenkins · 2019-08-26T17:41:43Z

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

AmplabJenkins · 2019-08-27T09:35:34Z

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

edoakes

Mostly looks good, just please be very clear about the semantics of the RayObject as it is getting more and more complicated.

edoakes · 2019-08-30T16:21:11Z

src/ray/core_worker/common.h

+    }
+
+    RAY_CHECK((data_ && data_->Size()) || (metadata_ && metadata_->Size()))
+        << "Data and metadat cannot be both empty.";


Suggested change

<< "Data and metadat cannot be both empty.";

<< "Data and metadata cannot both empty.";

edoakes · 2019-08-30T16:24:39Z

src/ray/core_worker/common.h

+  }
+
+  /// Whether this object has metadata.
+  bool HasMetadata() const { return metadata_ != nullptr; }


If the invariant is that the object has either data_ or metadata_, please also add a HasData method.

edoakes · 2019-08-30T16:25:15Z

src/ray/core_worker/common.h

 };

+/// Binary representation of ray object.
+class RayObject {


Please comment that only one of data_ or metadata_ must be present.

edoakes · 2019-08-30T16:27:43Z

src/ray/core_worker/common.h

+      // If this object is required to hold a copy of the data,
+      // make a copy if the passed in buffers don't already have a copy.
+      if (data_ && !data_->OwnsData()) {
+        data_ = std::make_shared<LocalMemoryBuffer>(data_->Data(), data_->Size(), true);


Suggested change

data_ = std::make_shared<LocalMemoryBuffer>(data_->Data(), data_->Size(), true);

data_ = std::make_shared<LocalMemoryBuffer>(data_->Data(), data_->Size(), /*flag_name=*/true);

Same for below.

edoakes · 2019-08-30T16:27:50Z

src/ray/core_worker/common.h

+  RayObject(const std::shared_ptr<Buffer> &data,
+            const std::shared_ptr<Buffer> &metadata = nullptr, bool copy_data = false)
+      : data_(data), metadata_(metadata), has_data_copy_(copy_data) {
+    if (has_data_copy_) {


I know that this PR didn't introduce this flag, but can you please explain why we need the has_data_copy_ flag and its semantics in the header comment?

What happens when it's set, when should the caller set it, what could go wrong if it isn't set when it should be, etc.

@zhijunfu Can you help answer the questions?

This is related to Buffer definition, please refer to src/ray/common/buffer.h.

For example, by default when you initialize a LocalMemoryBuffer with a data pointer and a length, it just assigns the pointer and length, but doesn't copy the data content, this is for performance reasons, but in this case the buffer cannot ensure data validity, it instead relies the lifetime passed in data pointer.

This is fine for most cases - for example when you put an object into store, you create a temporary RayObject and don't want to do an extra copy. But in some cases you do want to always hold a valid data - for example, memory store uses RayObject to represent objects, in this case you actually want the object data to remain valid after user puts it into store.

I see, that seems reasonable but we need to document it better (basically, the content of this comment). The comment in src/ray/common/buffer.h also doesn't have much more information.

edoakes · 2019-08-30T16:28:29Z

src/ray/core_worker/common.h

  const std::vector<std::string> function_descriptor;
 };

+/// Binary representation of ray object.


Make this comment more useful or remove it

Please check the updated comment.

edoakes · 2019-08-30T16:28:42Z

src/ray/core_worker/common.h

+  bool HasMetadata() const { return metadata_ != nullptr; }
+
+ private:
+  // TODO (kfstorm): Currently both a null pointer and a pointer points to a buffer with


Please fix this before merging. Shouldn't be too hard and can easily get lost afterwards.

As I said in the discussion with @zhijunfu, I tried it, but a lot of tests failed and I can't find out all the sources of empty buffers after hours. You may continue to discuss it on that thread.

edoakes · 2019-08-30T16:29:06Z

src/ray/core_worker/common.h

+  // zero size means empty data/metadata. We'd better pick one and treat the other as
+  // invalid.
+
+  /// Data of the ray object.


edoakes · 2019-08-30T16:29:10Z

src/ray/core_worker/common.h

+
+  /// Data of the ray object.
+  std::shared_ptr<Buffer> data_;
+  /// Metadata of the ray object.


src/ray/core_worker/common.h

raulchen · 2019-09-02T06:06:12Z

java/runtime/src/main/java/org/ray/runtime/object/NativeRayObject.java


  public NativeRayObject(byte[] data, byte[] metadata) {
+    Preconditions.checkNotNull(data);
+    Preconditions.checkNotNull(metadata);


should metadata be a zero-length array when there's no metadata? If so, we can document this.

Oops. This is something I didn't keep consistent with the C++ code. I'll make null pointer and zero-length array both valid. If you prefer to allow only one of them, we can discuss it in the first thread.

raulchen · 2019-09-02T06:07:22Z

java/runtime/src/main/java/org/ray/runtime/object/ObjectSerializer.java

+    byte[] data = nativeRayObject.data;
+
+    // If meta is not null, deserialize the object from meta.
+    if (meta != null && meta.length > 0) {


meta != null isn't needed now?

As I'll make null pointer valid, it is needed.

raulchen · 2019-09-02T06:10:18Z

java/runtime/src/main/java/org/ray/runtime/task/FunctionArg.java

+  public final NativeRayObject value;

-  private FunctionArg(ObjectId id, byte[] data) {
+  private FunctionArg(ObjectId id, NativeRayObject value) {


check (id == null) != (value == null)

This is a private constructor, but OK.

Adding this check can help catch bugs eariler

raulchen · 2019-09-02T06:20:19Z

src/ray/core_worker/common.h

 };

+/// Binary representation of ray object.
+class RayObject {


better put this RayObject class in src/common/, because raylet may also need to use this.

…adata

AmplabJenkins · 2019-09-02T14:58:53Z

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

raulchen

LGTM. can you fix the conflict? thx

…adata

AmplabJenkins · 2019-09-06T13:31:02Z

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

kfstorm · 2019-09-06T15:12:32Z

@raulchen Conflicts solved.

src/ray/common/ray_object.h

edoakes · 2019-09-07T04:42:01Z

src/ray/common/ray_object.h

+  /// \param[in] data Data of the ray object.
+  /// \param[in] metadata Metadata of the ray object.
+  /// \param[in] copy_data Whether this class should hold a copy of data.
+  RayObject(const std::shared_ptr<Buffer> &data, const std::shared_ptr<Buffer> &metadata,


Please add a comment explaining the has_data_copy flag and why it's necessary.

edoakes · 2019-09-07T04:42:28Z

src/ray/common/ray_object.h

+  bool HasMetadata() const { return metadata_ != nullptr && metadata_->Size() > 0; }
+
+ private:
+  // TODO (kfstorm): Currently both a null pointer and a pointer points to a buffer with


Please address this before we merge.

I've added checks to avoid empty buffers.

src/ray/common/task/task_spec.cc

edoakes · 2019-09-07T04:44:57Z

src/ray/core_worker/common.h

-    RAY_CHECK(data_ != nullptr) << "This argument isn't passed by value.";
-    return data_;
+  const RayObject &GetValue() const {
+    RAY_CHECK(value_ != nullptr) << "This argument isn't passed by value.";


Let's try not to use RAY_CHECK in the core worker as it causes an Abort trap: 6, making it very inconvenient to debug (and horrible for users if it happens somehow). Better to use RAY_RETURN_NO_OK.

How is that possible? RAY_RETURN_NOT_OK is used for functions with the return type of Status.

This RAY_CHECK should not get triggered by user code. If it fails, it must be a bug in ray codebase.

Yeah, I think it's better to use RAY_CHECK to catch system bugs.

Oops, sorry - of course you can't use RAY_RETURN_NOT_OK. Actually, isn't it better to define separate classes for by value and by reference arguments instead of having these checks? That way the compiler can catch these bugs for us...

src/ray/core_worker/common.h

src/ray/core_worker/test/core_worker_test.cc

AmplabJenkins · 2019-09-07T12:36:35Z

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

AmplabJenkins · 2019-09-07T14:48:28Z

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

raulchen · 2019-09-07T14:49:51Z

@edoakes All comments should be addressed. Please let us know if you have other comments. I'll postpone merging this PR until tomorrow.

…adata

AmplabJenkins · 2019-09-07T19:17:34Z

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

edoakes · 2019-09-07T21:29:40Z

LGTM aside from my comment about defining separate structs/classes for by value and by reference args. You can push that to another PR if this one is blocking things, but seems much better than RAY_CHECKing the fields.

raulchen · 2019-09-08T03:07:01Z

@edoakes By-value and by-reference arguments need different methods (GetReference and GetValue). If we use 2 separate classes, we'll need to check type and downcast the pointer, e.g., reinterpret_cast<ByValueTaskArg>(task_arg)->GetValue(). This looks more inconvenient.

I'll merge this PR first, as the refactor may need more discussions.

addressed

Enable metadata for passing by value

fd8c082

kfstorm requested review from jovany-wang, raulchen, robertnishihara and zhijunfu August 24, 2019 08:51

kfstorm added 5 commits August 24, 2019 16:59

Fix core worker test

2a39d58

Merge remote-tracking branch 'upstream/master' into pass_by_value_met…

487c13e

…adata

Fix typo

ae75b5e

lint

be4eb1e

clean

d3c7c90

zhijunfu reviewed Aug 25, 2019

View reviewed changes

Simplify code

e7d4753

Fix build

06661ad

Add todo

f99dae6

kfstorm mentioned this pull request Aug 28, 2019

Fix skipped Java test cases in direct actor call mode #5559

Closed

edoakes self-requested a review August 30, 2019 06:15

edoakes requested changes Aug 30, 2019

View reviewed changes

raulchen reviewed Sep 2, 2019

View reviewed changes

kfstorm added 2 commits September 2, 2019 19:10

Address comments

9f492de

Merge remote-tracking branch 'upstream/master' into pass_by_value_met…

0bd5697

…adata

kfstorm requested review from edoakes and raulchen September 2, 2019 11:13

raulchen approved these changes Sep 6, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pass_by_value_met…

c8597dd

…adata

edoakes previously requested changes Sep 7, 2019

View reviewed changes

kfstorm added 2 commits September 7, 2019 16:31

Address comments

6ce3429

Avoid empty buffer in RayObject

b62f4c3

Merge remote-tracking branch 'upstream/master' into pass_by_value_met…

bcb6fb1

…adata

raulchen merged commit d8f5804 into ray-project:master Sep 8, 2019

raulchen deleted the pass_by_value_metadata branch September 8, 2019 03:11

	<< "Data and metadat cannot be both empty.";
	<< "Data and metadata cannot both empty.";

	data_ = std::make_shared<LocalMemoryBuffer>(data_->Data(), data_->Size(), true);
	data_ = std::make_shared<LocalMemoryBuffer>(data_->Data(), data_->Size(), /flag_name=/true);

Conversation

kfstorm commented Aug 24, 2019

Why are these changes needed?

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Aug 24, 2019

Uh oh!

AmplabJenkins commented Aug 24, 2019

Uh oh!

AmplabJenkins commented Aug 24, 2019

Uh oh!

AmplabJenkins commented Aug 24, 2019

Uh oh!

AmplabJenkins commented Aug 24, 2019

Uh oh!

zhijunfu left a comment

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!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Aug 26, 2019

Uh oh!

AmplabJenkins commented Aug 26, 2019

Uh oh!

AmplabJenkins commented Aug 27, 2019

Uh oh!

edoakes left a comment

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!

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!

kfstorm Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kfstorm Sep 2, 2019 •

edited

Loading