Skip to content

Conversation

@filipecosta90
Copy link
Collaborator

@filipecosta90 filipecosta90 commented Apr 9, 2020

Current WIP

  • [add] simplified argument processing on RedisAI_ModelRun_RedisCommand (only 1 full-loop vs 5)
  • [add] code refactoring to reduce duplicates and reduce complexity.
  • [add] enforced error reply string comparison on tests.
  • [fix] error messages consistent across commands for tensor getting, etc...
  • removes usage of RedisModule_AutoMemory(ctx) from RedisAI_TensorGet_RedisCommand, RedisAI_ModelSet_RedisCommand, RedisAI_ModelGet_RedisCommand, RedisAI_ModelDel_RedisCommand, RedisAI_ModelRun_RedisCommand, RedisAI_ScriptDel_RedisCommand
  • moves queue from redisai.c to util/queue.c

…dd] enforced error reply string comparison on tests. [fix] error messages consistent across commands for tensor getting, etc...
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #321 into master will increase coverage by 0.14%.
The diff coverage is 82.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
+ Coverage   56.81%   56.96%   +0.14%     
==========================================
  Files          26       27       +1     
  Lines        5155     5080      -75     
==========================================
- Hits         2929     2894      -35     
+ Misses       2226     2186      -40     
Impacted Files Coverage Δ
src/backends/tflite.c 70.27% <0.00%> (ø)
src/backends/torch.c 85.33% <0.00%> (+2.00%) ⬆️
src/err.c 91.66% <0.00%> (ø)
src/redisai.h 0.00% <ø> (ø)
src/backends/onnxruntime.c 68.43% <16.66%> (ø)
src/model.c 69.79% <21.05%> (ø)
src/backends/tensorflow.c 70.68% <25.00%> (ø)
src/script.c 60.18% <33.33%> (ø)
src/util/queue.c 83.01% <83.01%> (ø)
src/redisai.c 77.30% <86.04%> (-0.03%) ⬇️
... and 4 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 4ec5163...dbe0ce0. Read the comment docs.

@filipecosta90 filipecosta90 requested a review from lantiga April 9, 2020 23:42
@filipecosta90 filipecosta90 changed the title [WIP] RedisAI_ModelRun_RedisCommand optimizations [WIP] Code refactoring, consistent error messages, and RedisAI_ModelRun_RedisCommand optimizations Apr 9, 2020
… [add] quick refactoring to have consistent command description on redisai.c
@filipecosta90 filipecosta90 requested a review from lantiga April 10, 2020 14:46
@filipecosta90 filipecosta90 changed the title [WIP] Code refactoring, consistent error messages, and RedisAI_ModelRun_RedisCommand optimizations Code refactoring, consistent error messages, and RedisAI_ModelRun_RedisCommand optimizations Apr 10, 2020
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.

Looks great. I've committed a couple of nits, we're good to merge

@lantiga lantiga merged commit 2cbfca8 into master Apr 10, 2020
@filipecosta90 filipecosta90 deleted the perf-modelrun branch May 1, 2020 10:49
lantiga added a commit that referenced this pull request May 6, 2020
 
Code refactoring, consistent error messages, and RedisAI_ModelRun_RedisCommand optimizations (#321)

* [wip] simplified argument processing on RedisAI_ModelRun_RedisCommand (only 1 full-loop vs 5)

* [add] code refactoring to reduce duplicates and reduce complexity. [add] enforced error reply string comparison on tests. [fix] error messages consistent across commands for tensor getting, etc...

* [add] increased coverage for tensorset and remove duplicate code

* [fix] ensuring slaves synced on tests_pytorch:test_pytorch_scriptrun. [add] quick refactoring to have consistent command description on redisai.c

* [fix] ERR error normalization.

* Fix function names, error message, correct warning

Co-authored-by: Luca Antiga <luca.antiga@orobix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants