Improve script API for RedisAI 1.2 #792

DvirDukhan · 2021-06-18T19:58:41Z

This PR introduces

AI.SCRIPTSTORE command which will replace AI.SCRIPTSET. In the new command, user will have to specify entry points for the script, all with the signature def entry_point(tensors: List[Tensor], keys: List[Str], args: List[Str]) , so the AI.SCRIPTSTORE command will look like AI.SCRIPTSTORE <key> ENTRY_POINTS <n_ep> <ep_1> ... <ep_n> SOURCE <script>
Modifications to the new AI.SCRIPTEXECUTE command which introduced a new but awkward interface. In the new modification the user will have to specify either INPUTS which are RedisAI tensors (either from keyspace or from DAG execution context), KEYS which are the keys that the function will touch during its execution (by calling redis.Execute command in the function, and ARGS which are strings representing any value the user needs. Those values should be cast later by the user in the script itself.

codecov · 2021-06-21T22:25:33Z

Codecov Report

Merging #792 (a3c5ed8) into master (c9f0198) will increase coverage by 0.02%.
The diff coverage is 80.70%.

@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
+ Coverage   79.92%   79.95%   +0.02%     
==========================================
  Files          52       53       +1     
  Lines        7767     8035     +268     
==========================================
+ Hits         6208     6424     +216     
- Misses       1559     1611      +52

Impacted Files	Coverage Δ
src/redisai.h	`96.34% <ø> (-0.33%)`	⬇️
src/serialization/AOF/rai_aof_rewrite.c	`0.00% <0.00%> (ø)`
.../serialization/RDB/decoder/previous/v2/decode_v2.c	`36.74% <0.00%> (ø)`
src/serialization/RDB/decoder/decode_previous.c	`66.66% <33.33%> (-8.34%)`	⬇️
src/backends/libtflite_c/tflite_c.cpp	`58.16% <50.00%> (+0.56%)`	⬆️
...c/serialization/RDB/decoder/current/v3/decode_v3.c	`68.13% <68.13%> (ø)`
src/execution/parsing/script_commands_parser.c	`77.12% <77.87%> (+1.67%)`	⬆️
src/execution/parsing/deprecated.c	`81.38% <78.84%> (-0.49%)`	⬇️
src/execution/parsing/dag_parser.c	`96.00% <81.81%> (-0.45%)`	⬇️
...ckends/libtorch_c/torch_extensions/torch_redis.cpp	`94.73% <96.87%> (+10.29%)`	⬆️
... and 28 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 a472c90...a3c5ed8. Read the comment docs.

alonre24

Looks good, much cleaner logic for executing scripts! I added many comments about missing tests (sorry...), and I saw that many tests were disabled (most of them in tests_deprecated_commands.py), for a reason that I couldn't understand...

docs/commands.md

src/backends/torch.c

tests/flow/tests_dag.py

tests/flow/tests_pytorch.py

alonre24 · 2021-06-22T15:21:03Z

tests/flow/tests_pytorch.py

    if not TEST_PT:
        env.debugPrint("skipping {} since TEST_PT=0".format(sys._getframe().f_code.co_name), force=True)
        return

    con = env.getConnection()

-    check_error(env, con, 'AI.SCRIPTSET', 'ket{1}', DEVICE, 'SOURCE', 'return 1')
+    check_error(env, con, 'AI.SCRIPTSTORE', 'ket{1}', DEVICE, 'SOURCE', 'return 1')


Why don't we check for the error message in these 4 tests?

tests/flow/tests_deprecated_commands.py

alonre24

Added few more comments. Also, there are still some parsing errors in script execute that are not tested. I'm suppose to talk with Meir on Sunday about making the KEYS not mandatory, then we could close this PR...
One last thing - the aof issue. I see that we are still not covering aof rewrite which is very strange. I will take a look and fix it a new PR

alonre24 · 2021-06-25T13:25:13Z

docs/commands.md

 ## AI.SCRIPTSET
+_This command is deprecated and will not be available in future versions. consider using AI.MODELSTORE command instead._


consider using AI.SCRIPTSTORE command instead._

alonre24 · 2021-06-25T14:20:11Z

src/backends/libtorch_c/torch_c.cpp

+    return function->getSchema().arguments().size();
+}
+
+extern "C" TorchScriptFunctionArgumentType torchScript_FunctionArgumentType(void* scriptCtx, size_t fn_index, size_t arg_index) {


Does this function still needed now that we have torchScript_FunctionArgumentTypeByFunctionName?

alonre24 · 2021-06-25T14:26:33Z

src/backends/libtorch_c/torch_c.cpp

@@ -687,8 +593,27 @@ extern "C" size_t torchScript_FunctionArgumentCount(void* scriptCtx, size_t fn_i
    return functions[fn_index]->getSchema().arguments().size();
 }


DO we still need torchScript_FunctionArgumentCount now that we have torchScript_FunctionArgumentCountByFunctionName?

src/execution/parsing/deprecated.c

alonre24 · 2021-06-25T14:31:07Z

src/redisai.c

+    for (size_t i = 0; i < nEntryPoints; i++) {
+        const char *entryPoint;
+        if (AC_GetString(&ac, &entryPoint, NULL, 0) != AC_OK) {
+            return RedisModule_ReplyWithError(


alonre24 · 2021-06-25T14:41:01Z

tests/flow/tests_commands.py

-    check_error_message(env, con, "Already encountered KEYS scope in current command",
-                        "AI.DAGEXECUTE KEYS 1 a{1} |> AI.SCRIPTEXECUTE script{1} bar KEYS 1 a{1}")
+    # # ERR KEYS section in an inner AI.SCRIPTEXEUTE command within a DAG is not allowed.
+    # check_error_message(env, con, "Already encountered KEYS scope in current command",


Right... ok let's leave it open, I'll discuss it with Meir on Sunday

alonre24 · 2021-06-25T14:45:24Z

tests/flow/tests_commands.py

-                        "AI.DAGEXECUTE KEYS 1 a{1} |> AI.SCRIPTEXECUTE script{1} bar KEYS 1 a{1}")
+    # # ERR KEYS section in an inner AI.SCRIPTEXEUTE command within a DAG is not allowed.
+    # check_error_message(env, con, "Already encountered KEYS scope in current command",
+    #                     "AI.DAGEXECUTE KEYS 1 a{1} |> AI.SCRIPTEXECUTE script{1} bar KEYS 1 a{1}")


 # Todo: this test should change once the script store command is implemented.


I meant that the #todo comment is no longer needed since script store is now implemented. Also, I think that the tests for AI.SCRIPTSTORE syntax (i.e., tests that catches errors in the RedisAI parsing phase, before we call torch.CompileScript) should be in this file, as we did for similar tests of the AI.MODELSTORE command.

alonre24 · 2021-06-25T14:57:44Z

src/execution/parsing/script_commands_parser.c

+    if (localArgpos >= argc) {
+        RAI_SetError(error, RAI_ESCRIPTRUN, "ERR Invalid arguments provided to AI.SCRIPTEXECUTE");
+        return REDISMODULE_ERR;
+    }


And if there is any difference, these errors are not tested....

src/execution/parsing/script_commands_parser.c

alonre24

Nice! final 3 comments... and lets merge it!

docs/commands.md

tests/flow/test_torchscript_extensions.py

DvirDukhan requested review from alonre24 and lantiga June 21, 2021 21:52

DvirDukhan marked this pull request as ready for review June 21, 2021 21:53

alonre24 reviewed Jun 22, 2021

View reviewed changes

alonre24 reviewed Jun 25, 2021

View reviewed changes

DvirDukhan added 17 commits July 4, 2021 14:47

added torch compile script, with enty points

9eaed53

after format

61d0fa9

modelstore command

80b5451

wip

b4f12f9

main tests pass

aab37ca

added support for deprecated commands

d408d53

fixed LLAPI test. format

4003213

wip aof-rdb

0805ea0

aof

496494d

aof, rdb

ac14556

valgrind leak

c3e6473

docs

92084f9

re-export of v3 rdb

34dee5e

fixed flaky test

6632ed5

restored deprecated tests

1fab76c

fixing PR comments. WIP

6b4ea89

fixed PR comments

6de3a1c

DvirDukhan force-pushed the fix_script_api_for_1.2 branch from 4e67931 to 6de3a1c Compare July 4, 2021 11:53

DvirDukhan added 2 commits July 4, 2021 15:43

fixed PR comments

272a47a

added DAG ROUTING keyword

fc20b9b

alonre24 reviewed Jul 4, 2021

View reviewed changes

docs/commands.md Show resolved Hide resolved

docs/commands.md Outdated Show resolved Hide resolved

tests/flow/test_torchscript_extensions.py Outdated Show resolved Hide resolved

DvirDukhan added 2 commits July 4, 2021 20:36

fixed PR comments

ee232d0

fixed tensor decode assert

a3c5ed8

DvirDukhan added the ci-test label Jul 4, 2021

alonre24 approved these changes Jul 4, 2021

View reviewed changes

DvirDukhan merged commit e954d21 into master Jul 4, 2021

DvirDukhan deleted the fix_script_api_for_1.2 branch July 4, 2021 21:34

		## AI.SCRIPTSET
		_This command is deprecated and will not be available in future versions. consider using AI.MODELSTORE command instead._

		@@ -687,8 +593,27 @@ extern "C" size_t torchScript_FunctionArgumentCount(void* scriptCtx, size_t fn_i
		return functions[fn_index]->getSchema().arguments().size();
		}

Improve script API for RedisAI 1.2 #792

Improve script API for RedisAI 1.2 #792

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Jun 21, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alonre24 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DvirDukhan commented Jun 18, 2021 •

edited

Loading

codecov bot commented Jun 21, 2021 •

edited

Loading

alonre24 left a comment •

edited

Loading