Skip to content

expose model inputs and outputs with respect to model definition #552

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 9 commits into from
Jan 13, 2021

Conversation

DvirDukhan
Copy link
Collaborator

@DvirDukhan DvirDukhan commented Jan 3, 2021

closes #529

@DvirDukhan DvirDukhan requested review from lantiga and alonre24 January 3, 2021 19:54
@codecov
Copy link

codecov bot commented Jan 3, 2021

Codecov Report

Merging #552 (21b39cc) into master (bd9f462) will decrease coverage by 0.42%.
The diff coverage is 71.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
- Coverage   72.57%   72.14%   -0.43%     
==========================================
  Files          35       35              
  Lines        5626     5824     +198     
==========================================
+ Hits         4083     4202     +119     
- Misses       1543     1622      +79     
Impacted Files Coverage Δ
src/model.c 77.64% <ø> (-0.26%) ⬇️
src/backends/tflite.c 63.81% <60.86%> (-4.71%) ⬇️
src/backends/onnxruntime.c 60.75% <72.09%> (+0.57%) ⬆️
src/libtflite_c/tflite_c.cpp 58.17% <73.33%> (-0.82%) ⬇️
src/libtorch_c/torch_c.cpp 57.52% <74.35%> (+2.18%) ⬆️
src/backends/torch.c 69.86% <75.51%> (+0.37%) ⬆️
src/backends/tensorflow.c 68.82% <100.00%> (+0.17%) ⬆️
src/command_parser.c 86.66% <100.00%> (-2.06%) ⬇️
... and 3 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 bd9f462...21b39cc. Read the comment docs.

@@ -71,15 +71,13 @@ static int _ModelRunCommand_ParseArgs(RedisModuleCtx *ctx, RedisModuleString **a
}
if ((*model)->inputs && (*model)->ninputs != ninputs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the condition (*model)->inputs is still needed? The inputs and outputs arrays should be allocated in all backends now (not only in TF) right?

@lantiga
Copy link
Contributor

lantiga commented Jan 12, 2021

Looks great, the failing tests should be just a matter of updating the tests themselves.
I would remove the condition as @alonre24 pointed out.

Copy link
Contributor

@lantiga lantiga left a comment

Choose a reason for hiding this comment

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

LGTM

@DvirDukhan DvirDukhan merged commit 2fc1827 into master Jan 13, 2021
@DvirDukhan DvirDukhan deleted the general_model_inputs_and_outputs_names branch January 13, 2021 13:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose model inputs and outputs number in AI.MODELGET
3 participants