[WIP][POC] Initial golang worker #17922

chenk008 · 2021-08-18T09:20:03Z

Why are these changes needed?

This is an initial version for Ray to support the Golang worker.

Recently we've received a potential requirement which is to build a Golang FaaS platform on Ray. I did some PoC work to support Golang and succeed to support some basic function of Ray actor. This may be helpful to potential Golang users, so I filed this PR. Nowadays, some popular AI project are starting to use Golang, like gorgonia, gop, gonum and etc.

This PR creates a simple prototype, only support actor creation and actor's method remote call. In Ray's core code, it's needed to support custom worker entry-point because most Golang programmers are used to compile their code into an executable binary file.

For now, a new subdirectory golang is added. But I think putting Golang code into a seperated repository is also an option so it can use go mod to manage the dependencies. Would you please let me know your guys' thoughts?

gorgonia: https://github.com/gorgonia/gorgonia
gop: https://github.com/goplus/gop
gonum: https://github.com/gonum/gonum

Related issue number

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

chenk008 · 2021-09-18T07:19:54Z

Cgo has a significant performance impact, https://github.com/petermattis/fastcgo can reduce overhead, although it is unsafe.

jon-chuang · 2021-12-28T02:31:03Z

Could the vendored libraries be made to be retrieved by Bazel?

Also, is the intention to have each go worker live in its own process? Or can they share a heap and GC like with Java worker?

chenk008 · 2021-12-30T13:21:13Z

Could the vendored libraries be made to be retrieved by Bazel?

Also, is the intention to have each go worker live in its own process? Or can they share a heap and GC like with Java worker?

@jon-chuang Here the vendored libraries are introduced temporarily to help compile. Indeed the bazel can help to manage go dependence.

The go worker process wasn't designed on purpose. I didn't figure out how to manage the relationship between actor and goroutine.

jon-chuang · 2022-01-10T01:55:53Z

src/ray/core_worker/lib/golang/go_worker.h

+extern void go_worker_execute(GoInt task_type, GoSlice ray_function_info, GoSlice args,
+                              GoSlice return_values);
+
+void go_worker_shutdown();


change this to Shutdown?

jon-chuang · 2022-01-10T08:19:36Z

src/ray/core_worker/lib/golang/go_worker.cpp

+  return 0;
+}
+
+DataBuffer *RayBufferToDataBuffer(const std::shared_ptr<ray::Buffer> buffer) {


How do you ensure the lifetime of the buffer? Who owns the respective buffers and is responsible for destructing it?

jon-chuang · 2022-01-10T08:20:16Z

src/ray/core_worker/lib/golang/go_worker.cpp

@@ -0,0 +1,328 @@
+#include "go_worker.h"


could you name this file ".cc" to respect the convention in the rest of the codebase?

jon-chuang · 2022-01-12T12:43:06Z

golang/pkg/ray/task_executor.go

+    }
+    dataSize := C.ulong(len(b))
+    // we need copy to avoid go runtime change the address
+    p := C.malloc(dataSize)


since you malloc here, do you make sure to call free in the execute task context?

jon-chuang · 2022-01-12T13:06:04Z

src/ray/core_worker/lib/golang/go_worker.cpp

+
+RAY_EXPORT DataValue *go_worker_AllocateDataValue(void *data_ptr, size_t data_size,
+                                                  void *meta_ptr, size_t meta_size) {
+  auto dv = new DataValue();


Are you sure that C++ will free DataValue created this way when passed back to C++ as a ptr? I believe it will not, right? How should it be managed, then?

jon-chuang · 2022-01-12T13:12:57Z

src/ray/core_worker/lib/golang/go_worker.cpp

+
+        std::vector<std::shared_ptr<DataValue>> return_value_list;
+        for (size_t i = 0; i < return_ids.size(); i++) {
+          return_value_list.push_back(make_shared<DataValue>());


What is the purpose of making this make_shared?

Since the return value list will get its pointers replaced by pointers generated from the callback without destructing the original make_shared ptr, this would result in memory leak.

bveeramani · 2022-01-30T05:45:26Z

‼️ ACTION REQUIRED ‼️

We've switched our code formatter from YAPF to Black (see #21311).

To prevent issues with merging your code, here's what you'll need to do:

Install Black

pip install -I black==21.12b0

Format changed files with Black

curl -o format-changed.sh https://gist.githubusercontent.com/bveeramani/42ef0e9e387b755a8a735b084af976f2/raw/7631276790765d555c423b8db2b679fd957b984a/format-changed.sh
chmod +x ./format-changed.sh
./format-changed.sh
rm format-changed.sh

Commit your changes.

git add --all
git commit -m "Format Python code with Black"

Merge master into your branch.

git pull upstream master

Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated format.sh.

jon-chuang · 2022-02-01T16:51:19Z

Could I ask what exactly is the issue with goroutines and actor? I believe this could be similar to async actors in python... In the context of a non-async actor, I don't believe the runtime will even need to spin up additional goroutines....?

jon-chuang · 2022-02-09T01:25:58Z

Actually I guess I understand your problem.
Async Go worker <- tasks
[run as goroutines]

Go worker is a single runtime context. As long as we ensure runtime spins up a new goroutine per task executed, and we do not block the main loop, we are probably fine. One should use some benchmarks to validate the performance.

@chenk008 what do you think?

Questions: does go worker block on calls to cgo/fastcgo?

stale · 2022-03-12T22:28:15Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

kfstorm · 2022-03-13T09:20:39Z

‼️ ACTION REQUIRED ‼️

We've updated our formatting configuration for C++ code. (see #22725)

This PR includes C++ code change. To prevent issues with merging your code, here's what you'll need to do:

Merge the latest changes from upstream/master branch into your branch.

git pull upstream master
git merge upstream/master

Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated C++ formatting configuration.

Format changed files.

scripts/format.sh

Commit your changes.

git add --all
git commit -m "Format C++ code"

stale · 2022-04-16T15:57:07Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

stale · 2022-07-30T20:23:13Z

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

chenk008 added 30 commits July 30, 2021 00:04

golang

167b459

golang

2676066

golang

074e2f9

golang

3228fc2

golang

578e8ac

go-c-go ok!

b0953d3

fix

c93ff9a

go

fb6aeca

go

a4deb01

golang

48488a5

golang

688cf99

fix

a716480

add golang worker command

ad14e91

Merge branch 'golang_worker' of github.com:alipay/ray into golang_worker

ef1c4af

golang

b04936c

Merge branch 'golang_worker' of github.com:alipay/ray into golang_worker

1b117e7

golang

d64a2e7

fix

58f2965

fix

d5280ae

fix

9b9993c

fix

b408035

fix

62101ea

fix

fafa1cb

fix

d78c95f

fix

1d9265e

golang

7ec6720

fix

9506f1d

fix

6c9faa8

fix

0a1035f

add api

f62267b

chenk008 added 4 commits August 26, 2021 11:54

add macos library

6bfcf18

improve readme

f17fa28

improve readme

c3a3d4f

fix

ff67efa

chenk008 requested a review from raulchen as a code owner September 8, 2021 11:43

chenk008 added 4 commits September 10, 2021 14:21

add bazel config

9cc673f

fix bazel

6135081

fix bazel

bb7db9f

support actor with args

8dd71bb

ericl removed their assignment Oct 8, 2021

jon-chuang reviewed Jan 10, 2022

View reviewed changes

jon-chuang reviewed Jan 12, 2022

View reviewed changes

jon-chuang mentioned this pull request Jan 13, 2022

[Rust] ray-rs/prototype-v2 #21572

Closed

6 tasks

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 12, 2022

stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 13, 2022

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 16, 2022

stale bot closed this Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][POC] Initial golang worker #17922

[WIP][POC] Initial golang worker #17922

chenk008 commented Aug 18, 2021 •

edited

Loading

chenk008 commented Sep 18, 2021 •

edited

Loading

jon-chuang commented Dec 28, 2021

chenk008 commented Dec 30, 2021

jon-chuang Jan 10, 2022

jon-chuang Jan 10, 2022

jon-chuang Jan 10, 2022

jon-chuang Jan 12, 2022

jon-chuang Jan 12, 2022 •

edited

Loading

jon-chuang Jan 12, 2022 •

edited

Loading

bveeramani commented Jan 30, 2022

jon-chuang commented Feb 1, 2022

jon-chuang commented Feb 9, 2022

stale bot commented Mar 12, 2022

kfstorm commented Mar 13, 2022

stale bot commented Apr 16, 2022

stale bot commented Jul 30, 2022

[WIP][POC] Initial golang worker #17922

[WIP][POC] Initial golang worker #17922

Conversation

chenk008 commented Aug 18, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

chenk008 commented Sep 18, 2021 • edited Loading

jon-chuang commented Dec 28, 2021

chenk008 commented Dec 30, 2021

jon-chuang Jan 10, 2022

Choose a reason for hiding this comment

jon-chuang Jan 10, 2022

Choose a reason for hiding this comment

jon-chuang Jan 10, 2022

Choose a reason for hiding this comment

jon-chuang Jan 12, 2022

Choose a reason for hiding this comment

jon-chuang Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

jon-chuang Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

bveeramani commented Jan 30, 2022

‼️ ACTION REQUIRED ‼️

jon-chuang commented Feb 1, 2022

jon-chuang commented Feb 9, 2022

stale bot commented Mar 12, 2022

kfstorm commented Mar 13, 2022

‼️ ACTION REQUIRED ‼️

stale bot commented Apr 16, 2022

stale bot commented Jul 30, 2022

chenk008 commented Aug 18, 2021 •

edited

Loading

chenk008 commented Sep 18, 2021 •

edited

Loading

jon-chuang Jan 12, 2022 •

edited

Loading

jon-chuang Jan 12, 2022 •

edited

Loading