Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) #438

filipecosta90 · 2020-08-05T17:06:13Z

When a module command is called in order to obtain the position of keys, since it was flagged as "getkeys-api" during the registration, the command implementation checks for this special call using the RedisModule_IsKeysPositionRequest() API and should use RedisModule_KeyAtPos() API in order to report keys.

Fixes Failed SCRIPTRUN with redis cluster #437
Fixes Define getkeys-api on AI.DAGRUN #434
Fixed dictKey allocation on RAI_parseDAGLoadArgs introduced by Multi-device parallel dag #424
Fixed RAI_FreeDagOp wrongfully calling RedisModule_Free on dagOp->inkeys, dagOp->outkeys
Fixed potential invalid mem accesses on RedisAI_DagRunSyntaxParser, RAI_parseDAGPersistArgs

"getkeys-api" Fix changes:

proceed with changes to DAGRUN, DAGRUN_RO, MODELRUN, and SCRIPTRUN
include cluster tests on CI
alter PyTorch tests to accommodate keys on the same slot. Run make -C opt test GEN=0 CLUSTER=1 AOF=0 SLAVES=0 TEST=tests_pytorch.py and pass.
alter Sanitizer tests to accommodate keys on the same slot. Run make -C opt test TEST=tests_sanitizer.py and pass.
alter TensorFlow tests to accommodate keys on the same slot. Run make -C opt test GEN=0 CLUSTER=1 AOF=0 SLAVES=0 TEST=tests_tensorflow.py and pass.
alter TensorFlow Lite tests to accommodate keys on the same slot. Run make -C opt test GEN=0 CLUSTER=1 AOF=0 SLAVES=0 TEST=tests_tflite.py and pass.
alter ONNX runtime tests to accommodate keys on the same slot. Run make -C opt test GEN=0 CLUSTER=1 AOF=0 SLAVES=0 TEST=tests_onnx.py and pass.
alter DAG tests to accommodate keys on the same slot. Run make -C opt test GEN=0 CLUSTER=1 AOF=0 SLAVES=0 TEST=tests_dag.py and pass.

…the registration. ( AI.MODELRUN, AI.SCRIPTRUN, AI.DAGRUN, AI.DAGRUN_RO )

…ser, RAI_parseDAGPersistArgs

…p->inkeys, dagOp->outkeys

…equest() from main code of dagrun,modelrun, and scriptrun to specific methods

lantiga

Looks great, only a couple of comments to have clarity on memory management on dict keys.

src/dag.c

…into fix.getkeys.api

lantiga

One last strdup to remove

src/dag.c

…ialNet_no_writes_multiple_modelruns

filipecosta90 · 2020-08-07T13:34:16Z

@lantiga I've checked that we're having an issue on the asci conversion of the DEVICE name on the GPU tests.
https://app.circleci.com/pipelines/github/RedisAI/RedisAI/1695/workflows/9103bb77-c88d-463f-9141-3ed2632fdef0/jobs/4342
I'm commenting out those check lines so that we can fully check CI.

codecov · 2020-08-07T20:00:38Z

Codecov Report

Merging #438 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   74.67%   74.74%   +0.06%     
==========================================
  Files          21       21              
  Lines        4849     4886      +37     
==========================================
+ Hits         3621     3652      +31     
- Misses       1228     1234       +6

Impacted Files	Coverage Δ
src/dag.c	`88.57% <100.00%> (-0.59%)`	⬇️
src/model.c	`71.46% <100.00%> (+0.65%)`	⬆️
src/redisai.c	`79.45% <100.00%> (+0.12%)`	⬆️
src/run_info.c	`88.31% <100.00%> (-0.06%)`	⬇️
src/script.c	`65.16% <100.00%> (+1.63%)`	⬆️
src/tensor.c	`83.16% <0.00%> (-0.17%)`	⬇️

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 26faf7e...67a7758. Read the comment docs.

filipecosta90 · 2020-08-07T20:05:38Z

@lantiga CI is passing. Can you give it a last review?

lantiga

LGTM!

…I.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) (#438) * [wip] wip on fixing the commands being flagged as getkeys-api during the registration. ( AI.MODELRUN, AI.SCRIPTRUN, AI.DAGRUN, AI.DAGRUN_RO ) * [add] pytorch oss cluster tests passing * [add] TensorFlow lite tests passing on oss cluster * [add] ONNX tests passing on oss cluster * [add] TensorFlow tests passing on oss cluster * [wip] wip on dag * [add] DAG tests passing on oss cluster * [add] enabling oss cluster tests on CI * [add] bumping rmbuilder version from 6.0.1 to 6.0.5 on circleci * [fix] fixed potential invalid mem accesses on RedisAI_DagRunSyntaxParser, RAI_parseDAGPersistArgs * [fix] fixed RAI_FreeDagOp wrongfully calling RedisModule_Free on dagOp->inkeys, dagOp->outkeys * [fix] fixed dictKey allocation on RAI_parseDAGLoadArgs * [add] alter Sanitizer tests to accommodate keys on the same slot. * [add] extracted the methods to respond to RedisModule_IsKeysPositionRequest() from main code of dagrun,modelrun, and scriptrun to specific methods * [fix] Fixes per PR review * [fix] fixes per PR review * [fix] fix per CI ascii conversion error on test_dagro_modelrun_financialNet_no_writes_multiple_modelruns

* Introduced readies submodule (#377) * Introduced readies submodule * Fix in paella * Updated get_deps.sh and docs * [WIP] add C API reference to docs using auto tool (#368) * [add] add C API reference to docs using auto tool ( python3 generate_llapi_reference.py ) * [add] generating C API reference on the fly * [add] moving api_reference under Developer Nav Co-authored-by: Luca Antiga <luca.antiga@orobix.com> * [WIP] Enable AI.SCRIPTRUN on AI.DAGRUN* (#383) * [add] decoupled scriptrun command parsing from runtime scriptrun. * [add] split positive/negative tests on pytorch scriptrun. * [add] refactor AI.DAGRUN_RO and AI.DAGRUN to use the same code base (with read/write modes) * [add] added positive and negative tests for dagrun with scriptrun * [add] updated documentation to reflect scriptrun support on dagrun * [add] added example enqueuing multiple SCRIPTRUN and MODELRUN commands within a DAG * Add support for variadic arguments to SCRIPT (#395) * Add support for variadic arguments to SCRIPT * Add negative errors * atomic ref count (#403) * Multi-platform build (#398) * CircleCI: increased GPU test timeout to 40m (#404) * CI: platform-build-defs -> on-master-and-version-tags (#405) * Llapi updates (#400) * added low level api return redis types * added variadic to llapi * fixed memory issue for params array re-alloc * Avoid splitting outputs in batches when nbatches == 1 (#406) * Avoid splitting outputs in batches when nbatches == 1 * Add batch size checks * Fix batch checks * Update readies * Add bad batching test * Update README.md * CircleCI: fixed redis installation in coverage builds (#423) * Docker6.0.5 (#427) * Update Dockerfile * Update Dockerfile.arm * Update Dockerfile.gpu * submodule pull moved up (#428) * Update mkdocs.yml (#432) * [add] Add relevant RedisAI config entries to the INFO output, so that standard monitoring systems would be able to monitor them. (#396) * Updated support email and Orobix to Tensorwork on ramp file (#436) * [add] updated support email and Orobix to Tensorwork on ramp file Co-authored-by: Sherin Thomas <sherin@tensorwerk.com> * CircleCI: Fixed problem with GPU testing (#440) * Fixed platforms build problem (#441) * Fixed platforms build problem * Build: updated Redis versions * Build: another Redis version update * Dependencies in ramp.yml (#444) * Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) (#438) * [wip] wip on fixing the commands being flagged as getkeys-api during the registration. ( AI.MODELRUN, AI.SCRIPTRUN, AI.DAGRUN, AI.DAGRUN_RO ) * [add] pytorch oss cluster tests passing * [add] TensorFlow lite tests passing on oss cluster * [add] ONNX tests passing on oss cluster * [add] TensorFlow tests passing on oss cluster * [wip] wip on dag * [add] DAG tests passing on oss cluster * [add] enabling oss cluster tests on CI * [add] bumping rmbuilder version from 6.0.1 to 6.0.5 on circleci * [fix] fixed potential invalid mem accesses on RedisAI_DagRunSyntaxParser, RAI_parseDAGPersistArgs * [fix] fixed RAI_FreeDagOp wrongfully calling RedisModule_Free on dagOp->inkeys, dagOp->outkeys * [fix] fixed dictKey allocation on RAI_parseDAGLoadArgs * [add] alter Sanitizer tests to accommodate keys on the same slot. * [add] extracted the methods to respond to RedisModule_IsKeysPositionRequest() from main code of dagrun,modelrun, and scriptrun to specific methods * [fix] Fixes per PR review * [fix] fixes per PR review * [fix] fix per CI ascii conversion error on test_dagro_modelrun_financialNet_no_writes_multiple_modelruns * Safely add to arrays + fix for #443 (#449) * Make sure we reassign the pointer in array_append * Fix case-sensitive comparison for devicestr * Fix sanitizer tests (cherry picked from commit 5c7813e) * fixed tests messages * Shallow copy persisted tensor * Resolve log format warnings * Fix error message and test * Fix memory management in local context dict * Fixed artifacts handling + added tests logs aggregation (#445) * Snapshot packages are placed into http://redismodules.s3.amazonaws.com/redisai/snapshots. Private branches (i.e., non-master) are also supported, just need to enable deploy-snapshot in config.yml to fire on on-any-branch rather than just on-master. * Release packages are placed into http://redismodules.s3.amazonaws.com/redisai, and it's going to be crowded in there, so we may want to consider putting each version into its own directory. * `BB` in-source breakpoints are now supported (in `DEBUG=1` builds): put `BB;` inside the code and it will stop if you're running under gdb. * Tests results in CircleCI are now aggregated as artifacts. * Build: setuptools-related fix (#455) (cherry picked from commit d1fcd0d) Co-authored-by: Rafi Einstein <raffapen@outlook.com> Co-authored-by: Luca Antiga <luca.antiga@orobix.com> Co-authored-by: DvirDukhan <dvir@redislabs.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Sherin Thomas <sherin@tensorwerk.com>

filipecosta90 added 12 commits August 5, 2020 12:53

[wip] wip on fixing the commands being flagged as getkeys-api during …

09c26cf

…the registration. ( AI.MODELRUN, AI.SCRIPTRUN, AI.DAGRUN, AI.DAGRUN_RO )

Merge branch 'master' into fix.getkeys.api

4535022

[add] pytorch oss cluster tests passing

8c8ac76

[add] TensorFlow lite tests passing on oss cluster

7d1dff7

[add] ONNX tests passing on oss cluster

405bdb6

[add] TensorFlow tests passing on oss cluster

a366ab6

[wip] wip on dag

165fd64

[add] DAG tests passing on oss cluster

2b3ad9e

[add] enabling oss cluster tests on CI

5205acb

[add] bumping rmbuilder version from 6.0.1 to 6.0.5 on circleci

e50a1e8

[fix] fixed potential invalid mem accesses on RedisAI_DagRunSyntaxPar…

9fdfb28

…ser, RAI_parseDAGPersistArgs

[fix] fixed RAI_FreeDagOp wrongfully calling RedisModule_Free on dagO…

264f8c4

…p->inkeys, dagOp->outkeys

filipecosta90 mentioned this pull request Aug 6, 2020

Add getkeys-api to the DAGRUN command #435

Closed

filipecosta90 added 2 commits August 6, 2020 12:40

[fix] fixed dictKey allocation on RAI_parseDAGLoadArgs

8714176

[add] alter Sanitizer tests to accommodate keys on the same slot.

21e7e04

filipecosta90 changed the title ~~[WIP] Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN )~~ Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) Aug 6, 2020

filipecosta90 requested a review from lantiga August 6, 2020 11:59

filipecosta90 added 2 commits August 6, 2020 15:04

[add] extracted the methods to respond to RedisModule_IsKeysPositionR…

e0aabf8

…equest() from main code of dagrun,modelrun, and scriptrun to specific methods

Merge branch 'master' into fix.getkeys.api

32484a8

filipecosta90 mentioned this pull request Aug 6, 2020

Failed SCRIPTRUN with redis cluster #437

Closed

lantiga reviewed Aug 6, 2020

View reviewed changes

src/dag.c Outdated Show resolved Hide resolved

src/dag.c Outdated Show resolved Hide resolved

filipecosta90 added 3 commits August 7, 2020 13:52

[fix] Fixes per PR review

1ee4d4b

Merge branch 'master' into fix.getkeys.api

827be02

Merge branch 'fix.getkeys.api' of https://github.com/RedisAI/RedisAI …

cf3dfc1

…into fix.getkeys.api

filipecosta90 requested a review from lantiga August 7, 2020 12:54

filipecosta90 added the bug label Aug 7, 2020

lantiga suggested changes Aug 7, 2020

View reviewed changes

src/dag.c Outdated Show resolved Hide resolved

[fix] fixes per PR review

056834b

filipecosta90 requested a review from lantiga August 7, 2020 13:03

[fix] fix per CI ascii conversion error on test_dagro_modelrun_financ…

67a7758

…ialNet_no_writes_multiple_modelruns

lantiga approved these changes Aug 7, 2020

View reviewed changes

lantiga merged commit b1494aa into master Aug 7, 2020

lantiga deleted the fix.getkeys.api branch August 7, 2020 21:28

filipecosta90 mentioned this pull request Aug 20, 2020

[WIP] v1.0 fixes cherry pick #448

Closed

5 tasks

filipecosta90 mentioned this pull request Aug 24, 2020

v1.0 cherry picks #450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) #438

Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) #438

Uh oh!

filipecosta90 commented Aug 5, 2020 •

edited

Loading

Uh oh!

lantiga left a comment

Uh oh!

Uh oh!

Uh oh!

lantiga left a comment

Uh oh!

Uh oh!

filipecosta90 commented Aug 7, 2020

Uh oh!

codecov bot commented Aug 7, 2020

Uh oh!

filipecosta90 commented Aug 7, 2020

Uh oh!

lantiga left a comment

Uh oh!

Uh oh!

Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) #438

Fixed flagged as "getkeys-api" during the registration ( AI.DAGRUN, AI.DAGRUN_RO, AI.MODELRUN, AI.SCRIPTRUN ) #438

Uh oh!

Conversation

filipecosta90 commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lantiga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lantiga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

filipecosta90 commented Aug 7, 2020

Uh oh!

codecov bot commented Aug 7, 2020

Codecov Report

Uh oh!

filipecosta90 commented Aug 7, 2020

Uh oh!

lantiga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

filipecosta90 commented Aug 5, 2020 •

edited

Loading