Skip to content

Commit ef09f12

Browse files
alonre24alonre24
committed
PR fixes
1 parent ea8e972 commit ef09f12

File tree

12 files changed

+90
-83
lines changed

12 files changed

+90
-83
lines changed

‎src/DAG/dag.c‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -570,27 +570,24 @@ void RedisAI_DagRunSessionStep(RedisAI_RunInfo *rinfo, const char *devicestr) {
570570
}
571571

572572
if (currentOp->result != REDISMODULE_OK) {
573-
__atomic_store_n(rinfo->dagError, 1, __ATOMIC_RELAXED);
574-
RAI_ContextWriteLock(rinfo);
575-
RAI_SetError(rinfo->err, RAI_GetErrorCode(currentOp->err), RAI_GetError(currentOp->err));
576-
RAI_ContextUnlock(rinfo);
573+
// If this is the first op with error, save the error in the DAG runInfo.
574+
if (__sync_val_compare_and_swap(rinfo->dagError, 0, 1) == 0) {
575+
RAI_SetError(rinfo->err, RAI_GetErrorCode(currentOp->err),
576+
RAI_GetError(currentOp->err));
577+
}
577578
}
578579
}
579580

580581
void RedisAI_BatchedDagRunSessionStep(RedisAI_RunInfo **batched_rinfo, const char *devicestr) {
581582
// Assumption: ops are guaranteed to be all MODELRUN
582583

583584
int n_ops = array_len(batched_rinfo);
584-
585585
assert(n_ops > 1);
586-
587586
RAI_DagOp *currentOps[n_ops];
588587

589588
for (int i = 0; i < n_ops; i++) {
590589
RedisAI_RunInfo *rinfo = batched_rinfo[i];
591-
592590
RAI_DagOp *currentOp = RedisAI_DagCurrentOp(rinfo);
593-
594591
currentOps[i] = currentOp;
595592
}
596593

@@ -601,12 +598,13 @@ void RedisAI_BatchedDagRunSessionStep(RedisAI_RunInfo **batched_rinfo, const cha
601598
RAI_DagOp *currentOp = currentOps[i];
602599

603600
if (currentOp->result != REDISMODULE_OK) {
604-
__atomic_store_n(rinfo->dagError, 1, __ATOMIC_RELAXED);
605-
RAI_SetError(rinfo->err, RAI_GetErrorCode(currentOp->err),
606-
RAI_GetError(currentOp->err));
601+
// If this is the first op with error, save the error in the DAG runInfo.
602+
if (__sync_val_compare_and_swap(rinfo->dagError, 0, 1) == 0) {
603+
RAI_SetError(rinfo->err, RAI_GetErrorCode(currentOp->err),
604+
RAI_GetError(currentOp->err));
605+
}
607606
}
608607
}
609-
return;
610608
}
611609

612610
int RedisAI_DagRun_Reply(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {

‎src/DAG/dag_builder.c‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -157,18 +157,9 @@ int RAI_DAGAddOpsFromString(RAI_DAGRunCtx *run_info, const char *dag, RAI_Error
157157
}
158158

159159
if (ParseDAGOps(rinfo, new_ops) != REDISMODULE_OK) {
160-
// Remove all ops that where created.
161160
RAI_SetError(err, RAI_GetErrorCode(rinfo->err), RAI_GetError(rinfo->err));
162-
for (size_t i = 0; i < array_len(new_ops); i++) {
163-
RAI_FreeDagOp(new_ops[i]);
164-
}
165161
goto cleanup;
166162
}
167-
168-
// Copy the new op pointers to the DAG run info.
169-
for (size_t i = 0; i < array_len(new_ops); i++) {
170-
rinfo->dagOps = array_append(rinfo->dagOps, new_ops[i]);
171-
}
172163
rinfo->dagOpCount = array_len(rinfo->dagOps);
173164
res = REDISMODULE_OK;
174165

‎src/DAG/dag_execute.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ size_t RAI_DAGNumOutputs(RAI_OnFinishCtx *finish_ctx) {
266266
return n_outputs;
267267
}
268268

269-
RAI_Tensor *RAI_DAGOutputTensor(RAI_OnFinishCtx *finish_ctx, size_t index) {
269+
const RAI_Tensor *RAI_DAGOutputTensor(RAI_OnFinishCtx *finish_ctx, size_t index) {
270270
size_t tensor_get_op_ind = -1;
271271
RedisAI_RunInfo *rinfo = (RedisAI_RunInfo *)finish_ctx;
272272
for (size_t i = 0; i < rinfo->dagOpCount; i++) {
@@ -289,7 +289,7 @@ bool RAI_DAGRunError(RAI_OnFinishCtx *finish_ctx) {
289289
return *((RedisAI_RunInfo *)finish_ctx)->dagError;
290290
}
291291

292-
RAI_Error *RAI_DAGGetError(RAI_OnFinishCtx *finish_ctx) {
292+
const RAI_Error *RAI_DAGGetError(RAI_OnFinishCtx *finish_ctx) {
293293
RedisAI_RunInfo *rinfo = (RedisAI_RunInfo *)finish_ctx;
294294
return rinfo->err;
295295
}

‎src/DAG/dag_execute.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ size_t RAI_DAGNumOutputs(RAI_OnFinishCtx *finish_ctx);
4242
* @param index The index of the TENSORGET op in the DAG.
4343
* @retval returns the tensor that the i'th TENSORGET op outputs.
4444
*/
45-
RAI_Tensor *RAI_DAGOutputTensor(RAI_OnFinishCtx *finish_ctx, size_t index);
45+
const RAI_Tensor *RAI_DAGOutputTensor(RAI_OnFinishCtx *finish_ctx, size_t index);
4646

4747
/**
4848
* @brief Returns true if (at least) one of the DAG ops encountered an error.
@@ -55,4 +55,4 @@ bool RAI_DAGRunError(RAI_OnFinishCtx *finish_ctx);
5555
* @retval returns an object that represents the DAG status, from which a user can
5656
* obtain the error code (error code is "OK" if no error has occurred) and error details.
5757
*/
58-
RAI_Error *RAI_DAGGetError(RAI_OnFinishCtx *finish_ctx);
58+
const RAI_Error *RAI_DAGGetError(RAI_OnFinishCtx *finish_ctx);

‎src/DAG/dag_parser.c‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ int ParseDAGOps(RedisAI_RunInfo *rinfo, RAI_DagOp **ops) {
174174
currentOp->inkeys = array_append(currentOp->inkeys, currentOp->argv[1]);
175175
currentOp->fmt = ParseTensorGetArgs(rinfo->err, currentOp->argv, currentOp->argc);
176176
if (currentOp->fmt == TENSOR_NONE)
177-
return REDISMODULE_ERR;
177+
goto cleanup;
178178
continue;
179179
}
180180
if (!strcasecmp(arg_string, "AI.TENSORSET")) {
@@ -184,28 +184,40 @@ int ParseDAGOps(RedisAI_RunInfo *rinfo, RAI_DagOp **ops) {
184184
currentOp->outkeys = array_append(currentOp->outkeys, currentOp->argv[1]);
185185
if (RAI_parseTensorSetArgs(currentOp->argv, currentOp->argc, &currentOp->outTensor, 0,
186186
rinfo->err) == -1)
187-
return REDISMODULE_ERR;
187+
goto cleanup;
188188
continue;
189189
}
190190
if (!strcasecmp(arg_string, "AI.MODELRUN")) {
191191
if (ParseModelRunCommand(rinfo, currentOp, currentOp->argv, currentOp->argc) !=
192192
REDISMODULE_OK) {
193-
return REDISMODULE_ERR;
193+
goto cleanup;
194194
}
195195
continue;
196196
}
197197
if (!strcasecmp(arg_string, "AI.SCRIPTRUN")) {
198198
if (ParseScriptRunCommand(rinfo, currentOp, currentOp->argv, currentOp->argc) !=
199199
REDISMODULE_OK) {
200-
return REDISMODULE_ERR;
200+
goto cleanup;
201201
}
202202
continue;
203203
}
204204
// If none of the cases match, we have an invalid op.
205205
RAI_SetError(rinfo->err, RAI_EDAGBUILDER, "unsupported command within DAG");
206-
return REDISMODULE_ERR;
206+
goto cleanup;
207+
}
208+
209+
// After validating all the ops, insert them to the DAG.
210+
for (size_t i = 0; i < array_len(ops); i++) {
211+
rinfo->dagOps = array_append(rinfo->dagOps, ops[i]);
207212
}
213+
rinfo->dagOpCount = array_len(rinfo->dagOps);
208214
return REDISMODULE_OK;
215+
216+
cleanup:
217+
for (size_t i = 0; i < array_len(ops); i++) {
218+
RAI_FreeDagOp(ops[i]);
219+
}
220+
return REDISMODULE_ERR;
209221
}
210222

211223
int ParseDAGRunCommand(RedisAI_RunInfo *rinfo, RedisModuleCtx *ctx, RedisModuleString **argv,
@@ -292,16 +304,8 @@ int ParseDAGRunCommand(RedisAI_RunInfo *rinfo, RedisModuleCtx *ctx, RedisModuleS
292304
}
293305

294306
if (ParseDAGOps(rinfo, dag_ops) != REDISMODULE_OK) {
295-
for (size_t i = 0; i < array_len(dag_ops); i++) {
296-
RAI_FreeDagOp(dag_ops[i]);
297-
}
298307
goto cleanup;
299308
}
300-
// After validating all the ops, insert them to the DAG.
301-
for (size_t i = 0; i < array_len(dag_ops); i++) {
302-
rinfo->dagOps = array_append(rinfo->dagOps, dag_ops[i]);
303-
}
304-
rinfo->dagOpCount = array_len(rinfo->dagOps);
305309

306310
if (MangleTensorsNames(rinfo) != REDISMODULE_OK) {
307311
goto cleanup;

‎src/err.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@ const char *RAI_GetErrorOneLine(RAI_Error *err) { return err->detail_oneline; }
2929

3030
RAI_ErrorCode RAI_GetErrorCode(RAI_Error *err) { return err->code; }
3131

32+
void RAI_CloneError(RAI_Error *dest, const RAI_Error *src) {
33+
dest->code = src->code;
34+
RedisModule_Assert(!dest->detail);
35+
dest->detail = RedisModule_Strdup(src->detail);
36+
dest->detail_oneline = RAI_Chomp(dest->detail);
37+
}
38+
3239
void RAI_SetError(RAI_Error *err, RAI_ErrorCode code, const char *detail) {
3340
if (!err) {
3441
return;

‎src/err.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,14 @@ const char *RAI_GetErrorOneLine(RAI_Error *err);
8181
*/
8282
RAI_ErrorCode RAI_GetErrorCode(RAI_Error *err);
8383

84+
/**
85+
* Make dest a clone of src
86+
*
87+
* @param dest An allocated error
88+
* @param src The error to copy
89+
*/
90+
void RAI_CloneError(RAI_Error *dest, const RAI_Error *src);
91+
8492
/**
8593
* Resets an previously used/allocated RAI_Error
8694
*

‎src/redisai.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -953,7 +953,7 @@ static int RedisAI_RegisterApi(RedisModuleCtx *ctx) {
953953
REGISTER_API(GetError, ctx);
954954
REGISTER_API(GetErrorOneLine, ctx);
955955
REGISTER_API(GetErrorCode, ctx);
956-
REGISTER_API(SetError, ctx);
956+
REGISTER_API(CloneError, ctx);
957957

958958
REGISTER_API(TensorCreate, ctx);
959959
REGISTER_API(TensorCreateByConcatenatingTensors, ctx);

‎src/redisai.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ REDISAI_API void MODULE_API_FUNC(RedisAI_FreeError)(RAI_Error *err);
6767
REDISAI_API const char *MODULE_API_FUNC(RedisAI_GetError)(RAI_Error *err);
6868
REDISAI_API const char *MODULE_API_FUNC(RedisAI_GetErrorOneLine)(RAI_Error *err);
6969
REDISAI_API RedisAI_ErrorCode MODULE_API_FUNC(RedisAI_GetErrorCode)(RAI_Error *err);
70-
REDISAI_API void MODULE_API_FUNC(RedisAI_SetError)(RAI_Error *err, int code, const char *detail);
70+
REDISAI_API void MODULE_API_FUNC(RedisAI_CloneError)(RAI_Error *dest, const RAI_Error *src);
7171

7272
REDISAI_API RAI_Tensor *MODULE_API_FUNC(RedisAI_TensorCreate)(const char *dataTypeStr,
7373
long long *dims, int ndims);
@@ -183,10 +183,10 @@ REDISAI_API int MODULE_API_FUNC(RedisAI_DAGRun)(RAI_DAGRunCtx *run_info,
183183
RAI_OnFinishCB DAGAsyncFinish, void *private_data,
184184
RAI_Error *err);
185185
REDISAI_API size_t MODULE_API_FUNC(RedisAI_DAGNumOutputs)(RAI_OnFinishCtx *finish_ctx);
186-
REDISAI_API RAI_Tensor *MODULE_API_FUNC(RedisAI_DAGOutputTensor)(RAI_OnFinishCtx *finish_ctx,
187-
size_t index);
186+
REDISAI_API const RAI_Tensor *MODULE_API_FUNC(RedisAI_DAGOutputTensor)(RAI_OnFinishCtx *finish_ctx,
187+
size_t index);
188188
REDISAI_API int MODULE_API_FUNC(RedisAI_DAGRunError)(RAI_OnFinishCtx *finish_ctx);
189-
REDISAI_API RAI_Error *MODULE_API_FUNC(RedisAI_DAGGetError)(RAI_OnFinishCtx *finish_ctx);
189+
REDISAI_API const RAI_Error *MODULE_API_FUNC(RedisAI_DAGGetError)(RAI_OnFinishCtx *finish_ctx);
190190
REDISAI_API void MODULE_API_FUNC(RedisAI_DAGRunOpFree)(RAI_DAGRunOp *dagOp);
191191
REDISAI_API void MODULE_API_FUNC(RedisAI_DAGFree)(RAI_DAGRunCtx *run_info);
192192

@@ -224,7 +224,7 @@ static int RedisAI_Initialize(RedisModuleCtx *ctx) {
224224
REDISAI_MODULE_INIT_FUNCTION(ctx, GetError);
225225
REDISAI_MODULE_INIT_FUNCTION(ctx, GetErrorOneLine);
226226
REDISAI_MODULE_INIT_FUNCTION(ctx, GetErrorCode);
227-
REDISAI_MODULE_INIT_FUNCTION(ctx, SetError);
227+
REDISAI_MODULE_INIT_FUNCTION(ctx, CloneError);
228228

229229
REDISAI_MODULE_INIT_FUNCTION(ctx, GetLLAPIVersion);
230230

‎tests/module/DAG_utils.c‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,8 @@ static void _DAGFinishFunc(RAI_OnFinishCtx *onFinishCtx, void *private_data) {
5454

5555
RAI_RunResults *results = (RAI_RunResults *)private_data;
5656
if (RedisAI_DAGRunError(onFinishCtx)) {
57-
for (size_t i = 0; i < RedisAI_DAGNumOps(onFinishCtx); i++) {
58-
RAI_Error *error = RedisAI_DAGGetError(onFinishCtx);
59-
RedisAI_SetError(results->error, RedisAI_GetErrorCode(error), RedisAI_GetError(error));
60-
}
57+
const RAI_Error *error = RedisAI_DAGGetError(onFinishCtx);
58+
RedisAI_CloneError(results->error, error);
6159
pthread_cond_signal(&global_cond);
6260
return;
6361
}
@@ -74,10 +72,12 @@ static void _DAGFinishFunc(RAI_OnFinishCtx *onFinishCtx, void *private_data) {
7472
pthread_cond_signal(&global_cond);
7573
}
7674

77-
int testModelRunOpError(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
75+
int testModelRunOpError(RedisModuleCtx *ctx) {
7876

77+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
7978
RAI_Error *err;
8079
RedisAI_InitError(&err);
80+
int res = LLAPIMODULE_ERR;
8181
// The model m{1} should exist in key space.
8282
RAI_Model *model = (RAI_Model *)_getFromKeySpace(ctx, "m{1}");
8383
RAI_DAGRunOp *op = RedisAI_DAGCreateModelRunOp(model);
@@ -86,25 +86,28 @@ int testModelRunOpError(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
8686
// This model expect for 2 inputs not 1.
8787
int status = RedisAI_DAGAddRunOp(run_info, op, err);
8888
if (!_assertError(err, status, "Number of keys given as INPUTS does not match model definition")) {
89-
RedisAI_FreeError(err);
90-
return LLAPIMODULE_ERR;
89+
goto cleanup;
9190
}
9291
RedisAI_ClearError(err);
9392
RedisAI_DAGRunOpAddInput(op, "second_input");
9493
status = RedisAI_DAGAddRunOp(run_info, op, err);
9594

9695
// We still get an error since the model expects for an output as well.
9796
if (!_assertError(err, status, "Number of keys given as OUTPUTS does not match model definition")) {
98-
RedisAI_FreeError(err);
99-
return LLAPIMODULE_ERR;
97+
goto cleanup;
10098
}
99+
res = LLAPIMODULE_OK;
100+
101+
cleanup:
101102
RedisAI_FreeError(err);
102103
RedisAI_DAGRunOpFree(op);
103-
return LLAPIMODULE_OK;
104+
RedisAI_DAGFree(run_info);
105+
return res;
104106
}
105107

106-
int testEmptyDAGError(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
108+
int testEmptyDAGError(RedisModuleCtx *ctx) {
107109

110+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
108111
RAI_Error* err;
109112
RedisAI_InitError(&err);
110113
int res = LLAPIMODULE_ERR;
@@ -124,8 +127,9 @@ int testEmptyDAGError(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
124127
return res;
125128
}
126129

127-
int testKeysMismatchError(RedisModuleCtx *ctx,RAI_DAGRunCtx *run_info) {
130+
int testKeysMismatchError(RedisModuleCtx *ctx) {
128131

132+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
129133
RAI_Error* err;
130134
RedisAI_InitError(&err);
131135
int res = LLAPIMODULE_ERR;
@@ -146,8 +150,9 @@ int testKeysMismatchError(RedisModuleCtx *ctx,RAI_DAGRunCtx *run_info) {
146150
return res;
147151
}
148152

149-
int testBuildDAGFromString(RedisModuleCtx *ctx,RAI_DAGRunCtx *run_info) {
153+
int testBuildDAGFromString(RedisModuleCtx *ctx) {
150154

155+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
151156
RAI_RunResults results;
152157
_InitRunResults(&results);
153158
int res = LLAPIMODULE_ERR;
@@ -200,8 +205,9 @@ int testBuildDAGFromString(RedisModuleCtx *ctx,RAI_DAGRunCtx *run_info) {
200205
return res;
201206
}
202207

203-
int testSimpleDAGRun(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
208+
int testSimpleDAGRun(RedisModuleCtx *ctx) {
204209

210+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
205211
RAI_RunResults results;
206212
_InitRunResults(&results);
207213
int res = LLAPIMODULE_ERR;
@@ -252,8 +258,9 @@ int testSimpleDAGRun(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
252258
return res;
253259
}
254260

255-
int testSimpleDAGRun2(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
261+
int testSimpleDAGRun2(RedisModuleCtx *ctx) {
256262

263+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
257264
RAI_RunResults results;
258265
_InitRunResults(&results);
259266
int res = LLAPIMODULE_ERR;
@@ -304,8 +311,9 @@ int testSimpleDAGRun2(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
304311
return res;
305312
}
306313

307-
int testSimpleDAGRun2Error(RedisModuleCtx *ctx, RAI_DAGRunCtx *run_info) {
314+
int testSimpleDAGRun2Error(RedisModuleCtx *ctx) {
308315

316+
RAI_DAGRunCtx *run_info = RedisAI_DAGRunCtxCreate();
309317
RAI_RunResults results;
310318
_InitRunResults(&results);
311319
int res = LLAPIMODULE_ERR;

0 commit comments

Comments
 (0)