-
Notifications
You must be signed in to change notification settings - Fork 155
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
chore(all): use a builder pattern for safe serialization API #1465
Conversation
b8bdd99
to
7159326
Compare
7159326
to
05fada8
Compare
74d8771
to
456a7aa
Compare
bbb4ce1
to
72f9a6f
Compare
456a7aa
to
3ec15d9
Compare
a7822c5
to
15c9215
Compare
15c9215
to
efc6954
Compare
3ec15d9
to
a9c0bce
Compare
a9c0bce
to
1a241a1
Compare
1a241a1
to
9dbfb8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
efc6954
to
f6119ad
Compare
.serialize_into(&ct, &mut buffer) | ||
.unwrap(); | ||
|
||
assert!(DeserializationConfig::new(1 << 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we think having a default value for the size_limit could be good, and maybe have an impl Default for SerializationConfig
that uses that default size_limit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be a good default value ? We don't know the kind of device where this will run nor the concrete type that is deserialized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all our example we use 1 << 20, which is roughly 1GB, and I bet most users are just going to copy paste that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you ok if I add this as an issue to not rush this before release ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1<<20 = 1 MB IIRC
1 << 30 = 1GB
ed3d310
to
274c7ec
Compare
9dbfb8d
to
ed3d310
Compare
274c7ec
to
19ea462
Compare
ed3d310
to
7502a82
Compare
BREAKING CHANGES: - `safe_serialize` and `safe_deserialize` are replaced by `SerializationConfig::serialize_into` and `DeserializationConfig::deserialize_from`. - C API: the `XXX_safe_serialize_versioned` is deprecated, `XXX_safe_serialize` is now versioned by default - JS API: the `safe_serialize` method now versionize the data before serialization. This is *NOT* a serialization breaking change for data serialized in previous versions with `safe_serialize_versioned`.
7502a82
to
d2c031f
Compare
closes: https://github.com/zama-ai/tfhe-rs-internal/issues/646
PR content/description
The goal of this PR is to simplify the safe_serialization API. With the introduction of data versioning, there are many different serialization possibilities:
This can be difficult for us to maintain and confusing to the user since the function they should use by default have the most complicated names (
safe_deserialize_conformant_versioned
). The idea is to introduce a simple builder pattern to configure the serialization, with the safest defaults and with a possibility to disable functionalities for advanced users.This PR introduces 2 objects,
SerializationConfig
andDeserializationConfig
. The idea for serialization is to create this object, eventually modify it to disable some functionalities, and serialize an object with theserialize_into
method:The
safe_serialize
/safe_serialize
have been kept as defaults for users that don't need customization. They include versioning.Other changes:
DeserializationConfig::new(...).deserialize_from(...)
handles both versioned and unversioned data.serialize/deserialize
(without safe) that are only wrappers around serde.Check-list: