Many refactors #983

mayeul-zama · 2024-03-13T10:28:12Z

No description provided.

IceTDrinker

Several comments, see with David and Thomas if need be for coverage and function executor patterns for tests

I think the shortint compression would not work as expected as it would worsen the degree of the ciphertexts as it's currently written

I would therefore require a test checking for an identity decompression that the degree is conserved

tfhe/src/shortint/ciphertext/compressed_ciphertext.rs

tfhe/src/shortint/server_key/mod.rs

tfhe/src/integer/ciphertext/compressed_ciphertext.rs

tfhe/src/shortint/ciphertext/compressed_ciphertext.rs

IceTDrinker · 2024-03-15T10:13:20Z

tfhe/src/integer/ciphertext/compressed_ciphertext.rs

+    fn modulus_switch_compression<P>(param: P)
+    where
+        P: Into<PBSParameters>,


does not use the Function executor pattern ?

if need be ask Thomas how to use the executor stuff

tfhe/src/shortint/server_key/mod.rs

tfhe/src/shortint/ciphertext/compressed_ciphertext.rs

IceTDrinker · 2024-03-19T12:50:40Z

tfhe/src/shortint/server_key/tests/parametrized_test_bivariate_pbs_compliant.rs

+        let ct2 = cks.encrypt_with_message_modulus(clear2, MessageModulus(modulus as usize));
+
+        println!("MUL SMALL CARRY:: clear1 = {clear1}, clear2 = {clear2}, mod = {modulus}");
+        let ct_res = sks.unchecked_mul_lsb_small_carry(&ct1, &ct2);


I don't think this uses the bivariate PBS, I think I would go for an integer like rework, where each op gets its file, so you would export the macro in the shortint module that would get moved to e.g.

tests/mod.rs

and then crate a file for the operation you are currently working on ?

so

tests/modulus_switch_compression.rs

does that make sense ?

tfhe/src/shortint/server_key/mod.rs

tfhe/src/shortint/ciphertext/mod.rs

IceTDrinker · 2024-03-19T13:00:41Z

tfhe/src/shortint/ciphertext/mod.rs

-        }
-    }
-}
+#[allow(clippy::module_inception)]


unsure for the naming below

also, let's keep the modules public for now, more often than not having private stuff ends being more problems than it's worth at the moment

By reexporting everything publicly, the interface of this module remains unchanged I believe
I don't see what problem this could cause

we'll see but from experience on TFHE-rs as soon as something is not public, then it gets annoying/someone needs access for fairly good reason

I believe everything is accessible through the pub use, the only difference being that it appears one module higher (as before this refactor)

as I said, we will see :)

IceTDrinker · 2024-03-19T16:25:22Z

tfhe/src/shortint/server_key/tests/mod.rs

-pub mod shortint;
+pub mod parametrized_test;
+pub mod parametrized_test_bivariate_pbs_compliant;


the split does not make sense I believe as we discussed ?

Even if eventually, we'd like to have many more files, I think, it's better to have two files rather than one

yes but the split does not make sense as some tests are not bivar PBS compliant in the corresponding file :/

IceTDrinker · 2024-03-19T16:27:45Z

tfhe/examples/sha256.rs

@@ -37,7 +35,9 @@ macro_rules! __join_implementation {
    };
 }

-#[macro_export]
+#[doc(hidden)]


doc(hidden) on the re export ?

It was already hidden, but I can remove it if you think it makes sense

I mean does a doc hidden make sense on a pub use ?

It's a pub(crate) use

sorry

I mean

doc(hidden) on a use statement does nothing ?

IceTDrinker · 2024-03-19T16:30:46Z

tfhe/src/integer/ciphertext/mod.rs

@@ -1,490 +1,11 @@
-//! This module implements the ciphertext structures.
+mod base;


let's keep things public, as you can see the boolean_value one was public, same thing for shortint please

IceTDrinker · 2024-03-19T16:31:34Z

tfhe/src/integer/ciphertext/utils.rs

+use super::{CrtCiphertext, RadixCiphertext, SignedRadixCiphertext};
+use crate::shortint::Ciphertext;
+
+pub trait IntegerCiphertext: Clone {
+    fn blocks(&self) -> &[Ciphertext];


shouldn't this be in the mod.rs itself ?

I think it's clearer to have the mod only containing other modules (and possibly reexports)
Renamed module integer_ciphertext

IceTDrinker · 2024-03-19T16:31:52Z

tfhe/src/integer/client_key/radix.rs

@@ -27,8 +27,7 @@ use serde::{Deserialize, Serialize};
 ///
 /// let ct = cks.encrypt(msg);
 ///
-/// // Decryption
-/// let dec = cks.decrypt(&ct);
+/// /// let dec = cks.decrypt(&ct);


this looks wrong

IceTDrinker · 2024-03-19T16:33:45Z

tfhe/src/shortint/ciphertext/common.rs

+use super::super::CheckError;
+pub use crate::core_crypto::commons::parameters::PBSOrder;
+use crate::shortint::parameters::{CarryModulus, MessageModulus};
+use serde::{Deserialize, Serialize};
+use std::cmp;
+use std::fmt::Debug;


could be in the mod.rs itself ?

I prefer it this way
I think it's clearer to have the mod only containing other modules (and possibly reexports)

IceTDrinker · 2024-03-19T16:35:22Z

tfhe/src/shortint/ciphertext/mod.rs

+pub use common::*;
+pub use compact_list::*;
+pub use compressed::*;
+pub use standard::*;


let's not have pub use of everything, it was a bad pattern/practice in core_crypto, Thomas said it was not the best way of making things available where required

IceTDrinker · 2024-03-19T16:35:29Z

tfhe/src/shortint/ciphertext/mod.rs

+mod common;
+mod compact_list;
+mod compressed;
+mod standard;


IceTDrinker · 2024-03-19T16:35:58Z

tfhe/src/integer/ciphertext/mod.rs

+pub use base::*;
+pub use boolean_value::*;
+pub use compact_list::*;
+pub use compressed::*;
+pub use utils::*;


pub use the minimum required/which makes sense to compile

tfhe/src/shortint/server_key/mod.rs

IceTDrinker · 2024-03-20T08:26:05Z

https://github.com/zama-ai/tfhe-rs/pull/983/checks?check_run_id=22842849533
https://github.com/zama-ai/tfhe-rs/pull/983/checks?check_run_id=22842848468
https://github.com/zama-ai/tfhe-rs/pull/983/checks?check_run_id=22842846681

benchmarks look ok

IceTDrinker · 2024-03-20T14:28:08Z

tfhe/src/c_api/high_level_api/utils.rs

@@ -617,6 +656,9 @@ macro_rules! impl_scalar_binary_assign_fn_on_type {
    };
 }

+#[cfg(feature = "integer")]


feature gate required ?

No
Removing

IceTDrinker · 2024-03-20T14:30:28Z

cuda test red @mayeul-zama

IceTDrinker

Thanks for the preliminary refactor and most notably for the shortint test fixes !

cla-bot bot added the cla-signed label Mar 13, 2024

mayeul-zama force-pushed the mz/ms_compression_shortint branch from d6e4ac3 to af86fd7 Compare March 13, 2024 14:29

mayeul-zama changed the title ~~Mz/ms compression shortint~~ Mz/ms compression shortint/integer Mar 13, 2024

mayeul-zama force-pushed the mz/ms_compression_shortint branch 2 times, most recently from d687fa6 to 7951e25 Compare March 14, 2024 12:27

mayeul-zama requested a review from IceTDrinker March 14, 2024 14:24

IceTDrinker requested changes Mar 15, 2024

View reviewed changes

mayeul-zama force-pushed the mz/ms_compression_shortint branch 2 times, most recently from eee4ddb to 35b2ee0 Compare March 19, 2024 10:34

IceTDrinker reviewed Mar 19, 2024

View reviewed changes

mayeul-zama force-pushed the mz/ms_compression_shortint branch from 35b2ee0 to f2abd31 Compare March 19, 2024 15:43

mayeul-zama changed the title ~~Mz/ms compression shortint/integer~~ Many refactors Mar 19, 2024

IceTDrinker reviewed Mar 19, 2024

View reviewed changes

mayeul-zama force-pushed the mz/ms_compression_shortint branch 2 times, most recently from 2fae26a to acba6ca Compare March 20, 2024 14:14

IceTDrinker reviewed Mar 20, 2024

View reviewed changes

mayeul-zama force-pushed the mz/ms_compression_shortint branch from acba6ca to 0bcfc17 Compare March 20, 2024 14:46

IceTDrinker approved these changes Mar 20, 2024

View reviewed changes

zama-bot added the approved label Mar 20, 2024

mayeul-zama force-pushed the mz/ms_compression_shortint branch from 0bcfc17 to c2f3c2c Compare March 20, 2024 15:50

zama-bot removed the approved label Mar 20, 2024

IceTDrinker approved these changes Mar 20, 2024

View reviewed changes

zama-bot added the approved label Mar 20, 2024

mayeul-zama added 5 commits March 21, 2024 10:23

fix(shortint): fix and rename tests

40c06d7

refactor(shortint): split shortint parametrized tests in 2 files

8a3312b

chore(tfhe): make macros scoped

5e2a776

chore(tfhe): cleanup unused macros

8ad1cc1

feat(core_crypto): rename modulus switch compression

1ce1586

mayeul-zama added 7 commits March 21, 2024 10:23

refactor(shortint): factorize PBS code

905317d

refactor(shortint): ciphertext module

3651cc2

refactor(integer): ciphertext module

0c02588

chore(tfhe): remove useless comments

31c4515

fix(shortint): fix test

94afe03

fix(core): fix doctest comment

3a4cbd7

style(all): regroup uses

53df75b

mayeul-zama force-pushed the mz/ms_compression_shortint branch from c2f3c2c to 53df75b Compare March 21, 2024 09:24

zama-bot removed the approved label Mar 21, 2024

IceTDrinker approved these changes Mar 21, 2024

View reviewed changes

zama-bot added the approved label Mar 21, 2024

mayeul-zama merged commit 31e2949 into main Mar 21, 2024
36 checks passed

mayeul-zama deleted the mz/ms_compression_shortint branch March 21, 2024 12:58

		@@ -1,490 +1,11 @@
		//! This module implements the ciphertext structures.
		mod base;

Many refactors #983

Many refactors #983

Conversation

mayeul-zama commented Mar 13, 2024

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IceTDrinker commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IceTDrinker commented Mar 20, 2024

IceTDrinker left a comment

Choose a reason for hiding this comment