First Part of Internal Ray API Refactor #1173

pcmoritz · 2017-10-31T15:06:36Z

This PR imports the new C++ APIs for the GCS into the repo, sets up the build environment for them, implements the task classes and as an example how to use those implements object table and task table lookups, as well as tests for the above.

It does NOT change any of the existing calls to the new calls.

AmplabJenkins · 2017-10-31T15:43:17Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-31T15:43:17Z

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

AmplabJenkins · 2017-10-31T15:45:33Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-31T15:45: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/2238/
Test PASSed.

AmplabJenkins · 2017-10-31T15:46:43Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-31T15:46: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/2239/
Test PASSed.

AmplabJenkins · 2017-10-31T15:56:58Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-31T15:56:59Z

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

AmplabJenkins · 2017-10-31T18:41:18Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-10-31T18:41:19Z

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

AmplabJenkins · 2017-11-07T05:10:26Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-07T05:10:26Z

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

AmplabJenkins · 2017-11-16T00:38:58Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-16T00:38:58Z

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

AmplabJenkins · 2017-11-16T02:18:35Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-16T02:18:36Z

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

AmplabJenkins · 2017-11-16T06:45:03Z

Build finished. Test FAILed.

AmplabJenkins · 2017-11-16T06:45:03Z

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

AmplabJenkins · 2017-12-13T01:38:30Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-13T01:38:30Z

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

AmplabJenkins · 2017-12-13T01:54:31Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-13T01:54:32Z

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

AmplabJenkins · 2017-12-13T02:28:19Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-13T02:28:20Z

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

AmplabJenkins · 2017-12-13T02:35:10Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-13T02:35:10Z

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

AmplabJenkins · 2017-12-13T02:47:40Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-13T02:47:41Z

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

concretevitamin

Quick look, hope it helps!

concretevitamin · 2017-12-13T23:19:11Z

src/common/redis_module/ray_redis_module.cc

@@ -388,6 +388,46 @@ bool PublishObjectNotification(RedisModuleCtx *ctx,
  return true;
 }

+int TableAdd_RedisCommand(RedisModuleCtx *ctx,


Comment? E.g. // Experimental usage.

doing that now

concretevitamin · 2017-12-13T23:22:45Z

src/common/redis_module/ray_redis_module.cc

+
+  RedisModuleKey *key;
+  key = OpenPrefixedKey(ctx, "T:", id, REDISMODULE_READ);
+  size_t len;


In local scope I think it's a good practice to always initialize: size_t len = 0;. I could be misremembering, but such a statement was an undefined behavior?

Yeah, you are right, it is undefined behavior in the sense that if it is accessed the value is undefined (in many cases it will just be whatever was in the corresponding memory location on the stack). In this case it would be fine because RedisModule_StringDMA will initialize the memory location and not read before it, but I'm initializing it nonetheless :)

concretevitamin · 2017-12-13T23:23:13Z

src/common/redis_module/ray_redis_module.cc

+  RedisModuleKey *key;
+  key = OpenPrefixedKey(ctx, "T:", id, REDISMODULE_READ);
+  size_t len;
+  char *buf = RedisModule_StringDMA(key, &len, REDISMODULE_READ);


Add const, and put asterisk right after the type? const char* buf

added the const; the asterisk is currently encoded that way in the clang-format file and enforced (we started as a C project, remember), we will change everywhere consistently with a future PR...

concretevitamin · 2017-12-13T23:24:15Z

src/common/redis_module/ray_redis_module.cc

+  }
+
+  if (RedisModule_CreateCommand(ctx, "ray.table_lookup",
+                                TableLookup_RedisCommand, "write", 0, 0,


concretevitamin · 2017-12-13T23:29:11Z

src/ray/constants.h

+constexpr int64_t kUniqueIDSize = 20;
+
+/// Prefix for the object table keys in redis.
+constexpr char ObjectTablePrefix[] = "ObjectTable";


We should be consistent with naming?: kObjectTablePrefix?

Less importantly, here's another way of initializing string constants:

constexpr char* const kObjectTablePrefix = "ObjectTable";

did the first, for the second, the compiler complains with
error: ISO C++11 does not allow conversion from string literal to 'char *const'

concretevitamin · 2017-12-13T23:35:43Z

src/ray/gcs/client.h

+  std::shared_ptr<RedisContext> context_;
+};
+
+class SyncGSCClient {


S and C reordered

Also, why not Gcs? https://google.github.io/styleguide/cppguide.html#Function_Names says Prefer to capitalize acronyms as single words (i.e. StartRpc(), not StartRPC()).

Done, including the capitalization. I agree it is more readable if there is a followup word.

concretevitamin · 2017-12-13T23:37:00Z

src/ray/util/macros.h

+#define RAY_UTIL_MACROS_H
+
+// From Google gutil
+#ifndef RAY_DISALLOW_COPY_AND_ASSIGN


concretevitamin · 2017-12-13T23:37:22Z

src/ray/util/logging.h

+
+namespace ray {
+
+// Stubbed versions of macros defined in glog/logging.h, intended for


Love these macros.

concretevitamin · 2017-12-13T23:38:18Z

src/ray/status.h

+#include "ray/util/visibility.h"
+
+// Return the given status if it is not OK.
+#define RAY_RETURN_NOT_OK(s)           \


Let's be consistent with a bunch of codebase out there and use RAY_RETURN_IF_ERROR?

I'd prefer this one since it it the one used by arrow and we already use it in a bunch of places in plasma (which was merged with arrow). But no big deal, if you strongly prefer the other way we can also rename it :)

concretevitamin · 2017-12-13T23:39:24Z

src/ray/status.h

+// non-const method, all threads accessing the same Status must use
+// external synchronization.
+
+// Adapted from Apache Arrow, Apache Kudu, TensorFlow


Yes. Let's return Status everywhere when it makes sense.

AmplabJenkins · 2017-12-14T06:15:20Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-14T06:15:21Z

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

AmplabJenkins · 2017-12-14T06:22:34Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-14T06:22: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/2795/
Test FAILed.

AmplabJenkins · 2017-12-14T06:41:22Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-14T06:41:22Z

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

robertnishihara · 2017-12-14T08:01:06Z

src/ray/constants.h

+/// Prefix for the object table keys in redis.
+constexpr char kObjectTablePrefix[] = "ObjectTable";
+/// Prefix for the task table keys in redis.
+constexpr char kTaskTablePrefix[] = "TaskTable";


These prefixes aren't used yet, right? We're still hardcoding TT and OI and OL and things like that in various places in the code so at some point we'll have to replace those with these constants.

Yeah, that's right!

I think a more "correct" way (at least how I was educated at my previous employer :p) of defining constants are,

In header: extern const int64_t kUniqueIDSize; ...; extern const char* const kObjectTablePrefix;

In .cc: const int64_t kUniqueIDSize = 20; ...; const char* const kObjectTablePrefix = "ObjectTable"

Just an FYI, no need to do it this way in this PR.

robertnishihara · 2017-12-14T08:05:15Z

src/ray/gcs/client.h

+#include "ray/id.h"
+#include "ray/status.h"
+#include "ray/gcs/tables.h"
+#include "ray/util/logging.h"


would be more alphabetical to move ray/gcs before ray/id

things are ordered in such a way that includes that are in more nested directories are further down

robertnishihara · 2017-12-14T08:05:47Z

src/ray/gcs/client_test.cc

+extern "C" {
+#include "hiredis/async.h"
+#include "hiredis/hiredis.h"
+#include "hiredis/adapters/ae.h"


maybe move adapters above async?

same as the above

concretevitamin

Heroic PR, @pcmoritz! Just a few nits.

concretevitamin · 2017-12-14T22:39:44Z

src/ray/constants.h

+/// Prefix for the object table keys in redis.
+constexpr char kObjectTablePrefix[] = "ObjectTable";
+/// Prefix for the task table keys in redis.
+constexpr char kTaskTablePrefix[] = "TaskTable";


I think a more "correct" way (at least how I was educated at my previous employer :p) of defining constants are,

In header: extern const int64_t kUniqueIDSize; ...; extern const char* const kObjectTablePrefix;

In .cc: const int64_t kUniqueIDSize = 20; ...; const char* const kObjectTablePrefix = "ObjectTable"

Just an FYI, no need to do it this way in this PR.

concretevitamin · 2017-12-14T22:41:58Z

src/ray/gcs/client.cc

+
+namespace gcs {
+
+AsyncGcsClient::AsyncGcsClient() {}


Since these are empty, might be cleaner to just remove ctor & dtor declarations and definitions.

concretevitamin · 2017-12-14T22:42:43Z

src/ray/gcs/client.h

+
+class AsyncGcsClient {
+ public:
+  AsyncGcsClient();


concretevitamin · 2017-12-14T22:43:34Z

src/ray/gcs/client.h

+                          std::string *name,
+                          std::string *data);
+
+  Status AddExport(const std::string &driver_id, std::string &export_data);


const for 2nd arg

concretevitamin · 2017-12-14T22:44:37Z

src/ray/gcs/redis_context.cc

+  }
+  int64_t callback_index = reinterpret_cast<int64_t>(privdata);
+  redisReply *reply = reinterpret_cast<redisReply *>(r);
+  std::string data = "";


Just std::string data; suffices, I think.

concretevitamin · 2017-12-14T22:45:40Z

src/ray/gcs/redis_context.h

+  RedisCallback &get(int64_t callback_index);
+
+ private:
+  RedisCallbackManager() : num_callbacks(0){};


We should keep it simple if possible: remove num_callbacks and its maintenance, since it's just callbacks_.size().

concretevitamin · 2017-12-14T22:47:24Z

src/ray/gcs/redis_context.h

+
+  int64_t add(const RedisCallback &function);
+
+  RedisCallback &get(int64_t callback_index);


Why not keep it more consistent to name as Add(..) and Get(..)?

concretevitamin · 2017-12-14T22:47:55Z

src/ray/gcs/redis_context.h

+
+class RedisContext {
+ public:
+  RedisContext() {}


Nit: remove empty ctor, dtor.

robertnishihara · 2017-12-14T22:54:32Z

Thanks @concretevitamin, we'll fix the nits in a follow up PR.

robertnishihara mentioned this pull request Nov 15, 2017

Restructure worker.py code. #614

Closed

pcmoritz added 10 commits December 11, 2017 19:58

add Ray status class

5df278f

add C++ util files

3f4126f

add ID types

9c8417b

more APIs

cb25bd2

build system integration

88e24b9

add test infrastructure and implement some APIs

6ff6b83

add more tests

deca844

fix bugs

6c05c75

add task table tests

77f319a

update

7a44f82

pcmoritz force-pushed the c++-api branch from c48f670 to 7a44f82 Compare December 12, 2017 03:58

linting

679aa4d

pcmoritz force-pushed the c++-api branch from a98001e to 679aa4d Compare December 13, 2017 01:20

fixes

97bdade

pcmoritz force-pushed the c++-api branch from ef03e66 to 97bdade Compare December 13, 2017 01:49

add constants file

c2e2d7d

concretevitamin suggested changes Dec 13, 2017

View reviewed changes

pcmoritz added 3 commits December 13, 2017 21:32

comments

0ac36aa

more fixes

2f57fe6

fix linting

85d25c5

robertnishihara approved these changes Dec 14, 2017

View reviewed changes

concretevitamin approved these changes Dec 14, 2017

View reviewed changes

robertnishihara merged commit cac5f47 into ray-project:master Dec 14, 2017

robertnishihara deleted the c++-api branch December 14, 2017 22:54


		namespace ray {

		// Stubbed versions of macros defined in glog/logging.h, intended for


		int64_t add(const RedisCallback &function);

		RedisCallback &get(int64_t callback_index);

First Part of Internal Ray API Refactor #1173

First Part of Internal Ray API Refactor #1173

Conversation

pcmoritz commented Oct 31, 2017 • edited Loading

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Oct 31, 2017

AmplabJenkins commented Nov 7, 2017

AmplabJenkins commented Nov 7, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Nov 16, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

AmplabJenkins commented Dec 13, 2017

concretevitamin left a 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

pcmoritz Dec 14, 2017 • edited Loading

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

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 Dec 14, 2017

AmplabJenkins commented Dec 14, 2017

AmplabJenkins commented Dec 14, 2017

AmplabJenkins commented Dec 14, 2017

AmplabJenkins commented Dec 14, 2017

AmplabJenkins commented Dec 14, 2017

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

concretevitamin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertnishihara commented Dec 14, 2017

pcmoritz commented Oct 31, 2017 •

edited

Loading

pcmoritz Dec 14, 2017 •

edited

Loading