fix: key compression always #726
Conversation
| @@ -347,7 +347,7 @@ def _get_training_quantized_module( | |||
| # Enable the underlying FHE circuit to be composed with itself | |||
| # This feature is used in order to be able to iterate in the clear n times without having | |||
| # to encrypt/decrypt the weight/bias values between each loop | |||
| configuration = Configuration(composable=True) | |||
| configuration = Configuration(composable=True, compress_evaluation_keys=True) | |||
There was a problem hiding this comment.
compress pbs keys for training
There was a problem hiding this comment.
which makes me thinkg, it should be using enable_input_compression as well here !
There was a problem hiding this comment.
yes, you're right, same as for DF which was not enabled before
There was a problem hiding this comment.
actually same as below, we should probably revert this change : it's enabled by QM's compilation anyway, so it's a bit confusing to add it here
DF is different because it directly compiles with CP tho yes
| @@ -40,6 +40,8 @@ def run_hybrid_llm_test( | |||
| # Multi-parameter strategy is used in order to speed-up the FHE executions | |||
| configuration = Configuration( | |||
| single_precision=False, | |||
| compress_input_ciphertexts=True, | |||
There was a problem hiding this comment.
these circuits can be fully leveled or not, always good to test with default compilation parameters
There was a problem hiding this comment.
so why not also enabling key compression ?
There was a problem hiding this comment.
it is enabled by the QM compilation (the env variable defaults to 1)
There was a problem hiding this comment.
so is compress_input_ciphertexts then right ? imo either we put both here or we revert this small change 🤔
| @@ -120,6 +122,8 @@ def function_to_compile(x): | |||
| enable_unsafe_features=is_fast, | |||
| use_insecure_key_cache=is_fast, | |||
| insecure_key_cache_location=keyring_dir_as_str, | |||
| compress_input_ciphertexts=True, | |||
There was a problem hiding this comment.
sanity checks should run with default compile parameters
conftest.py
Outdated
| @@ -173,6 +174,7 @@ def simulation_configuration(): | |||
| fhe_simulation=True, | |||
| fhe_execution=False, | |||
| compress_input_ciphertexts=os.environ.get("USE_INPUT_COMPRESSION", "1") == "1", | |||
| compress_eval_keys=True, | |||
There was a problem hiding this comment.
tests should use the default for compressed keys, which is True
There was a problem hiding this comment.
imo we shouldn't force it like this but do like for input compression, meaning we use env variables. The main reason is that forcing it like this makes it impossible to disable it for tests or other things through input configuration objects. Basically this is what I reverted for fhe/simu exec in #724 (which is going to conflict a bit with your PR btw)
additionally we probably want to add tests to make sure that the compression properly works, like we did for input compression
andrei-stoian-zama
force-pushed
the
fix/compress_keys_always
branch
from
82f7ef0
to
0b7e4e4
Compare
andrei-stoian-zama
force-pushed
the
fix/compress_keys_always
branch
from
0b7e4e4
to
3a8dca0
Compare
Coverage passed ✅Coverage details
|
Enable key compression for all models