Skip to content

Turn dag local context dict into array #582

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 10 commits into from
Feb 2, 2021

Conversation

alonre24
Copy link
Collaborator

Introduce a shared array in DAG runInfo that stores DAG intermediate tensors (instead of the DagLocalContext dict). Every DAG operation holds the indices of its input and output tensors, and uses this array to load and store them in turn.

- In TF: verify that model inputs names correspond to model inputs ops (and not just for any op)
…tensors (instead of the DagLocalContext dict). Every op holds the indices of its input and output tensors, and uses this array to load and store them in turn.
@alonre24 alonre24 self-assigned this Jan 30, 2021
@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #582 (63cf975) into master (812444a) will increase coverage by 0.01%.
The diff coverage is 87.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   74.27%   74.28%   +0.01%     
==========================================
  Files          39       39              
  Lines        6083     5997      -86     
==========================================
- Hits         4518     4455      -63     
+ Misses       1565     1542      -23     
Impacted Files Coverage Δ
src/DAG/dag.c 85.97% <79.41%> (+3.64%) ⬆️
src/DAG/dag_parser.c 95.32% <87.50%> (-1.05%) ⬇️
src/run_info.c 72.57% <88.23%> (+0.28%) ⬆️
src/DAG/dag_builder.c 100.00% <100.00%> (ø)
src/DAG/dag_execute.c 92.56% <100.00%> (-0.11%) ⬇️
src/redisai.c 84.16% <100.00%> (+0.04%) ⬆️
src/tensor.c 76.45% <100.00%> (-0.17%) ⬇️
src/model.c 78.28% <0.00%> (+1.14%) ⬆️

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 812444a...63cf975. Read the comment docs.

- Save the indices of the tensors to persist as values in persistTensors dict, and release TensorsNamesToIndices before DAG run.
lantiga
lantiga previously approved these changes Feb 1, 2021
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.

Awesome job, this looks really good to me. Let's kill mangling!

src/DAG/dag.h Outdated
* @param index The index of the tensor in the Dag shared array to return
* @return The tensor of the given index (NULL is returned if this tensor hasn't been realized yet)
*/
RAI_Tensor *Dag_GetInternalTensor(RedisAI_RunInfo *rinfo, size_t index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
RAI_Tensor *Dag_GetInternalTensor(RedisAI_RunInfo *rinfo, size_t index);
RAI_Tensor *Dag_GetTensorFromGlobalCtx(RedisAI_RunInfo *rinfo, size_t index);

src/DAG/dag.h Outdated
* @param index The index to put in the given tensor in the Dag shared array.
* @param t The tensor to shallow copy and store in the given index.
*/
void Dag_SetInternalTensor(RedisAI_RunInfo *rinfo, size_t index, RAI_Tensor *t);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void Dag_SetInternalTensor(RedisAI_RunInfo *rinfo, size_t index, RAI_Tensor *t);
void Dag_SetTensorInGlobalCtx(RedisAI_RunInfo *rinfo, size_t index, RAI_Tensor *t);

@alonre24 alonre24 merged commit a4173a1 into master Feb 2, 2021
@alonre24 alonre24 deleted the Turn_DAG_local_context_into_array branch February 2, 2021 20:14
DvirDukhan pushed a commit that referenced this pull request Feb 3, 2021
@chayim chayim mentioned this pull request Jul 22, 2021
@chayim chayim mentioned this pull request Nov 7, 2021
# 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