Merge pull request #575 from RedisAI/Valgrind_cleaning

alonre24 · DvirDukhan · commit d9d9387e6272 · 2021-02-03T13:23:19.000+02:00
Valgrind cleaning
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -201,7 +201,63 @@ jobs:
       - run:
           name: Test with valgrind
           command: |
-            make -C opt test VALGRIND=1
+            make -C opt test VALGRIND=1 CLUSTER=0 AOF=0
+          no_output_timeout: 120m
+
+  valgrind-cluster:
+    docker:
+      - image: redisfab/rmbuilder:6.0.5-x64-buster
+    steps:
+      - checkout
+      - run:
+          name: Submodule checkout
+          command: git submodule update --init --recursive
+      - restore_cache:
+          keys:
+            - build-dependencies-{{ checksum "get_deps.sh" }}
+          # If no exact match is found will get dependencies from source
+      - setup-automation
+      - run:
+          name: Install dependencies
+          command: |
+            ./opt/readies/bin/getredis -v 6 --valgrind --force
+            ./get_deps.sh cpu
+      - run:
+          name: Build for valgrind with cluster
+          command: |
+            make -C opt all VALGRIND=1 SHOW=1
+      - run:
+          name: Test with valgrind and cluster
+          command: |
+            make -C opt test VALGRIND=1 GEN=0 AOF=0
+          no_output_timeout: 120m
+
+  valgrind-AOF:
+    docker:
+      - image: redisfab/rmbuilder:6.0.5-x64-buster
+    steps:
+      - checkout
+      - run:
+          name: Submodule checkout
+          command: git submodule update --init --recursive
+      - restore_cache:
+          keys:
+            - build-dependencies-{{ checksum "get_deps.sh" }}
+          # If no exact match is found will get dependencies from source
+      - setup-automation
+      - run:
+          name: Install dependencies
+          command: |
+            ./opt/readies/bin/getredis -v 6 --valgrind --force
+            ./get_deps.sh cpu
+      - run:
+          name: Build for valgrind with AOF
+          command: |
+            make -C opt all VALGRIND=1 SHOW=1
+      - run:
+          name: Test with valgrind and AOF
+          command: |
+            make -C opt test VALGRIND=1 GEN=0 CLUSTER=0
           no_output_timeout: 120m
 
   build-macos:
@@ -406,7 +462,13 @@ workflows:
           <<: *on-any-branch
           <<: *after-linter
       - valgrind:
-          <<: *on-master
+          <<: *on-any-branch
+          <<: *after-linter
+      - valgrind-cluster:
+          <<: *on-integ-branch
+          <<: *after-linter
+      - valgrind-AOF:
+          <<: *on-integ-branch
           <<: *after-linter
       - build-and-test-gpu:
           <<: *on-any-branch
diff --git a/opt/redis_valgrind.sup b/opt/redis_valgrind.sup
@@ -26,6 +26,13 @@
    obj:*/libtorch.so.*
 }
 
+{
+   ignore_unversioned_libs
+   Memcheck:Overlap
+   ...
+   obj:*/libtorch_cpu.so*
+}
+
 {
    ignore_unversioned_libs
    Memcheck:Leak
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -16,6 +16,7 @@ file (GLOB BACKEND_COMMON_SRC
         err.c
         util/dict.c
         tensor.c
+        util/string_utils.c
         serialization/ai_datatypes.c)
 
 ADD_LIBRARY(redisai_obj OBJECT
diff --git a/src/DAG/dag.c b/src/DAG/dag.c
@@ -122,8 +122,7 @@ static int _StoreTensorInKeySpace(RedisModuleCtx *ctx, RAI_Tensor *tensor,
         RedisModule_ReplyWithError(ctx, "ERR could not save tensor");
         goto clean_up;
     }
-    if (RedisModule_ModuleTypeSetValue(key, RedisAI_TensorType, RAI_TensorGetShallowCopy(tensor)) !=
-        REDISMODULE_OK) {
+    if (RedisModule_ModuleTypeSetValue(key, RedisAI_TensorType, tensor) != REDISMODULE_OK) {
         RedisModule_ReplyWithError(ctx, "ERR could not save tensor");
         RedisModule_CloseKey(key);
         goto clean_up;
@@ -152,6 +151,7 @@ static void _DAG_PersistTensors(RedisModuleCtx *ctx, RedisAI_RunInfo *rinfo) {
             persist_entry = AI_dictNext(persist_iter);
             continue;
         }
+        tensor = RAI_TensorGetShallowCopy(tensor);
         if (_StoreTensorInKeySpace(ctx, tensor, persist_key_name, true) == REDISMODULE_ERR) {
             *rinfo->dagError = 1;
             RedisModule_Log(ctx, "warning",
diff --git a/src/backends/tensorflow.c b/src/backends/tensorflow.c
@@ -247,7 +247,7 @@ RAI_Model *RAI_ModelCreateTF(RAI_Backend backend, const char *devicestr, RAI_Mod
 
     for (size_t i = 0; i < ninputs; ++i) {
         TF_Operation *oper = TF_GraphOperationByName(model, inputs[i]);
-        if (oper == NULL) {
+        if (oper == NULL || strcmp(TF_OperationOpType(oper), "Placeholder") != 0) {
             size_t len = strlen(inputs[i]);
             char *msg = RedisModule_Calloc(60 + len, sizeof(*msg));
             sprintf(msg, "ERR Input node named \"%s\" not found in TF graph.", inputs[i]);
diff --git a/src/libtorch_c/torch_extensions/torch_redis.cpp b/src/libtorch_c/torch_extensions/torch_redis.cpp
@@ -56,8 +56,8 @@ torch::IValue redisExecute(std::string fn_name, std::vector<std::string> args )
     RedisModuleCallReply *reply = RedisModule_Call(ctx, fn_name.c_str(), "!v", arguments, len);
     RedisModule_ThreadSafeContextUnlock(ctx);
     torch::IValue value = IValueFromRedisReply(reply);
-    RedisModule_FreeThreadSafeContext(ctx);
     RedisModule_FreeCallReply(reply);
+    RedisModule_FreeThreadSafeContext(ctx);
     for(int i= 0; i < len; i++){
         RedisModule_FreeString(NULL, arguments[i]);
     }
diff --git a/src/tensor.c b/src/tensor.c
@@ -15,6 +15,7 @@
 #include "rmutil/alloc.h"
 #include "tensor_struct.h"
 #include "util/dict.h"
+#include "util/string_utils.h"
 #include <assert.h>
 #include <pthread.h>
 #include <stddef.h>
@@ -96,7 +97,8 @@ int Tensor_DataTypeStr(DLDataType dtype, char *dtypestr) {
 
 RAI_Tensor *RAI_TensorCreateWithDLDataType(DLDataType dtype, long long *dims, int ndims,
                                            int tensorAllocMode) {
-    const size_t dtypeSize = Tensor_DataTypeSize(dtype);
+
+    size_t dtypeSize = Tensor_DataTypeSize(dtype);
     if (dtypeSize == 0) {
         return NULL;
     }
@@ -132,11 +134,6 @@ RAI_Tensor *RAI_TensorCreateWithDLDataType(DLDataType dtype, long long *dims, in
         break;
     }
 
-    if (tensorAllocMode != TENSORALLOC_NONE && data == NULL) {
-        RedisModule_Free(ret);
-        return NULL;
-    }
-
     ret->tensor = (DLManagedTensor){.dl_tensor = (DLTensor){.ctx = ctx,
                                                             .data = data,
                                                             .ndim = ndims,
@@ -162,18 +159,14 @@ void RAI_RStringDataTensorDeleter(DLManagedTensor *arg) {
         RedisModuleString *rstr = (RedisModuleString *)arg->manager_ctx;
         RedisModule_FreeString(NULL, rstr);
     }
-
+    RedisModule_Free(arg->dl_tensor.data);
     RedisModule_Free(arg);
 }
 
-RAI_Tensor *RAI_TensorCreateWithDLDataTypeAndRString(DLDataType dtype, long long *dims, int ndims,
-                                                     RedisModuleString *rstr) {
-    const size_t dtypeSize = Tensor_DataTypeSize(dtype);
-    if (dtypeSize == 0) {
-        return NULL;
-    }
+RAI_Tensor *_TensorCreateWithDLDataTypeAndRString(DLDataType dtype, size_t dtypeSize,
+                                                  long long *dims, int ndims,
+                                                  RedisModuleString *rstr, RAI_Error *err) {
 
-    RAI_Tensor *ret = RedisModule_Alloc(sizeof(*ret));
     int64_t *shape = RedisModule_Alloc(ndims * sizeof(*shape));
     int64_t *strides = RedisModule_Alloc(ndims * sizeof(*strides));
 
@@ -188,9 +181,21 @@ RAI_Tensor *RAI_TensorCreateWithDLDataTypeAndRString(DLDataType dtype, long long
     }
 
     DLContext ctx = (DLContext){.device_type = kDLCPU, .device_id = 0};
+    size_t nbytes = len * dtypeSize;
+
+    size_t blob_len;
+    const char *blob = RedisModule_StringPtrLen(rstr, &blob_len);
+    if (blob_len != nbytes) {
+        RedisModule_Free(shape);
+        RedisModule_Free(strides);
+        RAI_SetError(err, RAI_ETENSORSET, "ERR data length does not match tensor shape and type");
+        return NULL;
+    }
+    char *data = RedisModule_Alloc(nbytes);
+    memcpy(data, blob, nbytes);
+    RAI_HoldString(NULL, rstr);
 
-    char *data = (char *)RedisModule_StringPtrLen(rstr, NULL);
-
+    RAI_Tensor *ret = RedisModule_Alloc(sizeof(*ret));
     ret->tensor = (DLManagedTensor){.dl_tensor = (DLTensor){.ctx = ctx,
                                                             .data = data,
                                                             .ndim = ndims,
@@ -637,13 +642,16 @@ int RAI_parseTensorSetArgs(RedisModuleString **argv, int argc, RAI_Tensor **t, i
         RAI_SetError(error, RAI_ETENSORSET, "wrong number of arguments for 'AI.TENSORSET' command");
         return -1;
     }
+
     // get the tensor datatype
     const char *typestr = RedisModule_StringPtrLen(argv[2], NULL);
-    size_t datasize = RAI_TensorDataSizeFromString(typestr);
-    if (!datasize) {
+    DLDataType datatype = RAI_TensorDataTypeFromString(typestr);
+    size_t datasize = Tensor_DataTypeSize(datatype);
+    if (datasize == 0) {
         RAI_SetError(error, RAI_ETENSORSET, "ERR invalid data type");
         return -1;
     }
+
     const char *fmtstr;
     int datafmt = TENSOR_NONE;
     int tensorAllocMode = TENSORALLOC_CALLOC;
@@ -699,23 +707,17 @@ int RAI_parseTensorSetArgs(RedisModuleString **argv, int argc, RAI_Tensor **t, i
         }
     }
 
-    const long long nbytes = len * datasize;
-    size_t datalen;
-    const char *data;
-    DLDataType datatype = RAI_TensorDataTypeFromString(typestr);
     if (datafmt == TENSOR_BLOB) {
         RedisModuleString *rstr = argv[argpos];
-        RedisModule_RetainString(NULL, rstr);
-        *t = RAI_TensorCreateWithDLDataTypeAndRString(datatype, dims, ndims, rstr);
+        *t = _TensorCreateWithDLDataTypeAndRString(datatype, datasize, dims, ndims, rstr, error);
     } else {
         *t = RAI_TensorCreateWithDLDataType(datatype, dims, ndims, tensorAllocMode);
     }
-
-    if (!t) {
+    if (!(*t)) {
         array_free(dims);
-        RAI_SetError(error, RAI_ETENSORSET, "ERR could not create tensor");
         return -1;
     }
+
     long i = 0;
     if (datafmt == TENSOR_VALUES) {
         for (; (argpos <= argc - 1) && (i < len); argpos++) {
diff --git a/tests/flow/tests.sh b/tests/flow/tests.sh
@@ -72,7 +72,6 @@ valgrind_config() {
 
 	RLTEST_ARGS+="\
 		--use-valgrind \
-		--vg-no-fail-on-errors \
 		--vg-suppressions $VALGRIND_SUPRESSIONS"
 }
 
diff --git a/tests/flow/tests_common.py b/tests/flow/tests_common.py
@@ -50,6 +50,7 @@ def test_common_tensorset_error_replies(env):
     try:
         con.execute_command('SET','non-tensor','value')
         con.execute_command('AI.TENSORSET', 'non-tensor', 'INT32', 2, 'unsupported', 2, 3)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -58,6 +59,7 @@ def test_common_tensorset_error_replies(env):
     # ERR invalid data type
     try:
         con.execute_command('AI.TENSORSET', 'z', 'INT128', 2, 'VALUES', 2, 3)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -66,6 +68,7 @@ def test_common_tensorset_error_replies(env):
     # ERR invalid or negative value found in tensor shape
     try:
         con.execute_command('AI.TENSORSET', 'z', 'INT32', -1, 'VALUES', 2, 3)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -74,6 +77,7 @@ def test_common_tensorset_error_replies(env):
     # ERR invalid argument found in tensor shape
     try:
         con.execute_command('AI.TENSORSET', 'z', 'INT32', 2, 'unsupported', 2, 3)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -82,6 +86,7 @@ def test_common_tensorset_error_replies(env):
     # ERR invalid value
     try:
         con.execute_command('AI.TENSORSET', 'z', 'FLOAT', 2, 'VALUES', 2, 'A')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -90,56 +95,58 @@ def test_common_tensorset_error_replies(env):
     # ERR invalid value
     try:
         con.execute_command('AI.TENSORSET', 'z', 'INT32', 2, 'VALUES', 2, 'A')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
         env.assertEqual(exception.__str__(), "invalid value")
 
     try:
         con.execute_command('AI.TENSORSET', 1)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
 
     try:
         con.execute_command('AI.TENSORSET', 'y', 'FLOAT')
-    except Exception as e:
-        exception = e
-        env.assertEqual(type(exception), redis.exceptions.ResponseError)
-
-    try:
-        con.execute_command('AI.TENSORSET', 'y', 'FLOAT', '2')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
 
     try:
         con.execute_command('AI.TENSORSET', 'y', 'FLOAT', 2, 'VALUES')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
 
     try:
         con.execute_command('AI.TENSORSET', 'y', 'FLOAT', 2, 'VALUES', 1)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
 
     try:
         con.execute_command('AI.TENSORSET', 'y', 'FLOAT', 2, 'VALUES', '1')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
 
     try:
         con.execute_command('AI.TENSORSET', 'blob_tensor_moreargs', 'FLOAT', 2, 'BLOB', '\x00', 'extra-argument')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
         env.assertEqual("wrong number of arguments for 'AI.TENSORSET' command", exception.__str__())
 
     try:
         con.execute_command('AI.TENSORSET', 'blob_tensor_lessargs', 'FLOAT', 2, 'BLOB')
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
@@ -148,6 +155,7 @@ def test_common_tensorset_error_replies(env):
     # ERR data length does not match tensor shape and type
     try:
         con.execute_command('AI.TENSORSET', 'sample_raw_wrong_blob_for_dim', 'FLOAT', 1, 1, 28, 280, 'BLOB', sample_raw)
+        env.assertFalse(True)
     except Exception as e:
         exception = e
         env.assertEqual(type(exception), redis.exceptions.ResponseError)
diff --git a/tests/flow/tests_dag.py b/tests/flow/tests_dag.py
@@ -230,6 +230,7 @@ def test_dagrun_ro_modelrun_scriptrun_resnet(env):
     if(VALGRIND):
         env.debugPrint("skipping {} since it's hanging CI".format(sys._getframe().f_code.co_name), force=True)
         env.skip()
+
     con = env.getConnection()
     model_name = 'imagenet_model{{1}}'
     script_name = 'imagenet_script{{1}}'
diff --git a/tests/flow/tests_llapi.py b/tests/flow/tests_llapi.py
@@ -12,7 +12,7 @@
 def ensure_test_module_loaded(f):
     @wraps(f)
     def wrapper(env, *args, **kwargs):
-        goal_dir = os.path.join(os.getcwd(), "../module/LLAPI.so")
+        goal_dir = os.path.join(os.path.dirname(__file__), "../module/LLAPI.so")
         TEST_MODULE_PATH = os.path.abspath(goal_dir)
         con = env.getConnection()
         modules = con.execute_command("MODULE", "LIST")
diff --git a/tests/flow/tests_tensorflow.py b/tests/flow/tests_tensorflow.py

Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,13 @@`
`26`	`26`	`obj:/libtorch.so.`
`27`	`27`	`}`
`28`	`28`
	`29`	`+{`
	`30`	`+ ignore_unversioned_libs`
	`31`	`+ Memcheck:Overlap`
	`32`	`+ ...`
	`33`	`+ obj:/libtorch_cpu.so`
	`34`	`+}`
	`35`	`+`
`29`	`36`	`{`
`30`	`37`	`ignore_unversioned_libs`
`31`	`38`	`Memcheck:Leak`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,6 @@ valgrind_config() {`
`72`	`72`
`73`	`73`	`RLTEST_ARGS+="\`
`74`	`74`	`--use-valgrind \`
`75`		`- --vg-no-fail-on-errors \`
`76`	`75`	`--vg-suppressions $VALGRIND_SUPRESSIONS"`
`77`	`76`	`}`
`78`	`77`