Skip to content

Conversation

lantiga
Copy link
Contributor

@lantiga lantiga commented Apr 23, 2020

While testing on of the HuggingFace models (1.3 GB), MODELSET was failing due to the fact that Redis strings are limited to 512 MB.

This PR gives the possibility to provide a large model in chunks, overcoming the limitation (at the cost of a transient allocation + memcpy of a large chunk of memory, but for now there are no alternatives):

with open(model_filename, 'rb') as f:
    model = f.read()
chunk_size = 500 * 1024 * 1024
model_chunks = [model[i:i + chunk_size] for i in range(0, len(model), chunk_size)]
ret = con.execute_command('AI.MODELSET', 'm', 'TORCH', DEVICE, 'BLOB', *model_chunks)

Note that the API has been changed. One now has to specify BLOB to signal the start of chunks:

AI.MODELSET foo TORCH CPU BLOB ...

For consistency, the same was done for SCRIPTSET, with the addition of SOURCE:

AI.SCRIPTSET foo CPU SOURCE ...

@hhsecond: we need to update the Python client both with BLOB and SOURCE, as well as chunking (detect if blob is larger than 512 MB and chunk it tansparently)

Copy link
Collaborator

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

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

LGTM.

  • We'll need to port the docs :/
  • Semi-related - it would be good to add a few tests trying to load broken models. I managed to segfault IIRC by trying to load a file that I wgetted and was actually an HTML

@lantiga
Copy link
Contributor Author

lantiga commented Apr 23, 2020

Thank you @itamarhaber.
What do you think about adding a FILE option, in addition to BLOB, where the file is loaded by Redis in a location relative to some configurable PATH?
This would have the advantage that RDB and AOF would just reference that one, without growing 1.3GB in size upon serialization.
It would make sense for really large models.

cc @filipecosta90

@filipecosta90
Copy link
Collaborator

What do you think about adding a FILE option, in addition to BLOB, where the file is loaded by Redis in a location relative to some configurable PATH?
This would have the advantage that RDB and AOF would just reference that one, without growing 1.3GB in size upon serialization.
It would make sense for really large models.

cc @filipecosta90

@lantiga imho that would break the entire idea around portability of the DB files, and open the door for more complex errors that I believe should not be a responsibility of the module. The module should use the persistency options provided by redis and allow as single point of entrance for data the commandHandler/commands. What do you think?

@lantiga
Copy link
Contributor Author

lantiga commented Apr 23, 2020

@filipecosta90 that has always been my idea too. But these models are huge, so I was thinking about possible ways to anticipate RDB/AOF serialization issues.
I'm going to merge this now and we can think about it in the future (and the API now allows for it without further backward breakages).
I suspect this will come out as a request some of the use cases.

@lantiga lantiga merged commit 4b5c4e2 into master Apr 23, 2020
@filipecosta90
Copy link
Collaborator

@filipecosta90 that has always been my idea too. But these models are huge, so I was thinking about possible ways to anticipate RDB/AOF serialization issues.
I'm going to merge this now and we can think about it in the future (and the API now allows for it without further backward breakages).
I suspect this will come out as a request some of the use cases.

agree =)

lantiga added a commit that referenced this pull request May 6, 2020
 
Add possibility to provide a model in chunks (#338)

* Add possibility to provide a model in chunks

* Add test on chunked modelset
@gkorland gkorland deleted the chunked_models branch October 6, 2020 08:57
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