Let ai.scriptget and ai.modelget commands run successfully without optional args (#791)

* Align script get command, so it will return both source and meta if no optional argument is specified.

* Documentation fixes

* Add default behaviour for AI.MODELGET + documentation

Author: alonre24

docs/commands.md

@@ -222,8 +222,8 @@ AI.MODELGET <key> [META] [BLOB]
 _Arguments
 
 * **key**: the model's key name
-* **META**: will return the model's meta information on backend, device, tag and batching parameters
-* **BLOB**: will return the model's blob containing the serialized model
+* **META**: will return only the model's meta information on backend, device, tag and batching parameters
+* **BLOB**: will return only the model's blob containing the serialized model
 
 _Return_
 
@@ -237,7 +237,7 @@ An array of alternating key-value pairs as follows:
 1. **INPUTS**: array reply with one or more names of the model's input nodes (applicable only for TensorFlow models)
 1. **OUTPUTS**: array reply with one or more names of the model's output nodes (applicable only for TensorFlow models)
 1. **MINBATCHTIMEOUT**: The time in milliseconds for which the engine will wait before executing a request to run the model, when the number of incoming requests is lower than `MINBATCHSIZE`. When `MINBATCHTIMEOUT` is 0, the engine will not run the model before it receives at least `MINBATCHSIZE` requests.
-1. **BLOB**: a blob containing the serialized model (when called with the `BLOB` argument) as a String. If the size of the serialized model exceeds `MODEL_CHUNK_SIZE` (see `AI.CONFIG` command), then an array of chunks is returned. The full serialized model can be obtained by concatenating the chunks.
+1. **BLOB**: a blob containing the serialized model as a String. If the size of the serialized model exceeds `MODEL_CHUNK_SIZE` (see `AI.CONFIG` command), then an array of chunks is returned. The full serialized model can be obtained by concatenating the chunks.
 
 **Examples**
 
@@ -415,7 +415,7 @@ The **`AI.SCRIPTSTORE`** command stores a [TorchScript](https://pytorch.org/docs
 **Redis API**
 
 ```
-AI.SCRIPTSTORE <key> <device> [TAG tag] ENTRY_POINTS <entry_point_amoint> <entry_point> [<entry_point>...] SOURCE "<script>"
+AI.SCRIPTSTORE <key> <device> [TAG tag] ENTRY_POINTS <entry_points_count> <entry_point> [<entry_point>...] SOURCE "<script>"
 ```
 
 _Arguments_
@@ -427,7 +427,7 @@ _Arguments_
     * **CPU**: a CPU device
     * **GPU**: a GPU device
     * **GPU:0**, ..., **GPU:n**: a specific GPU device on a multi-GPU system
-* **ENTRY_POINTS** A list of entry points to be used in the script. Each entry point should have the signature of `def entry_point(tensors: List[Tensor], keys: List[str], args: List[str])`. The purpose of each list is as follows:
+* **ENTRY_POINTS** A list of function names in the script to be used as entry points upon execution. Each entry point should have the signature of `def entry_point(tensors: List[Tensor], keys: List[str], args: List[str])`. The purpose of each list is as follows:
 * `tensors`: A list holding the input tensors to the function.
 * `keys`: A list of keys that the torch script is about to preform read/write operations on.
 * `args`: A list of additional arguments to the function. If the desired argument is not from type string, it is up to the caller to cast it to the right type, within the script.
@@ -510,18 +510,18 @@ AI.SCRIPTGET <key> [META] [SOURCE]
 _Arguments_
 
 * **key**: the script's key name
-* **META**: will return the script's meta information on device and tag
-* **SOURCE**: will return a string containing [TorchScript](https://pytorch.org/docs/stable/jit.html) source code
+* **META**: will return only the script's meta information on device, tag and entry points.
+* **SOURCE**: will return only the string containing [TorchScript](https://pytorch.org/docs/stable/jit.html) source code
 
 _Return_
 
 An array with alternating entries that represent the following key-value pairs:
-!!!!The command returns a list of key-value strings, namely `DEVICE device TAG tag [SOURCE source]`.
+!!!!The command returns a list of key-value strings, namely `DEVICE device TAG tag ENTRY_POINTS [entry_point ...] SOURCE source`.
 
 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
-4. **ENTRY_POINTS** will return an array containing the script entry points
+4. **ENTRY_POINTS** will return an array containing the script entry point functions
 
 **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 of 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 receives a list of Redis keys, a list of input tensors and an additional list of arguments to be used in the script.
 
 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.
 
@@ -583,25 +583,25 @@ A `TIMEOUT t` argument can be specified to cause a request to be removed from th
 
 ```
 AI.SCRIPTEXECUTE <key> <function> 
-[KEYS n <key> [keys...]]
-[INPUTS m <input> [input ...]]
-[ARGS k <arg> [arg...]]
-[OUTPUTS k <output> [output ...] [TIMEOUT t]]+
+[KEYS <keys_count> <key> [keys...]]
+[INPUTS <input_count> <input> [input ...]]
+[ARGS <args_count> <arg> [arg...]]
+[OUTPUTS <outputs_count> <output> [output ...]]
+[TIMEOUT t]
 ```
 
 _Arguments_
 
-* **key**: the script's key name
-* **function**: the name of the function to run
-* **KEYS**: Either a squence of key names that the script will access before, during and after its execution, or a tag which all those keys share. 
-* **INPUTS**: Denotes the beginning of the input parameters list, followed by its length and one or more input tensors.
-* **ARGS**: A list additional arguments that a user can send to the script. All args are sent as strings, but can be casted to other types supported by torch script, such as `int`, or `float`. 
-
+* **key**: the script's key name.
+* **function**: the name of the entry point function to run.
+* **KEYS**: Denotes the beginning of a list of Redis key names that the script will access to during its execution, for both read and/or write operations.
+* **INPUTS**: Denotes the beginning of the input tensors list, followed by its length and one or more input tensors.
+* **ARGS**: Denotes the beginning of a list of additional arguments that a user can send to the script. All args are sent as strings, but can be casted to other types supported by torch script, such as `int`, or `float`. 
 * **OUTPUTS**: denotes the beginning of the output tensors keys' list, followed by its length and one or more key names.
 * **TIMEOUT**: the time (in ms) after which the client is unblocked and a `TIMEDOUT` string is returned
 
 Note:
-Either `KEYS` or `INPUTS` scopes should be provided this command (one or both scopes are acceptable). Those scopes indicate keyspace access and such, the right shard to execute the command at. Redis will verify that all potional key accesses are done to the right shard.
+Either `KEYS` or `INPUTS` scopes should be provided this command (one or both scopes are acceptable). Those scopes indicate keyspace access and such, the right shard to execute the command at. Redis will verify that all potential key accesses are done to the right shard.
 
 _Return_
 
@@ -611,27 +611,12 @@ A simple 'OK' string, a simple `TIMEDOUT` string, or an error.
 
 The following is an example of running the previously-created 'myscript' on two input tensors:
 
-```
-redis> AI.TENSORSET mytensor1 FLOAT 1 VALUES 40
-OK
-redis> AI.TENSORSET mytensor2 FLOAT 1 VALUES 2
-OK
-redis> AI.SCRIPTEXECUTE myscript addtwo KEYS 3 mytensor1 mytensor2 result INPUTS 2 mytensor1 mytensor2 OUTPUTS 1 result
-OK
-redis> AI.TENSORGET result VALUES
-1) FLOAT
-2) 1) (integer) 1
-3) 1) "42"
-```
-
-Note: The above command could be executed with a shorter version, given all the keys are tagged with the same tag:
-
 ```
 redis> AI.TENSORSET mytensor1{tag} FLOAT 1 VALUES 40
 OK
 redis> AI.TENSORSET mytensor2{tag} FLOAT 1 VALUES 2
 OK
-redis> AI.SCRIPTEXECUTE myscript{tag} addtwo KEYS 1 {tag} INPUTS 2 mytensor1{tag} mytensor2{tag} OUTPUTS 1 result{tag}
+redis> AI.SCRIPTEXECUTE myscript{tag} addtwo INPUTS 2 mytensor1{tag} mytensor2{tag} OUTPUTS 1 result{tag}
 OK
 redis> AI.TENSORGET result{tag} VALUES
 1) FLOAT
@@ -652,18 +637,18 @@ redis> AI.TENSORSET mytensor2{tag} FLOAT 1 VALUES 1
 OK
 redis> AI.TENSORSET mytensor3{tag} FLOAT 1 VALUES 1
 OK
-redis> AI.SCRIPTEXECUTE myscript{tag} addn keys 1 {tag} INPUTS 3 mytensor1{tag} mytensor2{tag} mytensor3{tag} OUTPUTS 1 result{tag}
+redis> AI.SCRIPTEXECUTE myscript{tag} addn INPUTS 3 mytensor1{tag} mytensor2{tag} mytensor3{tag} OUTPUTS 1 result{tag}
 OK
 redis> AI.TENSORGET result{tag} VALUES
 1) FLOAT
 2) 1) (integer) 1
 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.
+Note: for the time being, as `AI.SCRIPTSET` is still available 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 appear.
 
 ### 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.
+In RedisAI TorchScript now supports simple (non-blocking) Redis commands 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.
 
 ```
 def redis_int_to_tensor(redis_value: int):
@@ -692,13 +677,13 @@ The command receives 3 inputs:
 Return value - the model execution output tensors (List of torch.Tensor)
 The following script creates two tensors, and executes the (tensorflow) model which is stored under the name 'tf_mul{1}' with these two tensors as inputs.
 ```
-def test_model_execute(keys:List[str]):
+def test_model_execute(tensors: List[Tensor], keys: List[str], args: List[str]):
     a = torch.tensor([[2.0, 3.0], [2.0, 3.0]])
     b = torch.tensor([[2.0, 3.0], [2.0, 3.0]])
     return redisAI.model_execute(keys[0], [a, b], 1) # assume keys[0] is the model name stored in RedisAI.
 ```
 ```
-redis> AI.SCRIPTEXECUTE redis_scripts{1} test_model_execute KEYS 1 {1} LIST_INPUTS 1 tf_mul{1} OUTPUTS 1 y{1}
+redis> AI.SCRIPTEXECUTE redis_scripts{1} test_model_execute KEYS 1 tf_mul{1} OUTPUTS 1 y{1}
 OK
 redis> AI.TENSORGET y{1} VALUES
 1) (float) 4
@@ -833,9 +818,9 @@ A `TIMEOUT t` argument can be specified to cause a request to be removed from th
 **Redis API**
 
 ```
-AI.DAGEXECUTE [[LOAD <n> <key-1> <key-2> ... <key-n>] |
-          [PERSIST <n> <key-1> <key-2> ... <key-n>] |
-          [ROUTING <routing_tag>]]
+AI.DAGEXECUTE [LOAD <n> <key-1> <key-2> ... <key-n>]
+          [PERSIST <n> <key-1> <key-2> ... <key-n>]
+          [ROUTING <routing_tag>]
           [TIMEOUT t]
           |> <command> [|>  command ...]
 ```
@@ -844,7 +829,7 @@ _Arguments_
 
 * **LOAD**: denotes the beginning of the input tensors keys' list, followed by the number of keys, and one or more key names
 * **PERSIST**: denotes the beginning of the output tensors keys' list, followed by the number of keys, and one or more key names
-* **ROUTING**: denotes the a key name or a tag that will assist in routing the dag execution command to the right shard. Redis will verify that all potential key accesses are done to within the target shard.
+* **ROUTING**: denotes a key to be used in the DAG or a tag that will assist in routing the dag execution command to the right shard. Redis will verify that all potential key accesses are done to within the target shard.
 
 _While each of the LOAD, PERSIST and ROUTING sections are optional (and may appear at most once in the command), the command must contain **at least one** of these 3 keywords._
 * **TIMEOUT**: an optional argument, denotes the time (in ms) after which the client is unblocked and a `TIMEDOUT` string is returned

src/redisai.c

@@ -416,31 +416,24 @@ int RedisAI_ModelGet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
         return REDISMODULE_ERR;
     }
 
-    int meta = 0;
-    int blob = 0;
+    int meta = false;
+    int blob = false;
     for (int i = 2; i < argc; i++) {
         const char *optstr = RedisModule_StringPtrLen(argv[i], NULL);
         if (!strcasecmp(optstr, "META")) {
-            meta = 1;
+            meta = true;
         } else if (!strcasecmp(optstr, "BLOB")) {
-            blob = 1;
+            blob = true;
         }
     }
 
-    if (!meta && !blob) {
-        return RedisModule_ReplyWithError(ctx, "ERR no META or BLOB specified");
-    }
-
     char *buffer = NULL;
     size_t len = 0;
 
-    if (blob) {
+    if (!meta || blob) {
         RAI_ModelSerialize(mto, &buffer, &len, &err);
-        if (err.code != RAI_OK) {
-#ifdef RAI_PRINT_BACKEND_ERRORS
-            printf("ERR: %s\n", err.detail);
-#endif
-            int ret = RedisModule_ReplyWithError(ctx, err.detail);
+        if (RAI_GetErrorCode(&err) != RAI_OK) {
+            int ret = RedisModule_ReplyWithError(ctx, RAI_GetErrorOneLine(&err));
             RAI_ClearError(&err);
             if (*buffer) {
                 RedisModule_Free(buffer);
@@ -455,12 +448,14 @@ int RedisAI_ModelGet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
         return REDISMODULE_OK;
     }
 
-    const int outentries = blob ? 18 : 16;
-    RedisModule_ReplyWithArray(ctx, outentries);
+    // The only case where we return only META, is when META is given but BLOB
+    // was not. Otherwise, we return both META+SOURCE
+    const int out_entries = (meta && !blob) ? 16 : 18;
+    RedisModule_ReplyWithArray(ctx, out_entries);
 
     RedisModule_ReplyWithCString(ctx, "backend");
-    const char *backendstr = RAI_GetBackendName(mto->backend);
-    RedisModule_ReplyWithCString(ctx, backendstr);
+    const char *backend_str = RAI_GetBackendName(mto->backend);
+    RedisModule_ReplyWithCString(ctx, backend_str);
 
     RedisModule_ReplyWithCString(ctx, "device");
     RedisModule_ReplyWithCString(ctx, mto->devicestr);
@@ -495,7 +490,8 @@ int RedisAI_ModelGet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
     RedisModule_ReplyWithCString(ctx, "minbatchtimeout");
     RedisModule_ReplyWithLongLong(ctx, (long)mto->opts.minbatchtimeout);
 
-    if (meta && blob) {
+    // This condition is the negation of (meta && !blob)
+    if (!meta || blob) {
         RedisModule_ReplyWithCString(ctx, "blob");
         RAI_ReplyWithChunks(ctx, buffer, len);
         RedisModule_Free(buffer);
@@ -651,43 +647,40 @@ int RedisAI_ScriptGet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv
         return REDISMODULE_ERR;
     }
 
-    int meta = 0;
-    int source = 0;
+    bool meta = false;   // Indicates whether META argument was given.
+    bool source = false; // Indicates whether SOURCE argument was given.
     for (int i = 2; i < argc; i++) {
         const char *optstr = RedisModule_StringPtrLen(argv[i], NULL);
         if (!strcasecmp(optstr, "META")) {
-            meta = 1;
+            meta = true;
         } else if (!strcasecmp(optstr, "SOURCE")) {
-            source = 1;
+            source = true;
         }
     }
-
-    if (!meta && !source) {
-        return RedisModule_ReplyWithError(ctx, "ERR no META or SOURCE specified");
-    }
-
+    // If only SOURCE arg was given, return only the script source.
     if (!meta && source) {
         RedisModule_ReplyWithCString(ctx, sto->scriptdef);
         return REDISMODULE_OK;
     }
+    // We return (META+SOURCE) if both args are given, or if none of them was given.
+    // The only case where we return only META data, is if META is given while SOURCE was not.
+    int out_entries = (source || !meta) ? 8 : 6;
+    RedisModule_ReplyWithArray(ctx, out_entries);
 
-    int outentries = source ? 8 : 6;
-
-    RedisModule_ReplyWithArray(ctx, outentries);
     RedisModule_ReplyWithCString(ctx, "device");
     RedisModule_ReplyWithCString(ctx, sto->devicestr);
     RedisModule_ReplyWithCString(ctx, "tag");
     RedisModule_ReplyWithString(ctx, sto->tag);
-    if (source) {
-        RedisModule_ReplyWithCString(ctx, "source");
-        RedisModule_ReplyWithCString(ctx, sto->scriptdef);
-    }
     RedisModule_ReplyWithCString(ctx, "Entry Points");
     size_t nEntryPoints = array_len(sto->entryPoints);
     RedisModule_ReplyWithArray(ctx, nEntryPoints);
     for (size_t i = 0; i < nEntryPoints; i++) {
         RedisModule_ReplyWithCString(ctx, sto->entryPoints[i]);
     }
+    if (source || !meta) {
+        RedisModule_ReplyWithCString(ctx, "source");
+        RedisModule_ReplyWithCString(ctx, sto->scriptdef);
+    }
     return REDISMODULE_OK;
 }
 

tests/flow/tests_commands.py

@@ -54,7 +54,7 @@ def test_modelstore_errors(env):
                         'AI.MODELSTORE', 'm{1}', 'TORCH', DEVICE, 'BATCHSIZE', 2, 'BLOB')
 
 
-def test_modelget_errors(env):
+def test_modelget(env):
     if not TEST_TF:
         env.debugPrint("Skipping test since TF is not available", force=True)
         return
@@ -69,8 +69,17 @@ def test_modelget_errors(env):
 
     # ERR model key is empty
     con.execute_command('DEL', 'DONT_EXIST{1}')
-    check_error_message(env, con, "model key is empty",
-                        'AI.MODELGET', 'DONT_EXIST{1}')
+    check_error_message(env, con, "model key is empty", 'AI.MODELGET', 'DONT_EXIST{1}')
+
+    # The default behaviour on success is return META+BLOB
+    model_pb = load_file_content('graph.pb')
+    con.execute_command('AI.MODELSTORE', 'm{1}', 'TF', DEVICE, 'INPUTS', 2, 'a', 'b', 'OUTPUTS', 1, 'mul',
+                              'BLOB', model_pb)
+    _, backend, _, device, _, tag, _, batchsize, _, minbatchsize, _, inputs, _, outputs, _, minbatchtimeout, _, blob = \
+            con.execute_command('AI.MODELGET', 'm{1}')
+    env.assertEqual([backend, device, tag, batchsize, minbatchsize, minbatchtimeout, inputs, outputs],
+                         [b"TF", bytes(DEVICE, "utf8"), b"", 0, 0, 0, [b"a", b"b"], [b"mul"]])
+    env.assertEqual(blob, model_pb)
 
 
 def test_modelexecute_errors(env):

tests/flow/tests_deprecated_commands.py

@@ -239,6 +239,8 @@ def test_pytorch_scriptset(env):
 
     ret = con.execute_command('AI.SCRIPTSET', 'ket{1}', DEVICE, 'TAG', 'asdf', 'SOURCE', script)
     env.assertEqual(ret, b'OK')
+    _, device, _, tag, _, entry_points, _, source = con.execute_command('AI.SCRIPTGET', 'ket{1}')
+    env.assertEqual([device, tag, entry_points, source], [bytes(DEVICE, "utf8"), b"asdf", [], script])
 
     ensureSlaveSynced(con, env)