Set default backends path #924

alonre24 · 2022-06-02T11:42:44Z

Set the backends' default path on module load, so it can be retrieved with AI.CONFIG GET command. Also - pass the module's path to flow tests.

… ai.config get command.

…arallelism + fix in onnx test to adjust parallelism.

… limiting its size

codecov · 2022-06-07T14:57:26Z

Codecov Report

Merging #924 (7549768) into master (2dd4ef1) will increase coverage by 0.46%.
The diff coverage is 96.57%.

❗ Current head 7549768 differs from pull request most recent head fcec66d. Consider uploading reports for the commit fcec66d to get more accurate results

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
+ Coverage   81.30%   81.77%   +0.46%     
==========================================
  Files          54       54              
  Lines        8201     8214      +13     
==========================================
+ Hits         6668     6717      +49     
+ Misses       1533     1497      -36

Impacted Files	Coverage Δ
src/execution/DAG/dag_builder.c	`100.00% <ø> (ø)`
src/execution/DAG/dag_op.c	`100.00% <ø> (ø)`
src/execution/parsing/script_commands_parser.c	`78.57% <ø> (-0.12%)`	⬇️
.../serialization/RDB/decoder/previous/v2/decode_v2.c	`46.37% <50.00%> (+0.45%)`	⬆️
tests/module/LLAPI.c	`75.89% <80.00%> (-0.02%)`	⬇️
src/execution/background_workers.c	`94.63% <95.23%> (+0.01%)`	⬆️
src/redisai.c	`89.27% <97.14%> (+1.47%)`	⬆️
src/backends/backends.c	`78.57% <100.00%> (+0.53%)`	⬆️
...backends/libtorch_c/torch_extensions/torch_redis.h	`90.90% <100.00%> (ø)`
src/backends/tensorflow.c	`72.43% <100.00%> (+0.06%)`	⬆️
... and 18 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 5e4517d...fcec66d. Read the comment docs.

…ome configs (COV + slaves)

DvirDukhan · 2022-06-07T16:21:43Z

src/backends/backends.c

+    size_t backends_default_path_len = strlen(dyn_lib_dir_name) + strlen("/backends");
+    *backends_path = RedisModule_Alloc(backends_default_path_len + 1);
+    RedisModule_Assert(sprintf(*backends_path, "%s/backends", dyn_lib_dir_name) > 0);


RedisModule_CreateStringPrintf?

Is that better? We don't use HoldString or something here (so I can't see why using RedisModuleString object has an advantage...)

DvirDukhan · 2022-06-07T18:29:16Z

src/config/config.c

@@ -150,7 +149,7 @@ int Config_SetIntraOperationParallelism(RedisModuleString *num_threads_string) {
 int Config_SetModelChunkSize(RedisModuleString *chunk_size_string) {
    long long val;
    int result = RedisModule_StringToLongLong(chunk_size_string, &val);
-    if (result != REDISMODULE_OK || val <= 0) {
+    if (result != REDISMODULE_OK || val <= 0 || val > REDISAI_DEFAULT_MODEL_CHUNK_SIZE) {


why the addition of this condition?

We need to limit the chunk_size, since Redis cannot reply with blobs that are larger than 512MB, right?

redis/redis-doc#1653 (comment)
see here. You can configure the server to support bigger values

DvirDukhan · 2022-06-07T18:29:47Z

src/config/config.h

@@ -28,6 +28,8 @@ typedef enum { RAI_DEVICE_CPU = 0, RAI_DEVICE_GPU = 1 } RAI_Device;
 #define REDISAI_INFOMSG_MODEL_EXECUTION_TIMEOUT "Setting MODEL_EXECUTION_TIMEOUT parameter to"
 #define REDISAI_INFOMSG_BACKEND_MEMORY_LIMIT    "Setting BACKEND_MEMORY_LIMIT parameter to"

+#define REDISAI_DEFAULT_MODEL_CHUNK_SIZE (511 * 1024 * 1024)


why the addition?

To use this constant in Config_SetModelChunkSize

DvirDukhan · 2022-06-07T18:30:42Z

src/redisai.c

@@ -1424,6 +1429,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)

    RedisModule_SetModuleOptions(ctx, REDISMODULE_OPTIONS_HANDLE_IO_ERRORS);

+    RedisModule_SubscribeToServerEvent(ctx, RedisModuleEvent_Shutdown, RAI_CleanupModule);


what is the minimal redis version supporting this event? does it align with our min_redis_version on our ramp files?

yes, this API is supported from version 6.0.0, which is the minimal version for RedisAI anyway....

DvirDukhan · 2022-06-07T18:31:25Z

tests/flow/tests_onnx.py

@@ -567,7 +567,7 @@ def test_synchronization(self):

        def launch_redis_and_run_onnx(con, proc_id, pipes):
            my_pipe = pipes[proc_id]
-            port = 6380 + proc_id  # Let every subprocess run on a fresh port.
+            port = 6380 + 30*proc_id  # Let every subprocess run on a fresh port (safe distance for RLTEST parallelism).


didn't you remove RLTest parallelism?

Yes... But I don't think that this addition is bad, as we might wan't to use the parallelism in the future. WDYT?

alonre24 added 2 commits June 2, 2022 14:38

Set backends default path on module load, so it can be retrieved with…

5f400f0

… ai.config get command.

Merge branch 'master' into set_default_backends_path

b7b7060

alonre24 requested a review from DvirDukhan June 2, 2022 11:42

alonre24 marked this pull request as draft June 2, 2022 14:27

alonre24 added 2 commits June 2, 2022 20:30

Limit the backends' path length + fix in script that rns tests with p…

9d454c2

…arallelism + fix in onnx test to adjust parallelism.

Merge branch 'master' into set_default_backends_path

8b9f3aa

alonre24 marked this pull request as ready for review June 2, 2022 17:31

alonre24 added 2 commits June 3, 2022 11:21

Update docs

a6d5f03

Use server shutdown event to clean up global backends path instead of…

77a2a27

… limiting its size

Remove parallelism from tests, since it may cause problem in CI for s…

7549768

…ome configs (COV + slaves)

alonre24 added the ci-test label Jun 7, 2022

DvirDukhan reviewed Jun 7, 2022

View reviewed changes

Remove chunk_size limitation.

ec1b2ff

DvirDukhan approved these changes Jun 9, 2022

View reviewed changes

Merge branch 'master' into set_default_backends_path

fcec66d

alonre24 added ci-test and removed ci-test labels Jun 9, 2022

alonre24 merged commit 1ea457a into master Jun 9, 2022

alonre24 deleted the set_default_backends_path branch June 9, 2022 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set default backends path #924

Set default backends path #924

Uh oh!

alonre24 commented Jun 2, 2022 •

edited

Loading

Uh oh!

codecov bot commented Jun 7, 2022 •

edited

Loading

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

alonre24 Jun 7, 2022

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

alonre24 Jun 7, 2022

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

alonre24 Jun 7, 2022

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

alonre24 Jun 7, 2022

Uh oh!

DvirDukhan Jun 7, 2022

Uh oh!

alonre24 Jun 7, 2022

Uh oh!

Uh oh!

		@@ -1424,6 +1429,8 @@ int RedisModule_OnLoad(RedisModuleCtx ctx, RedisModuleString *argv, int argc)

		RedisModule_SetModuleOptions(ctx, REDISMODULE_OPTIONS_HANDLE_IO_ERRORS);

		RedisModule_SubscribeToServerEvent(ctx, RedisModuleEvent_Shutdown, RAI_CleanupModule);

Set default backends path #924

Set default backends path #924

Uh oh!

Conversation

alonre24 commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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 commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading