feat: expose statuses to compile torch #261

fd0r · 2023-09-20T13:13:28Z

No description provided.

src/concrete/ml/quantization/quantized_module.py

src/concrete/ml/torch/compile.py

tests/quantization/test_quantized_module.py

RomanBredehoft

thanks for this ! I have a few comments

jfrery

This could be needed indeed thanks!

jfrery · 2023-09-21T09:20:59Z

src/concrete/ml/quantization/quantized_module.py

@@ -577,6 +577,7 @@ def compile(
        p_error: Optional[float] = None,
        global_p_error: Optional[float] = None,
        verbose: bool = False,
+        statuses: Optional[Dict[str, str]] = None,


I don't feel like statuses is very talkative. Should be referring to the input I think. input_statuses or input_encryption where the user passes booleans in a list following the order of the input in the forward call? so that you don't have to specify onnx node names.

fd0r · 2023-09-21T20:30:27Z

Looks like KNN test is flaky

jfrery

Looks great thanks!

fd0r · 2023-09-22T07:44:27Z

What I did:

changed the name of the variable to a more explicit naming.
updated the doc-strings to something clearer.
added match= to the tests
changed the type of the variable from a dict to a list to avoid having to handle onnx names

RomanBredehoft · 2023-09-22T09:11:05Z

src/concrete/ml/quantization/quantized_module.py

@@ -598,9 +599,15 @@ def compile(
                error to a default value.
            verbose (bool): Indicate if compilation information should be printed
                during compilation. Default to False.
+            inputs_encryption_status (Optional[Sequence[str]]): encryption status ('clear',


I would still mention that everything is encrypted if the parameter is set to None !

It's mentionned in all the user facing APIs, I don't think many users will use this one tbh

I mean it was mostly for making the docstring consistent with what this parameters does, as we now provide a way to chose what inputs to encrypt, to avoid confusion that we don't chose which ones are encrypted or not based on whatever other rules. But sure if no one else pushes for this then go for it

RomanBredehoft · 2023-09-22T09:11:17Z

src/concrete/ml/torch/compile.py

@@ -142,6 +143,8 @@ def _compile_torch_or_onnx_model(
        global_p_error (Optional[float]): probability of error of the full circuit. In FHE
            simulation `global_p_error` is set to 0
        verbose (bool): whether to show compilation information
+        inputs_encryption_status (Optional[Sequence[str]]): encryption status ('clear', 'encrypted')


Same what? There is a comment on the fact that by default everything is encrypted

same as in "same comment as above" in order to not avoid making changes in both places if there are some

tests/quantization/test_quantized_module.py

RomanBredehoft

I still have a few comments

fd0r · 2023-09-22T09:29:02Z

Adressed the feedback made by @RomanBredehoft

fd0r · 2023-09-22T11:28:08Z

KNN flaky, relaunching

RomanBredehoft · 2023-09-22T12:12:01Z

src/concrete/ml/quantization/quantized_module.py


        Returns:
            Circuit: The compiled Circuit.
+
+        Raises:
+            ValueError: if inputs_encryption_status does not match with the


it's a bit more than that right ?

Well it's if the configuration is wrong, how would you formulate it?

I would usually very quickly describe what are bad configurations

github-actions · 2023-09-22T12:52:03Z

Coverage passed ✅

Coverage details

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL    6178      0   100%

51 files skipped due to complete coverage.

RomanBredehoft

if no one pushes for these last comments, fine for me, thanks a lot for this feature !

cla-bot bot added the cla-signed label Sep 20, 2023

fd0r force-pushed the 3829-add-encryption-status-per-input-in-compile_torch_model branch from 137570e to 02d8c10 Compare September 20, 2023 15:09

fd0r marked this pull request as ready for review September 20, 2023 18:43

fd0r requested a review from a team as a code owner September 20, 2023 18:43

andrei-stoian-zama previously approved these changes Sep 21, 2023

View reviewed changes