Skip to content

Model run refactor and support async LLAPI #537

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Dec 23, 2020
Merged

Model run refactor and support async LLAPI #537

merged 18 commits into from
Dec 23, 2020

Conversation

alonre24
Copy link
Collaborator

No description provided.

@alonre24 alonre24 requested a review from DvirDukhan December 14, 2020 14:31
@alonre24 alonre24 self-assigned this Dec 14, 2020
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from fe5cd80 to 72fe75f Compare December 14, 2020 14:32
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from 769b65e to a2c2d45 Compare December 14, 2020 15:55
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from 4f0c05b to ae48edc Compare December 14, 2020 22:02
@codecov
Copy link

codecov bot commented Dec 14, 2020 •
edited
Loading

Codecov Report

Merging #537 (fa2a334) into master (e06c663) will increase coverage by 0.38%.
The diff coverage is 87.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
+ Coverage   75.35%   75.74%   +0.38%     
==========================================
  Files          22       25       +3     
  Lines        5226     5384     +158     
==========================================
+ Hits         3938     4078     +140     
- Misses       1288     1306      +18
Impacted Files Coverage Δ
src/background_workers.c 87.43% <ø> (ø)
src/redisai.h 0.00% <0.00%> (ø)
src/err.c 95.34% <50.00%> (ø)
src/tensor.c 82.76% <60.00%> (+0.02%) ⬆️
src/run_info.c 69.74% <81.81%> (+0.55%) ⬆️
src/model.c 69.45% <82.35%> (-2.18%) ⬇️
src/modelRun_ctx.c 82.60% <82.60%> (ø)
src/DAG/dag.c 86.27% <86.27%> (ø)
src/DAG/dag_parser.c 89.64% <89.64%> (ø)
src/command_parser.c 90.69% <90.69%> (ø)
... and 8 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 e06c663...fa2a334. Read the comment docs.

@alonre24
Refactor - Create a function that parses all run commands in command …
9624857 
…parser file. The parsing is split according to the original command, and at the end we always create run info and sent it to queues.
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from 9cf5b23 to 9624857 Compare December 15, 2020 13:52
DvirDukhan
DvirDukhan reviewed Dec 16, 2020
View reviewed changes
src/DAG/dag.c Outdated
Comment on lines 626 to 627
if (tensor)
StoreTensorInKeySpace(ctx, tensor, key_string, mangled);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which situations we will have NULL here and why we continue without errors?

@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from c1e8517 to 79f7a0d Compare December 20, 2020 08:42
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch 4 times, most recently from a756fed to acd7c8a Compare December 20, 2020 13:06
@alonre24 alonre24 force-pushed the Model_run_refactor_and_support_async_LLAPI branch from acd7c8a to 2b539d4 Compare December 20, 2020 13:06
DvirDukhan
DvirDukhan reviewed Dec 21, 2020
View reviewed changes
src/run_info.c Outdated
Comment on lines 349 to 351
RAI_DagOp *op = rinfo->dagOps[0];
RAI_SetError(err, RAI_GetErrorCode(op->err), RAI_GetError(op->err));
RAI_ModelRunCtx *mctx = op->mctx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if user do this operation over a DAG which is not modelrun?
Please validate that mctx is actually model run context and return NULL in case it doesn't.

also, why duplicate the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User should not call this in a DAGRUN OnFinish CB, but only in a MODELRUN. I can set this in the error msg.
The error is not duplicated, we copy from the runInfo to the RAI_Error that the user sent and then we remove the runInfo (which contains the error in its first op, but not in the mctx).

DvirDukhan
DvirDukhan approved these changes Dec 23, 2020
View reviewed changes
src/DAG/dag_parser.c
Comment on lines +382 to +385
char buf[16];
sprintf(buf, "%04d", *instance);
RedisModuleString *mangled_key = RedisModule_CreateStringFromString(NULL, key);
RedisModule_StringAppendBuffer(NULL, mangled_key, buf, strlen(buf));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is duplicated 4 times. funciton?

@alonre24 alonre24 merged commit 3f9d4f0 into master Dec 23, 2020
@alonre24 alonre24 deleted the Model_run_refactor_and_support_async_LLAPI branch December 23, 2020 12:42
@DvirDukhan DvirDukhan mentioned this pull request Dec 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DvirDukhan DvirDukhan DvirDukhan approved these changes

Assignees

@alonre24 alonre24

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alonre24 @DvirDukhan