[Core] Remove multiple core workers in one process 1/n. #24147

jovany-wang · 2022-04-24T08:27:58Z

Why are these changes needed?

This is the 1st PR to remove the code path of multiple core workers in one process. This PR is aiming to remove the flags and APIs related to num_workers.
After this PR checking in, we needn't to consider the multiple core workers any longer.

The further following PRs are related to the deeper logic refactor, like eliminating the gap between core worker and core worker process, removing the logic related to multiple workers from workerpool, gcs and etc.

BREAK CHANGE
This PR removes these APIs:

Ray.wrapRunnable();
Ray.wrapCallable();
Ray.setAsyncContext();
Ray.getAsyncContext();

And the following APIs are not allowed to invoke in a user-created thread in local mode:

Ray.getRuntimeContext().getCurrentActorId();
Ray.getRuntimeContext().getCurrentTaskId()

Note that this PR shouldn't be merged to 1.x.

Related issue number

#21547 #24567

Checks

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 :(

rkooo567 · 2022-05-03T16:33:05Z

Hey @jovany-wang do you plan to work on this?

jovany-wang · 2022-05-04T03:00:19Z

@rkooo567 Yes. I'm now on vacation, and will be back tomorrow.

jovany-wang · 2022-05-06T09:23:58Z

NOTE: After the last version of 1.x being cut, we could remove don't-merge label.

jovany-wang · 2022-05-06T09:25:19Z

@kfstorm @rkooo567 This is ready for review now.

kfstorm

More APIs to be removed:

Ray.getAsyncContext
Ray.setAsyncContext
Ray.wrapRunnable`
Ray.wrapCallable

Curious to me that you mentioned you want to remove APIs but you didn't touch the "java/api" directory.

And some implementation code in case you didn't notice (it's OK to remove them in a later PR):

RayAsyncContextUpdater
ClassLoaderTest
MultiThreadingTest
nativeSetCoreWorker
CoreWorkerProcess::SetCurrentThreadWorkerId
thread_local_core_worker_
Search number of workers in core_worker_process.h and update comments.

java/test/src/main/java/io/ray/test/JobConfigTest.java

src/ray/core_worker/core_worker.cc

src/ray/gcs/gcs_server/test/gcs_job_manager_test.cc

src/ray/raylet/worker_pool.cc

kfstorm

Clicked review twice. Ignore this comment.

jjyao · 2022-05-06T16:00:51Z

1.13 is already cut (next release will be 2.0) so we should be able to merge it.

jovany-wang · 2022-05-11T06:09:09Z

It will be also great if you can write what APIs are removed from the PR description (for the future reference)!

Added.

raulchen · 2022-05-16T06:10:29Z

java/runtime/src/main/java/io/ray/runtime/functionmanager/FunctionManager.java

@@ -34,7 +33,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-/** Manages functions by job id. */
+/** Manages functions globally. */


Suggested change

/** Manages functions globally. */

/** Manages functions in the current worker. */

globally sounds ambiguous.

rkooo567 · 2022-05-16T15:01:26Z

src/ray/core_worker/core_worker_process.cc

-    workers_.erase(worker->GetWorkerID());
-    RAY_LOG(INFO) << "Removed worker " << worker->GetWorkerID();
-  }
+  RAY_LOG(INFO) << "Removed worker " << worker->GetWorkerID();
  if (global_worker_ == worker) {


In this case, is there case global_worker != worker?

No case that globalworker != worker. I'm going to refactor core worker process in the next PR.

jovany-wang · 2022-05-17T07:04:46Z

src/ray/object_manager/common.h

-  int64_t data_size;
-  int64_t metadata_size;
+  int64_t data_size = 0;
+  int64_t metadata_size = 0;


These initializations are used to avoid the failure from UBSAN, the error message is:

[ RUN ] ObjectLifecycleManagerTest.RemoveReferenceTwoRef -- | bazel-out/k8-opt/bin/_virtual_includes/plasma_store_server_lib/ray/object_manager/common.h:53:54: runtime error: signed integer overflow: 5921777712514757477 + 8315161630257148773 cannot be represented in type 'long int'

The cause is these 2 fields are not initialized and then the result of data_size + metadata_size is overflow.

Why it doesn't happen on master?
Because this PR may change the memory view of the process. I tested on master, data_size and metadata_size are also big numbers but the sum of them is a little smaller than the max value of int64_t.

Nice catch. cc @scv119 can you check setting these to 0 & 0 is reasonable?

…33074) Multiple workers in one process is removed in #24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

…ay-project#33074) Multiple workers in one process is removed in ray-project#24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

…ay-project#33074) Multiple workers in one process is removed in ray-project#24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…ay-project#33074) Multiple workers in one process is removed in ray-project#24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

…ay-project#33074) Multiple workers in one process is removed in ray-project#24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: elliottower <elliot@elliottower.com>

…ay-project#33074) Multiple workers in one process is removed in ray-project#24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

jovany-wang requested review from wuisawesome, ericl, AmeerHajAli, robertnishihara, pcmoritz, raulchen, fishbone, scv119 and mwtian as code owners April 24, 2022 08:27

rkooo567 self-assigned this May 3, 2022

jovany-wang added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label May 4, 2022

jovany-wang force-pushed the arch-cleaning branch from 5ca0595 to fb5f771 Compare May 5, 2022 06:31

WIP

bd890f1

jovany-wang force-pushed the arch-cleaning branch 2 times, most recently from 2828d23 to 264bf5a Compare May 6, 2022 08:44

WIP: removing from workerpool

8ddc6b7

jovany-wang force-pushed the arch-cleaning branch from 264bf5a to 8ddc6b7 Compare May 6, 2022 08:45

Remove unused change.

ae23ab5

jovany-wang changed the title ~~[WIP] Remove multi core workers in one process.~~ [Core] Remove multiple core workers in one process 1/n. May 6, 2022

jovany-wang assigned kfstorm May 6, 2022

jovany-wang added do-not-merge Do not merge this PR! and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels May 6, 2022

jovany-wang assigned scv119 May 6, 2022

kfstorm reviewed May 6, 2022

View reviewed changes

jovany-wang added Ray 2.0 and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels May 11, 2022

jovany-wang mentioned this pull request May 11, 2022

[RFC][Java] Track topics on Java worker and APIs for Ray2.0 #21220

Closed

jovany-wang added 2 commits May 11, 2022 18:26

Fix CI

c611ea4

Merge branch 'master' into arch-cleaning

cba6381

kfstorm approved these changes May 12, 2022

View reviewed changes

jovany-wang mentioned this pull request May 12, 2022

[RFC] Move multi-actors-per-process feature to a separate util lib #21547

Closed

Merge branch 'master' into arch-cleaning

6b0d0c0

kfstorm mentioned this pull request May 14, 2022

[Core] Share GCS client among CoreWorker instances #18267

Closed

6 tasks

raulchen approved these changes May 16, 2022

View reviewed changes

jovany-wang added 2 commits May 16, 2022 15:29

Address comment.

540d97c

Merge branch 'master' into arch-cleaning

59357cf

rkooo567 approved these changes May 16, 2022

View reviewed changes

jovany-wang added 2 commits May 17, 2022 14:56

Add a initial value to avoid failure from UBSAN.

1853eda

Merge branch 'master' into arch-cleaning

6ecedf4

jovany-wang commented May 17, 2022

View reviewed changes

jovany-wang merged commit eb29895 into ray-project:master May 18, 2022

jovany-wang deleted the arch-cleaning branch May 18, 2022 16:36

jovany-wang mentioned this pull request May 18, 2022

Track items of Removal about multiple core workers in one process. #24567

Closed

16 tasks

jjyao mentioned this pull request Mar 6, 2023

[Core] Remove dead code related to multiple workers in one process #33074

Merged

7 tasks

jjyao added a commit that referenced this pull request Mar 7, 2023

[Core] Remove dead code related to multiple workers in one process (#…

c0aa534

…33074) Multiple workers in one process is removed in #24147 but there is still some dead code Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Remove multiple core workers in one process 1/n. #24147

[Core] Remove multiple core workers in one process 1/n. #24147

jovany-wang commented Apr 24, 2022 •

edited

Loading

rkooo567 commented May 3, 2022

jovany-wang commented May 4, 2022

jovany-wang commented May 6, 2022

jovany-wang commented May 6, 2022

kfstorm left a comment

kfstorm left a comment •

edited

Loading

jjyao commented May 6, 2022

jovany-wang commented May 11, 2022

raulchen May 16, 2022

rkooo567 May 16, 2022

jovany-wang May 16, 2022

jovany-wang May 17, 2022 •

edited

Loading

rkooo567 May 17, 2022

	/** Manages functions globally. */
	/** Manages functions in the current worker. */

[Core] Remove multiple core workers in one process 1/n. #24147

[Core] Remove multiple core workers in one process 1/n. #24147

Conversation

jovany-wang commented Apr 24, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 commented May 3, 2022

jovany-wang commented May 4, 2022

jovany-wang commented May 6, 2022

jovany-wang commented May 6, 2022

kfstorm left a comment

Choose a reason for hiding this comment

kfstorm left a comment • edited Loading

Choose a reason for hiding this comment

jjyao commented May 6, 2022

jovany-wang commented May 11, 2022

raulchen May 16, 2022

Choose a reason for hiding this comment

rkooo567 May 16, 2022

Choose a reason for hiding this comment

jovany-wang May 16, 2022

Choose a reason for hiding this comment

jovany-wang May 17, 2022 • edited Loading

Choose a reason for hiding this comment

rkooo567 May 17, 2022

Choose a reason for hiding this comment

jovany-wang commented Apr 24, 2022 •

edited

Loading

kfstorm left a comment •

edited

Loading

jovany-wang May 17, 2022 •

edited

Loading