Skip to content

Conversation

@DvirDukhan
Copy link

No description provided.

@DvirDukhan DvirDukhan requested a review from lantiga August 26, 2020 13:54
@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #451 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #451   +/-   ##
=======================================
  Coverage   75.00%   75.00%           
=======================================
  Files          21       21           
  Lines        4916     4916           
=======================================
  Hits         3687     3687           
  Misses       1229     1229           
Impacted Files Coverage Δ
src/model.c 71.46% <100.00%> (ø)

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 4818084...c4c566d. Read the comment docs.

@lantiga
Copy link
Contributor

lantiga commented Aug 27, 2020

This enables chunking if the version is = 100, although chunking is being backported in the context of #450. In other words, chunking wasn't there in RedisAI 1.0. What is the rationale for this change?

@DvirDukhan
Copy link
Author

@lantiga
1.0.x encver is 100
this means that backporting chunking to 1.0.2 is will cause it to encode the model in chunks but decode it in one string, which causes an invalid read of string, while the actual value itself is the number of chunks.
So perhaps a better way is to disable chunking in 1.0.x? We can drop the backport to 1.0.x (better) or have 2 encoders (I prefer not).

@lantiga
Copy link
Contributor

lantiga commented Aug 28, 2020

I see. I propose we could release proper chunking together with parallel DAG in a 1.1.0 release, while leaving chunking out for 1.0.x (including the current backport. What do you think @DvirDukhan ?
/cc @filipecosta90

@DvirDukhan
Copy link
Author

Closing this PR

@DvirDukhan DvirDukhan closed this Aug 28, 2020
@DvirDukhan DvirDukhan deleted the fix_encver_compare branch August 28, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants