Skip to content

Add support for variadic arguments to SCRIPT #395

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 2 commits into from
Jun 1, 2020
Merged

Conversation

lantiga
Copy link
Contributor

@lantiga lantiga commented Jun 1, 2020

This PR addresses #392.
A slightly different command API was chosen to keep things efficient.

We use the $ sign to signal that INPUTS following that sign will be passed to the script as a list:

redis-cli -x AI.SCRIPTSET foo CPU SOURCE < foo.py
redis-cli AI.TENSORSET a FLOAT 1 1 VALUES 1
redis-cli AI.TENSORSET b FLOAT 1 1 VALUES 1
redis-cli AI.TENSORSET c FLOAT 1 1 VALUES 1
redis-cli AI.SCRIPTRUN foo foo INPUTS a $ b c OUTPUTS d
redis-cli AI.TENSORGET d META VALUES

The corresponding script will need to be type-annotated with List[Tensor] for the last argument

def foo(a : Tensor, args: List[Tensor]):
    return a + torch.stack(args).sum()

Within the script, args will act as a standard Python list containing tensors.

Test has been added and docs have been updated.

DvirDukhan
DvirDukhan previously approved these changes Jun 1, 2020
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #395 into master will decrease coverage by 0.07%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   72.79%   72.72%   -0.08%     
==========================================
  Files          21       21              
  Lines        4345     4359      +14     
==========================================
+ Hits         3163     3170       +7     
- Misses       1182     1189       +7     
Impacted Files Coverage Δ
src/backends/torch.c 84.37% <ø> (ø)
src/script.c 66.86% <66.66%> (+0.19%) ⬆️
src/libtorch_c/torch_c.cpp 56.77% <100.00%> (+1.64%) ⬆️
src/redisai.c 76.93% <0.00%> (-1.02%) ⬇️

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 0557118...b7388a8. Read the comment docs.

filipecosta90
filipecosta90 previously approved these changes Jun 1, 2020
Copy link
Collaborator

@filipecosta90 filipecosta90 left a comment

Choose a reason for hiding this comment

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

LGTM @lantiga . WDYT of adding some more negative tests with variadic, just to ensure that the parsing, and run will always return the expected errors,etc... ?

@lantiga lantiga dismissed stale reviews from filipecosta90 and DvirDukhan via b7388a8 June 1, 2020 13:01
@filipecosta90 filipecosta90 self-requested a review June 1, 2020 13:06
@lantiga lantiga merged commit 1a3f394 into master Jun 1, 2020
@lantiga lantiga deleted the variadic_script branch June 1, 2020 13:48
filipecosta90 pushed a commit that referenced this pull request Aug 24, 2020
* Add support for variadic arguments to SCRIPT

* Add negative errors
@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 <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>
# 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.

3 participants