Skip to content

Enable AI.SCRIPTRUN on AI.DAGRUN* #383

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.

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

Merged
merged 12 commits into from
May 25, 2020
Merged

Enable AI.SCRIPTRUN on AI.DAGRUN* #383

merged 12 commits into from
May 25, 2020

Conversation

filipecosta90
Copy link
Collaborator

@filipecosta90 filipecosta90 commented May 24, 2020

This is a WIP PR for including AI.SCRIPTRUN within the DAG execution ( feel free to jump in @lantiga :) )

  • decoupled scriptrun command parsing from scriptrun command
  • splitted positive/negative tests on pytorch scriptrun.
  • refactor AI.DAGRUN_RO and AI.DAGRUN to use the same code base (with read/write modes)
  • include SCRIPTRUN on DAGRUN and DAGRUN_RO
  • extensive testing for AI.SCRIPTRUN within AI.DAGRUN
  • update documentation to reflect scriptrun capabilities in DAGRUN

…d] splitted positive/negative tests on pytorch scriptrun. [fix] fixed #382 AI.SCRIPTRUN results are being replicated twice
@filipecosta90 filipecosta90 requested a review from lantiga May 24, 2020 14:14
@filipecosta90 filipecosta90 linked an issue May 24, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 24, 2020

Codecov Report

Merging #383 into master will increase coverage by 0.31%.
The diff coverage is 85.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   72.47%   72.79%   +0.31%     
==========================================
  Files          21       21              
  Lines        4349     4345       -4     
==========================================
+ Hits         3152     3163      +11     
+ Misses       1197     1182      -15     
Impacted Files Coverage Δ
src/model.c 69.60% <0.00%> (ø)
src/script.c 66.66% <77.08%> (+3.25%) ⬆️
src/dag.c 88.11% <88.37%> (+0.18%) ⬆️
src/redisai.c 77.94% <89.47%> (-0.94%) ⬇️
src/libtorch_c/torch_c.cpp 55.13% <0.00%> (+3.42%) ⬆️

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 d220f36...3732129. Read the comment docs.

lantiga
lantiga previously approved these changes May 25, 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.

LvGTM, just a couple of very minor nits

@filipecosta90
Copy link
Collaborator Author

@lantiga I'll split the fix for #382 (AI.SCRIPTRUN results are being replicated twice) from this added feature. It makes sense to not mix them.

@filipecosta90 filipecosta90 merged commit 3cd5caf into master May 25, 2020
@filipecosta90 filipecosta90 deleted the scriptrun.refactor branch May 25, 2020 14:46
@filipecosta90 filipecosta90 changed the title [WIP] Enable AI.SCRIPTRUN on AI.DAGRUN* Enable AI.SCRIPTRUN on AI.DAGRUN* May 27, 2020
filipecosta90 added a commit that referenced this pull request Aug 24, 2020
* [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
@filipecosta90 filipecosta90 mentioned this pull request Aug 24, 2020
filipecosta90 added a commit that referenced this pull request Sep 2, 2020
* 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 <[email protected]>

* [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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Luca Antiga <[email protected]>
Co-authored-by: DvirDukhan <[email protected]>
Co-authored-by: Guy Korland <[email protected]>
Co-authored-by: Sherin Thomas <[email protected]>
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.

Finalize DAG with SCRIPTRUN support
2 participants