Enable chunking for large models in AOF, RDB, replication and MODELGET #387
Conversation
Codecov Report
@@ Coverage Diff @@
## master #387 +/- ##
==========================================
+ Coverage 72.60% 72.96% +0.36%
==========================================
Files 21 21
Lines 4424 4472 +48
==========================================
+ Hits 3212 3263 +51
+ Misses 1212 1209 -3
Continue to review full report at Codecov.
|
src/redisai.c
Outdated
| RedisModule_ReplyWithCString(ctx, "blob"); | ||
| RedisModule_ReplyWithStringBuffer(ctx, buffer, len); | ||
| const size_t n_chunks = len / RAI_CHUNK_LEN + 1; | ||
| RedisModule_ReplyWithArray(ctx, (long)n_chunks); |
There was a problem hiding this comment.
@lantiga this will break the clients correct? IMO we also need the change documented as soon as we merge this.
There was a problem hiding this comment.
Yes it will. Part of this review was also to coordinate on this aspect.
There was a problem hiding this comment.
as we've discussed today, let's reply with cstring if the total number of chunks is 1 ( for retro compatibility ) and reply with an array if more than 1 chunk.
Enabling a chunk size on load time or via ai.config should be also possible.
There was a problem hiding this comment.
We now need a chunking test and we are done
src/config.c
Outdated
| sizeof(*buffer)); | ||
| sprintf(buffer, "%s: %lld", REDISAI_INFOMSG_MODEL_CHUNK_SIZE, | ||
| getModelChunkSize()); | ||
| RedisModule_Log(ctx, "notice", buffer); |
There was a problem hiding this comment.
void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...)
I think you can do
RedisModule_Log(ctx, "notice", "%s: %lld", REDISAI_INFOMSG_MODEL_CHUNK_SIZE, getModelChunkSize());
src/config.c
Outdated
| RedisModule_Free(buffer); | ||
| } | ||
| } else if (strcasecmp((key), "MODEL_CHUNK_SIZE") == 0) { | ||
| RedisAI_Config_ModelChunkSize(rsval); |
src/redisai.c
Outdated
| long long chunk_size = getModelChunkSize(); | ||
| const size_t n_chunks = len / chunk_size + 1; | ||
| if (n_chunks > 1) { | ||
| RedisModule_ReplyWithArray(ctx, (long)n_chunks); | ||
| for (size_t i=0; i<n_chunks; i++) { | ||
| size_t chunk_len = i < n_chunks - 1 ? chunk_size : len % chunk_size; | ||
| RedisModule_ReplyWithStringBuffer(ctx, buffer + i * chunk_size, chunk_len); | ||
| } | ||
| } | ||
| else { | ||
| RedisModule_ReplyWithStringBuffer(ctx, buffer, len); | ||
| } |
There was a problem hiding this comment.
Reorder code or migrate to a function?
| /* This is the major / minor encver, to be used as | ||
| argument to RM_CreateDataType, which expects | ||
| encver to be < 1024 */ | ||
| static const long long RAI_ENC_VER_MM = RAI_ENC_VER == 999999 ? 1023 : RAI_ENC_VER / 100; |
There was a problem hiding this comment.
As far as I understand, current (master) RAI_ENC_VER is 0?
How do you plan to maintain updates to RAI_ENC_VER in the objects RDB decoders?
There was a problem hiding this comment.
Current master is RAI_ENC_VER 999999, while for 1.0 RAI_ENC_VER is 10000 (major 1, minor 00, patch 00).
There was a problem hiding this comment.
ok
So there is a need to update a minor version once this update is merged, right?
For now, the decoding of objects from RDB is checking if RAI_ENC_VER > 100 for the new partitioned decode, otherwise it will decode as a single buffer. This is ok for now since there are only two "official" versions. Once there will be additional modifications in encoding/decoding objects to RDB I suggest moving to a dedicated decoding function per encoding version.
There was a problem hiding this comment.
Yes, if we release this will need to be a minor (1.1.0)
#387) * Enable chunking for large models in AOF, RDB, replication and MODELGET * Remove spurious printfs * Make chunk size configurable via MODEL_CHUNK_SIZE. Return blob directly if n_chunks==1 * Make chunk size configurable, add tests * Update docs * Address review comments
Addresses #386
Tests are missing, we need a strategy for downloading a large (512 MB) model from CI.
Note: this PR changes the format of RDB. Versioning is handled, ie older RDB are still loaded correctly.