fixing PR comments. WIP

Author: DvirDukhan

docs/commands.md

@@ -521,7 +521,7 @@ An array with alternating entries that represent the following key-value pairs:
 1. **DEVICE**: the script's device as a String
 2. **TAG**: the scripts's tag as a String
 3. **SOURCE**: the script's source code as a String
-* **ENTRY_POINTS** will return an array containing the script entry points
+4. **ENTRY_POINTS** will return an array containing the script entry points
 
 **Examples**
 
@@ -570,7 +570,7 @@ OK
 
 ## AI.SCRIPTEXECUTE
 
-The **`AI.SCRIPTEXECUTE`** command runs a script stored as a key's value on its specified device. It a list keys, input tensors and addtional script args.
+The **`AI.SCRIPTEXECUTE`** command runs a script stored as a key's value on its specified device. It a list of keys, input tensors and addtional script args.
 
 The run request is put in a queue and is executed asynchronously by a worker thread. The client that had issued the run request is blocked until the script run is completed. When needed, tensors data is automatically copied to the device prior to execution.
 
@@ -657,6 +657,8 @@ redis> AI.TENSORGET result{tag} VALUES
 3) 1) "42"
 ```
 
+Note: for the time being, as `AI.SCRIPTSET` is still avialable to use, `AI.SCRIPTEXECUTE` still supports running functions that are part of scripts stored with `AI.SCRIPTSET` or imported from old RDB/AOF files. Meaning calling `AI.SCRIPTEXECUTE` over a function without the dedicated signature of `(tensors: List[Tensor], keys: List[str], args: List[str]` will yield a "best effort" execution to match the deprecated API `AI.SCRIPTRUN` function execution. This will map `INPUTS` tensors only, to their counterpart input arguments in the function, according to the order which they apear.
+
 ### Redis Commands support.
 In RedisAI TorchScript now supports simple (non-blocking) Redis commnands via the `redis.execute` API. The following script gets a key name (`x{1}`), and an `int` value (3). First, the script `SET`s the value in the key. Next, the script `GET`s the value back from the key, and sets it in a tensor which is eventually stored under the key 'y{1}'. Note that the inputs are `str` and `int`. The script sets and gets the value and set it into a tensor.
 

src/backends/libtorch_c/torch_c.cpp

@@ -370,7 +370,7 @@ extern "C" void torchRunScript(void *scriptCtx, const char *fnName,
              * functions are not in the endpoint set to be executed in "best effort" manner.
              */
             if(!torchScript_FunctionExists(ctx, fnName)) {
-                throw std::runtime_error(std::string("Function does not exists: ") + fnName);
+                throw std::runtime_error(std::string("Function does not exist: ") + fnName);
             }
             size_t nArgs = torchScript_FunctionArgumentCountByFunctionName(ctx, fnName);
             if(nArgs > inputsCtx->tensorCount) {

src/backends/torch.c

@@ -351,7 +351,7 @@ RAI_Script *RAI_ScriptCreateTorch(const char *devicestr, const char *scriptdef,
         if (!torchScript_FunctionExists(script, entryPoint)) {
             torchDeallocContext(script);
             char *errMsg;
-            asprintf(&errMsg, "Fuction %s does not exists in the given script.", entryPoint);
+            asprintf(&errMsg, "Function %s does not exist in the given script.", entryPoint);
             RAI_SetError(error, RAI_ESCRIPTCREATE, errMsg);
             free(errMsg);
             return NULL;
@@ -362,7 +362,7 @@ RAI_Script *RAI_ScriptCreateTorch(const char *devicestr, const char *scriptdef,
         if (argCount != 3) {
             torchDeallocContext(script);
             char *errMsg;
-            asprintf(&errMsg, "Wrong number of inputs in fuction %s. Expected 3 but was %ld",
+            asprintf(&errMsg, "Wrong number of inputs in function %s. Expected 3 but was %ld",
                      entryPoint, argCount);
             RAI_SetError(error, RAI_ESCRIPTCREATE, errMsg);
             free(errMsg);
@@ -380,7 +380,7 @@ RAI_Script *RAI_ScriptCreateTorch(const char *devicestr, const char *scriptdef,
             torchDeallocContext(script);
             char *errMsg;
             asprintf(&errMsg,
-                     "Wrong inputs type in fuction %s. Expected signature similar to: def "
+                     "Wrong inputs type in function %s. Expected signature similar to: def "
                      "%s(tensors: List[Tensor], keys: List[str], args: List[str])",
                      entryPoint, entryPoint);
             RAI_SetError(error, RAI_ESCRIPTCREATE, errMsg);

src/redisai.c

@@ -830,14 +830,12 @@ int RedisAI_ScriptStore_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **ar
         AC_GetRString(&ac, &tag, 0);
     }
 
-    if (AC_IsAtEnd(&ac)) {
-        return RedisModule_ReplyWithError(
-            ctx, "ERR Insufficient arguments, missing script SOURCE and entry points");
-    }
-
     long long nEntryPoints;
     if (AC_AdvanceIfMatch(&ac, "ENTRY_POINTS")) {
-        AC_GetLongLong(&ac, &nEntryPoints, 0);
+        if (AC_GetLongLong(&ac, &nEntryPoints, 0) != AC_OK) {
+            return RedisModule_ReplyWithError(
+                ctx, "ERR Non numeric entry points number provided to AI.SCRIPTSTORE command");
+        }
     } else {
         return RedisModule_ReplyWithError(
             ctx, "ERR Insufficient arguments, missing script entry points");
@@ -885,9 +883,6 @@ int RedisAI_ScriptStore_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **ar
     }
 
     if (err.code != RAI_OK) {
-#ifdef RAI_PRINT_BACKEND_ERRORS
-        printf("ERR: %s\n", err.detail);
-#endif
         int ret = RedisModule_ReplyWithError(ctx, err.detail_oneline);
         RAI_ClearError(&err);
         return ret;

tests/flow/test_data/script_bad.txt

@@ -0,0 +1,9 @@
+def bar(tensors: List[Tensor], keys: List[str], args: List[Tensor]):
+    a = tensors[0]
+    b = tensors[1]
+    return a + b
+
+def bar_variadic(tensors: List[Tensor], keys: List[str], args: List[str]):
+    a = tensors[0]
+    l = tensors[1:]
+    return a + l[0]

tests/flow/tests_dag.py

@@ -395,13 +395,15 @@ def test_dagexecute_modelexecute_multidevice_resnet(env):
                         'AI.DAGEXECUTE', 'KEYS', '1', image_key, '|>', 'AI.MODELEXECUTE', model_name_0,
                         'INPUTS', 1, image_key, 'OUTPUTS', 1, temp_key1)
 
-    # check_error_message(env, con, "Wrong function name given to AI.SCRIPTEXECUTE command",
-    #                     'AI.DAGEXECUTE', 'KEYS', 1, '{1}',
-    #                     '|>', 'AI.TENSORSET', image_key, 'UINT8', img.shape[1], img.shape[0], 3, 'BLOB', img.tobytes(),
-    #                     '|>',
-    #                     'AI.SCRIPTEXECUTE',  script_name, 'wrong_fn',
-    #                     'INPUTS', 1, image_key,
-    #                     'OUTPUTS', 1, temp_key1)
+    ret = con.execute_command('AI.DAGEXECUTE', 
+                            'KEYS', 1, '{1}','|>',
+                            'AI.TENSORSET', image_key, 'UINT8', img.shape[1], img.shape[0], 3, 'BLOB', img.tobytes(),'|>',
+                            'AI.SCRIPTEXECUTE',  script_name, 'wrong_fn',
+                            'INPUTS', 1, image_key,
+                            'OUTPUTS', 1, temp_key1)
+    env.assertEquals(b'OK', ret[0])
+    env.assertEquals(type(ret[1]), redis.exceptions.ResponseError)
+    env.assertEquals("Function does not exist: wrong_fn",  ret[1].__str__())
 
     check_error_message(env, con, "Number of keys given as INPUTS here does not match model definition",
                         'AI.DAGEXECUTE', 'KEYS', 1, '{1}',
@@ -553,31 +555,39 @@ def test_dagexecute_modelexecute_multidevice_resnet_ensemble_alias(env):
                         'INPUTS', 1, temp_key1,
                         'OUTPUTS', 1, class_key_0)
 
-    # # The 'ensamble' function in script_name_0 expect to receive 2 inputs (not 1)
-    # check_error_message(env, con, "Wrong number of inputs provided to AI.SCRIPTEXECUTE command",
-    #                     'AI.DAGEXECUTE',
-    #                     'PERSIST', '1', class_key_0, '|>',
-    #                     'AI.TENSORSET', image_key, 'UINT8', img.shape[1], img.shape[0], 3, 'BLOB', img.tobytes(),
-    #                     '|>',
-    #                     'AI.SCRIPTEXECUTE',  script_name_0, 'pre_process_3ch',
-    #                     'INPUTS', 1, image_key,
-    #                     'OUTPUTS', 1, temp_key1,
-    #                     '|>',
-    #                     'AI.MODELEXECUTE', model_name_0,
-    #                     'INPUTS', 1, temp_key1,
-    #                     'OUTPUTS', 1, temp_key2_0,
-    #                     '|>',
-    #                     'AI.MODELEXECUTE', model_name_1,
-    #                     'INPUTS', 1, temp_key1,
-    #                     'OUTPUTS', 1, temp_key2_1,
-    #                     '|>',
-    #                     'AI.SCRIPTEXECUTE', script_name_0, 'ensemble',
-    #                     'INPUTS', 1, temp_key2_0,
-    #                     'OUTPUTS', 1, temp_key1,
-    #                     '|>',
-    #                     'AI.SCRIPTEXECUTE', script_name_0, 'post_process',
-    #                     'INPUTS', 1, temp_key1,
-    #                     'OUTPUTS', 1, class_key_0)
+    # The 'ensamble' function in script_name_0 expect to receive 2 inputs (not 1)
+    ret = con.execute_command(
+                        'AI.DAGEXECUTE',
+                        'PERSIST', '1', class_key_0, '|>',
+                        'AI.TENSORSET', image_key, 'UINT8', img.shape[1], img.shape[0], 3, 'BLOB', img.tobytes(),
+                        '|>',
+                        'AI.SCRIPTEXECUTE',  script_name_0, 'pre_process_3ch',
+                        'INPUTS', 1, image_key,
+                        'OUTPUTS', 1, temp_key1,
+                        '|>',
+                        'AI.MODELEXECUTE', model_name_0,
+                        'INPUTS', 1, temp_key1,
+                        'OUTPUTS', 1, temp_key2_0,
+                        '|>',
+                        'AI.MODELEXECUTE', model_name_1,
+                        'INPUTS', 1, temp_key1,
+                        'OUTPUTS', 1, temp_key2_1,
+                        '|>',
+                        'AI.SCRIPTEXECUTE', script_name_0, 'ensemble',
+                        'INPUTS', 1, temp_key2_0,
+                        'OUTPUTS', 1, temp_key1,
+                        '|>',
+                        'AI.SCRIPTEXECUTE', script_name_0, 'post_process',
+                        'INPUTS', 1, temp_key1,
+                        'OUTPUTS', 1, class_key_0)
+
+    env.assertEquals(b'OK', ret[0])
+    env.assertEquals(b'OK', ret[1])
+    env.assertEquals(b'OK', ret[2])
+    env.assertEquals(b'OK', ret[3])
+    env.assertEquals(b'NA', ret[5])
+    env.assertEqual(type(ret[4]), redis.exceptions.ResponseError)
+    env.assertTrue("list index out of range" in  ret[4].__str__())
 
     ret = con.execute_command(
         'AI.DAGEXECUTE',

tests/flow/tests_gears_llapi.py

@@ -209,7 +209,7 @@ def test_runtime_error(self):
         con = self.env.getConnection()
         ret = con.execute_command('rg.trigger', 'ScriptRun_AsyncRunError_test3')
         # This should raise an exception
-        self.env.assertTrue(str(ret[0]).startswith("b'Function does not exists:"))
+        self.env.assertTrue(str(ret[0]).startswith("b'Function does not exist:"))
 
 
 class TestDAGRunExecution:
@@ -335,7 +335,7 @@ def test_scriptexecute_op_runtime_error(self):
         con = self.env.getConnection()
         ret = con.execute_command('rg.trigger', 'DAGRun_test4')
         # This should raise an exception
-        self.env.assertTrue(str(ret[0]).startswith("b'Function does not exists:"))
+        self.env.assertTrue(str(ret[0]).startswith("b'Function does not exist:"))
 
     def test_build_dag_from_string(self):
         con = self.env.getConnection()

tests/flow/tests_pytorch.py

@@ -235,16 +235,30 @@ def test_pytorch_scriptstore(env):
         return
 
     con = env.getConnection()
+    script = load_file_content('script.txt')
+    old_script = load_file_content('old_script.txt')
+    bad_script = load_file_content('script_bad.txt')
 
-    check_error(env, con, 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'SOURCE', 'return 1')
+    check_error_message(env, con, "wrong number of arguments for 'AI.SCRIPTSTORE' command", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'SOURCE', 'return 1')
 
-    check_error(env, con, 'AI.SCRIPTSTORE', 'nope')
+    check_error_message(env, con, "wrong number of arguments for 'AI.SCRIPTSTORE' command", 'AI.SCRIPTSTORE', 'nope')
 
-    check_error(env, con, 'AI.SCRIPTSTORE', 'nope', 'SOURCE')
+    check_error_message(env, con, "wrong number of arguments for 'AI.SCRIPTSTORE' command", 'AI.SCRIPTSTORE', 'nope', 'SOURCE')
 
-    check_error(env, con, 'AI.SCRIPTSTORE', 'more', DEVICE)
+    check_error_message(env, con, "wrong number of arguments for 'AI.SCRIPTSTORE' command", 'AI.SCRIPTSTORE', 'more', DEVICE)
+
+    check_error_message(env, con, "Insufficient arguments, missing script entry points", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'NO_ENTRY_POINTS', 2, 'bar', 'bar_variadic', 'SOURCE', script)
+
+    check_error_message(env, con, "Non numeric entry points number provided to AI.SCRIPTSTORE command", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 'ENTRY_POINTS', 'bar', 'bar_variadic', 'SOURCE', script)
+
+    check_error_message(env, con, "Function bar1 does not exist in the given script.", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 2, 'bar', 'bar1', 'SOURCE', script)
+
+    check_error_message(env, con, "Insufficient arguments, missing script SOURCE", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 2, 'bar', 'bar_variadic', 'SOURCE')
+
+    check_error_message(env, con, "Wrong number of inputs in function bar. Expected 3 but was 2", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 2, 'bar', 'bar_variadic', 'SOURCE', old_script)
+
+    check_error_message(env, con, "Wrong inputs type in function bar. Expected signature similar to: def bar(tensors: List[Tensor], keys: List[str], args: List[str])", 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 2, 'bar', 'bar_variadic', 'SOURCE', bad_script)
 
-    script = load_file_content('script.txt')
 
     ret = con.execute_command('AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'ENTRY_POINTS', 2, 'bar', 'bar_variadic', 'SOURCE', script)
     env.assertEqual(ret, b'OK')