Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add serialization of data types #72

Merged
merged 15 commits into from
Mar 5, 2019
Merged

Add serialization of data types #72

merged 15 commits into from
Mar 5, 2019

Conversation

lantiga
Copy link
Contributor

@lantiga lantiga commented Mar 2, 2019

This PR addresses #64.

In a nutshell:

  • tensors are serialized from the underlying dlpack structures
  • scripts are serialized from their string definition
  • models are (re-)converted to protobufs from their in-memory representations, to avoid duplication

NOTE this is WIP, not ready for review yet (needs testing).

@gkorland gkorland requested a review from mnunberg March 3, 2019 08:10
src/model.c Outdated Show resolved Hide resolved
src/model.c Show resolved Hide resolved
Copy link
Contributor

@mnunberg mnunberg left a comment

Choose a reason for hiding this comment

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

see in-line

src/model.c Outdated Show resolved Hide resolved
@lantiga
Copy link
Contributor Author

lantiga commented Mar 4, 2019

@mnunberg Finally all tests check, both with AOF and RDB

I still haven't switched to array_, I can do that now. But otherwise feel free to review.

@lantiga lantiga changed the title Add serialization of data types [WIP] Add serialization of data types Mar 4, 2019
@lantiga lantiga requested a review from mnunberg March 4, 2019 22:40
@lantiga
Copy link
Contributor Author

lantiga commented Mar 5, 2019

I had to temporarily disable AOF as I stumbled on something that needs further investigation: AOF loading seem to choke on commands that use RedisModule_BlockClient, such as MODELRUN.

@lantiga
Copy link
Contributor Author

lantiga commented Mar 5, 2019

Confirmed: https://github.com/antirez/redis/blob/unstable/src/aof.c#L808
We should find a way to execute all commands sequentially and not blocking if we're loading from AOF.

@lantiga
Copy link
Contributor Author

lantiga commented Mar 5, 2019

@mnunberg This is an issue for later, we should review/merge as is and open a separate issue.

lantiga added a commit that referenced this pull request May 6, 2020
 
Add serialization of data types (#72)

* Add serialization of data types

* Add AofRewrite (it might go in the future) and replication

* Saving/loading tensors to/from RDB now working

* Fix loading scripts

* Handle encver properly

* Use zero initialization for the error struct

* Fix for script serialization. Add tests.

* Fix length of input and output arrays

* Fix arguments to calloc

* Use getkeys-api

* Use array_new instead of plain array

* Disable AOF for now

* Improve testing of unhappy paths

* More unhappy path tests

* Temporarily stop when AOF if activated
@gkorland gkorland deleted the serialization branch October 6, 2020 08:56
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.

None yet

2 participants