script execute command #682

DvirDukhan · 2021-04-13T19:23:13Z

This PR implements the new command:

AI.SCRIPTEXECUTE <key> <function>
[KEYS <keys_count> <key> [key ...]]
[[INPUTS <input_count> <input> [input ...]]? 
[LIST_INPUTS <size> <input> [input...]]* 
[OUTPUTS <output_count> <output> [output ...]]?
[TIMEOUT <time>]?

The new command infers the types of its inputs from the function signature and allows support for the following types:

torch::Tensor
torch::string
int
float
List[T] where T is one of the above.

This support allows better flexibility and expressiveness on the torch-redis commands extension in RedisAI.

The new command structure scopes:

KEYS - mandatory scope - this scope was added in order to infer the execution shard according to a given hash slot tag, or a list of keys that the script execution is going to access, either is inputs, outputs, or during the script run itself via the torch-redis extension.
For example, for a script key s{1}. if the inputs are a{1}, b{1} and the output is c{1} than the user just needs to provide KEYS 1 {1}. if the inputs are a, b, and the output is c, and the user is not sure about the hash slot, the user need to provide KEYS 3 a b c.
INPUTS - In this new command, an input is a union of torch::Tensor | torch::string | int | float, meaning that inputs now may not be tensors in the keyspace. If there are keyspace tensors in the inputs they either should be mentioned on the KEYS scope or have the KEYS scope aware of their hash slot tagging. The inputs should be provided in the order they are appearing in the function signature (without lists). Inputs number must be provided.
LIST_INPUTS - a list of inputs. Multiple lists can be provided and they should be provided in the order they are appearing in the function signature. list size must be provided
OUTPUTS - key names to store output tensors in the keyspace. Outputs number must be provided.
TIMEOUT - same as before

codecov · 2021-04-28T20:06:39Z

Codecov Report

Merging #682 (f7b870e) into master (9d8faa8) will increase coverage by 0.06%.
The diff coverage is 78.52%.

@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
+ Coverage   78.86%   78.93%   +0.06%     
==========================================
  Files          43       48       +5     
  Lines        6865     7343     +478     
==========================================
+ Hits         5414     5796     +382     
- Misses       1451     1547      +96

Impacted Files	Coverage Δ
src/execution/run_info.c	`93.54% <ø> (+20.97%)`	⬆️
src/execution/utils.c	`67.85% <ø> (-1.11%)`	⬇️
src/execution/execution_contexts/scriptRun_ctx.c	`70.22% <70.22%> (ø)`
src/backends/libtorch_c/torch_c.cpp	`63.44% <71.09%> (+5.55%)`	⬆️
src/execution/parsing/script_commands_parser.c	`71.73% <71.73%> (ø)`
src/execution/parsing/deprecated.c	`77.93% <77.55%> (ø)`
src/execution/execution_contexts/modelRun_ctx.c	`78.70% <78.70%> (ø)`
src/redis_ai_objects/script.c	`87.20% <82.35%> (+4.70%)`	⬆️
src/execution/parsing/model_commands_parser.c	`93.40% <93.40%> (ø)`
src/redisai.c	`85.82% <95.23%> (+0.16%)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a60ff12...f7b870e. Read the comment docs.

alonre24

Nice work!!! liked the new files structure and organization. I left some comments and suggestions, we can discuss it also tomorrow. Besides that, looks like there are quite a few cases of parsing errors that are not tested, consider adding some tests to increase coverage (depends on how thorough we want to be...)

src/CMakeLists.txt

src/backends/libtorch_c/torch_c.h

src/redis_ai_objects/script_struct.h

src/execution/execution_contxets/scriptRun_ctx.h

tests/flow/tests_deprecated_commands.py

tests/flow/tests_pytorch.py

alonre24

Looks good, added another few comments and suggestions.

alonre24 · 2021-05-04T07:38:02Z

docs/commands.md

@@ -514,7 +514,122 @@ OK
    The `AI.SCRIPTDEL` is equivalent to the [Redis `DEL` command](https://redis.io/commands/del) and should be used in its stead. This ensures compatibility with all deployment options (i.e., stand-alone vs. cluster, OSS vs. Enterprise).


+## AI.SCRIPTEXECUTE
+
+The **`AI.SCRIPTEXECUTE`** command runs a script stored as a key's value on its specified device. It accepts one or more input tensors, int, float or strings and store output tensors.


Maybe instead of "it accepts one or more input tensors..." say something like: "it uses one or more Redis keys both as inputs and/or by directly accessing them from within the script itself. Inputs may include tensors, integers, floats or strings, while outputs are tensors to be stored in Redis"

It accepts one or more inputs, where the inputs could be tensors stored in RedisAI, int, float, or strings and stores the script outputs as RedisAI tensors if required.

alonre24 · 2021-05-04T07:41:41Z

docs/commands.md

+**Redis API**
+
+```
+AI.SCRIPTRUN <key> <function> 


AI.SCRIPTEXECUTE
KEYS n <key> [keys...]
[INPUTS m <input> [input ...]
| LIST_INPUTS l <input> [input ...]]*
[OUTPUTS k <output> [output ...]] [TIMEOUT t]

AI.SCRIPTEXECUTE <key> <function> KEYS n <key> [keys...] [INPUTS m <input> [input ...] | [LIST_INPUTS l <input> [input ...]]*]+ [OUTPUTS k <output> [output ...] [TIMEOUT t]]+

alonre24 · 2021-05-04T07:51:26Z

docs/commands.md

+    return a + torch.stack(args).sum()
+```
+
+then one can provide an arbitrary number of inputs after the `$` sign:


This looks like a leftover... should be replaced with "LIST_INPUTS"

alonre24 · 2021-05-04T07:52:05Z

docs/commands.md

+OK
+redis> AI.TENSORSET mytensor3{tag} FLOAT 1 VALUES 1
+OK
+redis> AI.SCRIPTRUN myscript{tag} addn keys 1 {tag} INPUTS 1 mytensor1{tag} LIST_INPUTS 2 mytensor2{tag} mytensor3{tag} OUTPUTS 1 result{tag}


ai.scriptexecute

alonre24 · 2021-05-04T07:58:55Z

docs/commands.md

+```
+
+### Redis Commands support.
+In RedisAI TorchScript now supports simple (non-blocking) Redis commnands via the `redis.execute` API. The following (usless) script gets a key name, an `int` value and sets it in a tensor. Note that the inputs are `str` and `int`. The script sets and gets the value and set it into a tensor.


Maybe rephrase the last sentence: "here, the script sets an integer value (3) under the key 'x{1}' and then gets this value and set it into a tensor which is eventually stored under the key 'y{1}'."

docs/commands.md

src/execution/execution_contexts/scriptRun_ctx.h

tests/module/LLAPI.c

tests/flow/tests_llapi.py

DvirDukhan changed the title ~~added function dictionary~~ script execute command Apr 17, 2021

DvirDukhan force-pushed the script_execute branch from d09d8b8 to cfe2222 Compare April 20, 2021 11:06

DvirDukhan mentioned this pull request Apr 28, 2021

Support SCRIPTRUN with variadic INPUTS RedisAI/JRedisAI#30

Closed

DvirDukhan marked this pull request as ready for review April 28, 2021 21:24

DvirDukhan requested review from alonre24 and lantiga April 28, 2021 21:24

DvirDukhan mentioned this pull request Apr 29, 2021

Update RedisAI API danitseitlin/redis-modules-sdk-ts#83

Closed

alonre24 reviewed May 1, 2021

View reviewed changes

DvirDukhan added 21 commits May 2, 2021 17:05

added function dictionary

f76ef0e

added script execute parsing. schema validation.

737e005

legacy tests pass

0310e2b

make format

0f5e542

fixed memory leak

57816a0

torch redis pass with new API

91d2dee

wip

664c637

wip

9145971

wip

62347e1

wip

01420ef

wip

03e9281

torch-redis tests pass

a53d6ec

wip

a4ee7a3

starting valgrind

ae4b0bd

valgrind pass

eb9802c

make format

cf13c59

added LITE check for getting script from keyspace

2e447c7

fixed typo

2dc8f46

added torch_c docs

22997c9

fixed script_struct.h

9fffa16

renamed otherInputs to nonTensorInputs

a802039

DvirDukhan added 9 commits May 2, 2021 17:05

fixed dictionaries comments

85debbb

fixed number of args on script keys validation

5545bf0

removed empty lines

86bf299

docs for RAI_DagOpSetRunKey

12bcbce

fixed script_command_parser comments

7dcb8bf

refactor RAI_HoldString

bd9911a

fixed scriptRun_ctx comments

c4150d6

fixed python tests comments

742f8fb

added multiple lists test

fef615c

DvirDukhan force-pushed the script_execute branch from 1707686 to fef615c Compare May 2, 2021 14:05

DvirDukhan added 2 commits May 4, 2021 00:21

llapi

7399de8

docs

2daf94d

alonre24 reviewed May 4, 2021

View reviewed changes

DvirDukhan added 5 commits May 4, 2021 16:33

fixed commands.md

65a083a

fixed script.c and scriptRun_Ctx.h

2b827f1

added comment about KEYS scope

a2a93ad

fixed LLAPI tests

8ac0e71

make format

7143e89

alonre24 previously approved these changes May 4, 2021

View reviewed changes

Merge branch 'master' into script_execute

5f5af5d

alonre24 added the ci-test label May 4, 2021

DvirDukhan added 2 commits May 4, 2021 19:09

Merge branch 'master' into script_execute

7b57f13

fixed valgrind

f7b870e

DvirDukhan dismissed alonre24’s stale review via f7b870e May 4, 2021 21:29

DvirDukhan added ci-test and removed ci-test labels May 4, 2021

alonre24 self-requested a review May 5, 2021 06:10

alonre24 approved these changes May 5, 2021

View reviewed changes

DvirDukhan merged commit 8a3461f into master May 5, 2021

DvirDukhan deleted the script_execute branch May 5, 2021 06:14

script execute command #682

script execute command #682

Uh oh!

Conversation

DvirDukhan commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

alonre24 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

DvirDukhan May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alonre24 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

DvirDukhan May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alonre24 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

alonre24 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

alonre24 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DvirDukhan commented Apr 13, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

DvirDukhan May 4, 2021 •

edited

Loading

DvirDukhan May 4, 2021 •

edited

Loading