-
Notifications
You must be signed in to change notification settings - Fork 159
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
Ns/feat/atomic patterns #2024
base: main
Are you sure you want to change the base?
Ns/feat/atomic patterns #2024
Conversation
Since only one kind is used at a time we don't need do allocate both
This allows to call it without having to define a max_noise_level
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 foresee that the ServerKeyAtomicPattern
is going to have a dynamic variant ServerKeyAtomicPattern::Dynamic(Box<dyn AtomicPattern>)
if yes, should the ServerKey just store a Box directly ?
Reviewed 9 of 45 files at r1, all commit messages.
Reviewable status: 9 of 45 files reviewed, 4 unresolved discussions (waiting on @nsarlin-zama)
tfhe/src/high_level_api/backward_compatibility/integers.rs
line 76 at r1 (raw file):
.as_view() .try_into() .map(|sk| old_sk_decompress(sk, a))
We could have let key = sk.key.key.key.as_view().try_into()?;
at the beginning of the fn, and use the key in the par_iter.map
Code quote:
.try_into()
.map(|sk| old_sk_decompress(sk, a))
tfhe/src/high_level_api/backward_compatibility/integers.rs
line 122 at r1 (raw file):
.as_view() .try_into() .map(|sk| old_sk_decompress(sk, a))
Same here, we could do`let key = sk.key.key.key.as_view().try_into()?;
Code quote:
sk.key
.key
.key
.as_view()
.try_into()
.map(|sk| old_sk_decompress(sk, a))
tfhe/src/shortint/atomic_pattern.rs
line 50 at r1 (raw file):
} pub trait AtomicPatternMutOperations {
I would have pub trait AtomicPatternMutOperations: AtomicPatternOperations
so that having the mut version also gives all the non mul ops
tfhe/src/shortint/server_key/mod.rs
line 406 at r1 (raw file):
impl<AP: Clone> GenericServerKey<&AP> { pub fn into_owned(&self) -> GenericServerKey<AP> {
Generally into_
method take self
, so I think a better name would be to_owned
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.
Yes the dynamic one is in progress
enum dispatch is supposedly more performant than vtable lookups, and some things are easier to do with enums, like serialization
Reviewable status: 9 of 45 files reviewed, 4 unresolved discussions (waiting on @tmontaigu)
tfhe/src/high_level_api/backward_compatibility/integers.rs
line 76 at r1 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
We could have
let key = sk.key.key.key.as_view().try_into()?;
at the beginning of the fn, and use the key in the par_iter.map
good idea !
tfhe/src/shortint/atomic_pattern.rs
line 50 at r1 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
I would have
pub trait AtomicPatternMutOperations: AtomicPatternOperations
so that having the mut version also gives all the non mul ops
ok I will do that!
tfhe/src/shortint/server_key/mod.rs
line 406 at r1 (raw file):
Previously, tmontaigu (tmontaigu) wrote…
Generally
into_
method takeself
, so I think a better name would beto_owned
ok, but to_owned is generally associated with the ToOwned
trait, so maybe there is a third option that is less confusing ?
41c1d18
to
a413178
Compare
closes: https://github.com/zama-ai/tfhe-rs-internal/issues/643
PR content/description
This PR adds the notion of Atomic Pattern to the shortint layer. This is currently a draft to be able to discuss on the design.
Atomic Pattern
An atomic pattern is a sequence of homomorphic operations that can be executed indefinitely. In TFHE, the standard atomic pattern is the chain of 5 linear operations + KS + PBS. In TFHE-rs, this is implemented at the shortint level by the
ServerKey::apply_lookup_table
(to be precise this is only the KS/PBS part). The goal of the PR is to add a genericity layer to be able to easily switch atomic pattern without having to rewrite higher level operations.Implementation
(The names of the trait and types are not definitive)
Currently the shortint
ServerKey
is represented by this structure:We can split these fields in 2 parts:
To do that, this PR first adds a trait
AtomicPatternOperations
. This trait defines the operations that should be supported by all the atomic patterns. It is dyn compatible to allows having atomic patterns as trait objects:This trait is first implemented for the "classical" (CJP) atomic pattern:
From there we have an enum of atomic pattern specific keys that all implement this trait:
The enum also implements
AtomicPatternOperations
(the "enum dispatch" design pattern).Finally, we have the "GenericServerKey" (name not definitive) defined as follow:
Some type aliases are defined to make it more usable:
ServerKey
is the one that is used almost everywhere, this reduces the impact on the higher layers. Every methods that use the ServerKey for lookup tables and the shortint encoding are usable without (almost) any modification.However some features don't fit well in the atomic pattern concept (as I understand it):
For these features, this design allows to create impl blocks that only work for one specific atomic pattern, by using the
ClassicalServerKey
type. To go from one type to the other,ClassicalServerKey
implementsTryFrom<ServerKey>
.To make this more efficient, we have 2 "View" types that allow conversions without having to clone the keys:
In the future, it should be easy to extend the set of supported AP for a feature.
For example we can have an
OprfServerKeyAtomicPattern
enum with only the subset of ap that support the oprf, and define a typeOprfServerKey = GenericServerKey<OprfServerKeyAtomicPattern>;
Check-list:
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)