-
Notifications
You must be signed in to change notification settings - Fork 106
Set default backends path #924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5f400f0
b7b7060
9d454c2
8b9f3aa
a6d5f03
77a2a27
7549768
ec1b2ff
fcec66d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the addition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To use this constant in |
||
|
||
/** | ||
* Get number of threads used for parallelism between independent operations, by | ||
* backend. | ||
|
@@ -81,9 +83,8 @@ char *Config_GetBackendsPath(void); | |
int Config_LoadBackend(RedisModuleCtx *ctx, RedisModuleString **argv, int argc); | ||
|
||
/** | ||
* Helper method for AI.CONFIG BACKENDSPATH | ||
* <default_location_of_backend_libraries> | ||
* @param path string containing backend path | ||
* Helper method for AI.CONFIG BACKENDSPATH <default_location_of_backend_libraries> | ||
* @param path string containing backend path. | ||
*/ | ||
void Config_SetBackendsPath(const char *path); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1279,6 +1279,11 @@ void RAI_moduleInfoFunc(RedisModuleInfoCtx *ctx, int for_crash_report) { | |
AI_dictReleaseIterator(iter); | ||
} | ||
|
||
void RAI_CleanupModule(RedisModuleCtx *ctx, RedisModuleEvent eid, uint64_t subevent, void *data) { | ||
RedisModule_Log(ctx, "notice", "%s", "Clearing resources on shutdown"); | ||
RedisModule_Free(Config_GetBackendsPath()); | ||
} | ||
|
||
int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { | ||
|
||
#ifndef REDISAI_LITE | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the minimal redis version supporting this event? does it align with our min_redis_version on our ramp files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this API is supported from version 6.0.0, which is the minimal version for RedisAI anyway.... |
||
|
||
if (Config_SetLoadTimeParams(ctx, argv, argc) != REDISMODULE_OK) { | ||
return REDISMODULE_ERR; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. didn't you remove RLTest parallelism? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes... But I don't think that this addition is bad, as we might wan't to use the parallelism in the future. WDYT? |
||
redis_server = subprocess.Popen(['redis-server', '--port', str(port), | ||
'--loadmodule', f'{ROOT}/install-{DEVICE.lower()}/redisai.so', | ||
'--logfile', f'{self.env.logDir}/test_onnx_kill_switch_synchronization-{port}.log', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RedisModule_CreateStringPrintf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that better? We don't use
HoldString
or something here (so I can't see why usingRedisModuleString
object has an advantage...)