[CoreWorker] lazy bind core_work's job_config through task spec. by scv119 · Pull Request #31375 · ray-project/ray

scv119 · 2022-12-30T22:46:44Z

Why are these changes needed?

Previously the worker get job_config information from raylet on construction. This prevents us from lazily binding job_config to workers. This PR enables lazily bind job_config, by piggybacking job_confg in TaskSpec, and initialize the job_config when the worker receives task execution request (push_task) call.

We also refactor the WorkerContext and RayletClient as part of the chagne.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

scv119 · 2023-01-02T22:19:20Z

@liuyang-my the Java test failed but i'm not quite sure what exactly happened reading the logs. Do you know what might have gone wrong? (presumably we are hitting some deadlock issues?) https://buildkite.com/ray-project/oss-ci-build-pr/builds/8396#01857127-086b-406b-92da-6f935dcc8447 is the failed test

scv119 · 2023-01-03T01:25:48Z

 }
 // ---Actor death contexts end----

+message JobConfig {


move JobConfig to common.proto to break the circular dependency

cadedaniel · 2023-01-05T00:46:31Z

  std::string task_name =
      invocation.name.empty() ? functionDescriptor->DefaultTaskName() : invocation.name;

+  static rpc::JobConfig kDefaultJobConfig;


Is static a premature optimization? Is it possible for this to be called from multiple threads and corrupt the object?

fishbone · 2023-01-06T03:09:06Z

Question: will this slow down the perfs? I think this adds runtime env to all task specs (previously, only one). Do you mind benchmarking the perf regression?

Besides this, do you think it's good to pass job config through stdin for the workers? If doing this way, we probably could limit the all changes in worker pool.

I'm also thinking in the future this maybe need extension. We probably don't want to pass everything to task spec I believe.

Btw, ok with this approach if the benchmark with job config is ok. But let's add comment to job config proto to let people know it's passed to all tasks repeatedly.

Still reviewing...

fishbone · 2023-01-06T03:15:09Z

  const TaskID &GetCurrentTaskId();

-  const JobID &GetCurrentJobID();
+  JobID GetCurrentJobID();


Why update this one but not the rest?

this calls to the context which returns by value instead of reference; so we change it accordingly.
change them to return by value is a great idea, but will yield double the size of the PR and touching a lot of cpp runtime code, thus we prefer not changing it in this PR.

fishbone · 2023-01-06T03:17:15Z

  std::string task_name =
      invocation.name.empty() ? functionDescriptor->DefaultTaskName() : invocation.name;

+  rpc::JobConfig kDefaultJobConfig;


nit:
make it a global const static variable?

not sure, but I always feel kXYZ is global const variables.

fishbone · 2023-01-06T03:17:48Z

  return JobID::FromBinary(message_->job_id());
 }

+rpc::JobConfig TaskSpecification::JobConfig() const { return message_->job_config(); }


why not const reference?

fishbone · 2023-01-06T03:18:50Z

                                 Language::PYTHON,
                                 FunctionDescriptorBuilder::BuildPython("", "", "", ""),
                                 job_id,
+                                 config,


nit:
rpc::JobConfig()

seems easier to read. Otherwise we need check what's config in the code.

fishbone · 2023-01-06T03:23:43Z

+  JobID current_job_id_ GUARDED_BY(mutex_);
+  rpc::JobConfig job_config_ GUARDED_BY(mutex_);


should we use optional here if it's lazily initialized?

+1. Why don't we use optional instead of default config?

scv119 · 2023-01-06T05:37:45Z

thanks for reviewing!

kicking off benchmark here: https://buildkite.com/ray-project/release-tests-pr/builds/24753

rkooo567

This PR doesn't touch the worker pool code. In that case, those workers started by each job still considered to belong to the job?

Also, I am curious about the behavior changes. Previously,

When the worker starts, it belongs to the job
When the job terminates the workers are killed.

With this change, how are these semantics changed?

rkooo567 · 2023-01-06T14:33:29Z

  return JobID::FromBinary(message_->job_id());
 }

+rpc::JobConfig TaskSpecification::JobConfig() const { return message_->job_config(); }


rkooo567 · 2023-01-06T14:35:28Z

+    job_config_ = job_config;
+  }
+  RAY_CHECK(current_job_id_ == job_id);
+  RAY_CHECK(google::protobuf::util::MessageDifferencer::Equals(job_config_, job_config_));


is this necessary? Is overhead of MessageDifferencer::Equals small?

rkooo567 · 2023-01-06T14:36:22Z

+  return current_job_id_;
+}
+
+rpc::JobConfig WorkerContext::GetCurrentJobConfig() const {


const reference?

accessing a reference to a state in critical section yields undefined behavior.

rkooo567 · 2023-01-06T14:37:12Z

+  JobID current_job_id_ GUARDED_BY(mutex_);
+  rpc::JobConfig job_config_ GUARDED_BY(mutex_);


+1. Why don't we use optional instead of default config?

rkooo567 · 2023-01-06T14:42:36Z

+
+  if (options_.worker_type == WorkerType::DRIVER &&
+      !options_.serialized_job_config.empty()) {
+    // Driver populates job_config through worker startup options.


IIUC driver is not started with worker startup options?

Maybe it should be "driver populates the job config via initialization. Workers populates it when the first task is received"?

rkooo567

request change since Cade approves it already..

Fix and reopen java tests closed in #31375 Co-authored-by: Marcus Zhang <zxl265370@antgroup.com>

) Previously the worker get job_config information from raylet on construction. This prevents us from lazily binding job_config to workers. This PR enables lazily bind job_config, by piggybacking job_confg in TaskSpec, and initialize the job_config when the worker receives task execution request (push_task) call. We also refactor the WorkerContext and RayletClient as part of the chagne.

Fix and reopen java tests closed in #31375 Co-authored-by: Marcus Zhang <zxl265370@antgroup.com>

scv119 changed the title ~~[CoreWorker] populate taskspec with job_config~~ [CoreWorker] populate job_config through task spec. Jan 1, 2023

scv119 changed the title ~~[CoreWorker] populate job_config through task spec.~~ [CoreWorker] lazy bind core_work's job_config through task spec. Jan 1, 2023

scv119 assigned rkooo567 Jan 1, 2023

scv119 marked this pull request as ready for review January 1, 2023 01:04

scv119 requested review from AmeerHajAli, ericl, fishbone, pcmoritz, raulchen, robertnishihara and wuisawesome as code owners January 1, 2023 01:04

scv119 assigned cadedaniel Jan 1, 2023

cadedaniel reviewed Jan 3, 2023

View reviewed changes

Comment thread src/ray/core_worker/context.cc Outdated

Comment thread src/ray/core_worker/context.cc Outdated

scv119 assigned fishbone Jan 3, 2023

scv119 commented Jan 3, 2023

View reviewed changes

scv119 mentioned this pull request Jan 3, 2023

[Core][deprecate run_on_all_workers 1/n] set worker's sys.path through JobConfig._py_driver_sys_path #31383

Merged

7 tasks

cadedaniel reviewed Jan 3, 2023

View reviewed changes

Comment thread cpp/src/ray/runtime/local_mode_ray_runtime.cc Outdated

Comment thread src/ray/core_worker/context.cc Outdated

Comment thread src/ray/core_worker/context.cc Outdated

Comment thread src/ray/core_worker/core_worker.cc

scv119 force-pushed the job_config branch from 5884262 to 4575d0c Compare January 4, 2023 09:42

cadedaniel approved these changes Jan 5, 2023

View reviewed changes

fishbone reviewed Jan 6, 2023

View reviewed changes

rkooo567 reviewed Jan 6, 2023

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 6, 2023

rkooo567 requested changes Jan 6, 2023

View reviewed changes

scv119 added 23 commits January 11, 2023 16:05

add

b4d5811

add

ca36d7b

add

52f5586

fix

d8a2dcc

fix test

0c7497d

add

c1f7b65

address comments

8a8f1a0

address comments

6a58d99

add

cf94c45

fix cpp

1701102

fix

8f739fb

more fix

24a341c

fix

c044ff4

fix bug

ba64067

add check

df07391

add

72e99b1

address comments

d684763

add

397e996

add

340aaad

add

43a4823

fix merge conflict

081caa0

fix test

3b8cede

disable affected java test

5bb1a30

scv119 force-pushed the job_config branch from ea35bff to 5bb1a30 Compare January 12, 2023 00:06

fix rebase conflicts

0b665c5

scv119 merged commit 302a7e5 into ray-project:master Jan 12, 2023

MisterLin1995 mentioned this pull request Jan 12, 2023

Fix and reopen java tests #31631

Merged

7 tasks

scv119 pushed a commit that referenced this pull request Jan 12, 2023

[fix]byte buffer refer error (#31631)

05d6f30

Fix and reopen java tests closed in #31375 Co-authored-by: Marcus Zhang <zxl265370@antgroup.com>

AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023

[fix]byte buffer refer error (#31631)

c86f2f8

Fix and reopen java tests closed in #31375 Co-authored-by: Marcus Zhang <zxl265370@antgroup.com>

		JobID current_job_id_ GUARDED_BY(mutex_);
		rpc::JobConfig job_config_ GUARDED_BY(mutex_);

Conversation

scv119 commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

scv119 commented Jan 2, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishbone commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fishbone Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

scv119 commented Jan 6, 2023

Uh oh!

rkooo567 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scv119 commented Dec 30, 2022 •

edited

Loading

fishbone commented Jan 6, 2023 •

edited

Loading

fishbone Jan 6, 2023 •

edited

Loading