Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic #6171

ericl · 2019-11-15T20:55:15Z

Why are these changes needed?

Related issue number

Closes #6264

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.

AmplabJenkins · 2019-11-15T20:56:21Z

Can one of the admins verify this patch?

AmplabJenkins · 2019-11-15T21:42:24Z

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

AmplabJenkins · 2019-11-16T04:30:08Z

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

AmplabJenkins · 2019-11-18T20:27:18Z

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

ericl · 2019-11-18T20:57:07Z

jenkins retest this please

AmplabJenkins · 2019-11-18T22:11: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/18570/
Test FAILed.

ericl · 2019-11-18T22:45:34Z

jenkins retest this please

AmplabJenkins · 2019-11-19T01:28:20Z

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

AmplabJenkins · 2019-11-21T20:22:29Z

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

AmplabJenkins · 2019-11-21T21:01:48Z

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

AmplabJenkins · 2019-11-21T23:05:19Z

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

AmplabJenkins · 2019-11-22T01:10:55Z

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

AmplabJenkins · 2019-11-22T03:16:40Z

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

AmplabJenkins · 2019-11-22T04:28:54Z

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

ericl · 2019-11-24T23:23:41Z

python/ray/tests/BUILD

+    name = "test_actor_direct",
+    size = "medium",
+    srcs = ["test_actor_direct.py", "test_actor.py"],
+    tags = ["exclusive", "manual"],


test_actor_load_balancing is flaky, so setting it to MANUAL for now

stephanie-wang · 2019-11-24T23:53:45Z

python/ray/tests/BUILD

    srcs = ["test_basic.py"],
    tags = ["exclusive"],
    deps = ["//:ray_lib"],
 )

 py_test(
-    name = "test_advanced",
-    size = "large",
-    srcs = ["test_advanced.py"],


Should this have been removed?

Oops, fixed.

stephanie-wang · 2019-11-24T23:56:14Z

src/ray/core_worker/transport/dependency_resolver.cc

@@ -42,7 +42,7 @@ void DoInlineObjectValue(const ObjectID &obj_id, std::shared_ptr<RayObject> valu
  RAY_CHECK(found) << "obj id " << obj_id << " not found";
 }

-void LocalDependencyResolver::ResolveDependencies(const TaskSpecification &task,


Why no more const?

Since this mutates the task, it is weird to call it const.

stephanie-wang · 2019-11-24T23:57:44Z

src/ray/core_worker/core_worker.cc

-  return raylet_client_->SubmitTask(task_spec);
+  // TODO(ekl) if we moved actor creation to use direct call tasks, then we won't
+  // need to manually resolve direct call args here.
+  resolver_->ResolveDependencies(task_spec, [this, task_spec]() {


Just wondering, was there a test that was failing with the old version of this?

Yeah, it causes rllib ES to hang, since we pass an object id to an actor constructor there.

AmplabJenkins · 2019-11-25T00:31: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/18845/
Test FAILed.

AmplabJenkins · 2019-11-25T00:52: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/18847/
Test FAILed.

AmplabJenkins · 2019-11-25T01:41: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/18849/
Test FAILed.

ericl · 2019-11-25T04:45:37Z

Interestingly, the RLlib test is failing with corrupted sample data returned.

ericl · 2019-11-25T05:24:26Z

src/ray/core_worker/core_worker.cc

@@ -873,7 +874,7 @@ Status CoreWorker::BuildArgsForExecutor(const TaskSpecification &task,
        metadata = std::make_shared<LocalMemoryBuffer>(
            const_cast<uint8_t *>(task.ArgMetadata(i)), task.ArgMetadataSize(i));
      }
-      args->at(i) = std::make_shared<RayObject>(data, metadata);
+      args->at(i) = std::make_shared<RayObject>(data, metadata, /*copy_data*/ true);


FYI @edoakes @stephanie-wang this fixes #6264

AmplabJenkins · 2019-11-25T09:29:44Z

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

AmplabJenkins · 2019-11-25T10:42:54Z

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

AmplabJenkins · 2019-11-25T11:24: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/18866/
Test PASSed.

ericl · 2019-11-25T21:33:54Z

hm //python/ray/tests:test_multi_node seems to be consistently failing in bazel, but passes locally.

ericl · 2019-11-25T22:12:05Z

multi_node_test errors seem to be due to breakage on master

ericl added 2 commits November 15, 2019 12:54

force direct

57a3fda

bump

59ff3fc

fix

b726e15

fix es

6182824

ericl force-pushed the rllib-direct branch from 620a9fe to 6182824 Compare November 18, 2019 19:21

ericl added 4 commits November 21, 2019 10:42

Merge remote-tracking branch 'upstream/master' into rllib-direct

7799487

fix actor arg resolving

ffa3a9d

remove check

66fa574

move check

776230e

ericl added 4 commits November 21, 2019 15:32

test

c36346d

fix

0811773

Merge remote-tracking branch 'upstream/master' into rllib-direct

d22f70d

update

8f4ed47

ericl added 3 commits November 21, 2019 17:13

wip

d06016c

wip debug

d64056e

wip

6d1ed4b

ericl added 2 commits November 24, 2019 15:00

Merge remote-tracking branch 'upstream/master' into rllib-direct

c72def1

wip

18eea3b

ericl changed the title ~~[WIP] Try enabling direct calls for RLlib~~ Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic, and test_actor Nov 24, 2019

ericl requested a review from stephanie-wang November 24, 2019 23:21

ericl added 2 commits November 24, 2019 15:21

medium

761e58c

remove log

c5938a6

ericl changed the title ~~Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic, and test_actor~~ Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic Nov 24, 2019

manual

34386bf

ericl commented Nov 24, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rllib-direct

94a4c83

stephanie-wang approved these changes Nov 24, 2019

View reviewed changes

format

a788565

workaround memory corrupt error in direct call objects

25abe3e

ericl mentioned this pull request Nov 25, 2019

Memory corruption for inline args if RAY_FORCE_DIRECT=1 #6264

Closed

fix mem corruption for real

536df25

ericl commented Nov 25, 2019

View reviewed changes

fix advanced test

5ab91bb

ericl merged commit 64a3a72 into ray-project:master Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic #6171

Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic #6171

ericl commented Nov 15, 2019 •

edited

Loading

AmplabJenkins commented Nov 15, 2019

AmplabJenkins commented Nov 15, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 18, 2019

ericl commented Nov 18, 2019

AmplabJenkins commented Nov 18, 2019

ericl commented Nov 18, 2019

AmplabJenkins commented Nov 19, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 22, 2019

AmplabJenkins commented Nov 22, 2019

AmplabJenkins commented Nov 22, 2019

ericl Nov 24, 2019

stephanie-wang Nov 24, 2019

ericl Nov 25, 2019

stephanie-wang Nov 24, 2019

ericl Nov 25, 2019

stephanie-wang Nov 24, 2019

ericl Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

ericl commented Nov 25, 2019

ericl Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

ericl commented Nov 25, 2019

ericl commented Nov 25, 2019

Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic #6171

Set RAY_FORCE_DIRECT=1 for run_rllib_tests, test_basic #6171

Conversation

ericl commented Nov 15, 2019 • edited Loading

Why are these changes needed?

Related issue number

Checks

AmplabJenkins commented Nov 15, 2019

AmplabJenkins commented Nov 15, 2019

AmplabJenkins commented Nov 16, 2019

AmplabJenkins commented Nov 18, 2019

ericl commented Nov 18, 2019

AmplabJenkins commented Nov 18, 2019

ericl commented Nov 18, 2019

AmplabJenkins commented Nov 19, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 21, 2019

AmplabJenkins commented Nov 22, 2019

AmplabJenkins commented Nov 22, 2019

AmplabJenkins commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

ericl commented Nov 25, 2019

Choose a reason for hiding this comment

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

AmplabJenkins commented Nov 25, 2019

ericl commented Nov 25, 2019

ericl commented Nov 25, 2019

ericl commented Nov 15, 2019 •

edited

Loading