feat: support encrypted mul div #690

jfrery · 2024-05-23T07:26:52Z

closes https://github.com/zama-ai/concrete-ml-internal/issues/4418
closes https://github.com/zama-ai/concrete-ml-internal/issues/4163

ref https://github.com/zama-ai/concrete-internal/issues/716

Makefile

src/concrete/ml/quantization/base_quantized_op.py

src/concrete/ml/quantization/quantized_ops.py

RomanBredehoft · 2024-06-11T11:48:55Z

src/concrete/ml/quantization/quantized_ops.py

+            assert min_non_zero_value is not None and min_non_zero_value > 0
+            self.min_non_zero_value = min_non_zero_value
+
+            q_array_divider = QuantizedArray(self.n_bits, 1 / inputs[1])


also, how can we be sure that inputs[1]is not 0 ? I feel like 1 / inputs[1] can fail here no ?

btw are values always positive ? or are we talking about any floats (including negatives) ?

I do this right before:

min_non_zero_value = numpy.min(numpy.abs(inputs[1])) # mypy assert min_non_zero_value is not None and min_non_zero_value > 0

should be good no?

ah yes right, I missed the assert. But does that mean that we always expect values to be strictly positive ? how can we ensure that ?

What I could check is that no value are quant dequant to a 0 maybe

yes but my question is more about "how can you be sure of that ?" !

you are right we do the abs, so my questions is then : how can we be sure that we never get 0 in inputs[1] ? I understand that we assert or could add more assert, but I don't get how we are sure that this/these assert(s) will never ever fail in any circumstances (and if we can't, that's an issue imo) !

We can't. If the user provides a 0 it will fail. As any division fails when there are 0 in the denominator.

ah I see, in that case if it's a user issue, then we should have a raise ValueError with an explicit message rather than an empty assert. Unless an error is triggered before reaching this point (and in that case, is it explicit enough ?)

The assert here is for mypy. Not sure we should rewrite basic error message such as division by 0.

well I won't push too much on this one, but I just fear that the error will not be a clear "DivisionByZeroError" and might confuse the user, would be worth a check or even a test

RomanBredehoft · 2024-06-11T11:52:20Z

src/concrete/ml/quantization/quantized_ops.py

+        input_1 = q_input_1.dequant()
+
+        # Replace input_1 with min_non_zero_qvalue if input_1 is 0
+        input_1 = numpy.where(input_1 == 0, self.min_non_zero_value, input_1)


how is this working ? just replacing 0 with the (float) min ? I imagine that means that values are expected to be positive ! And then still, should we instead add self.min_non_zero_value to ìnput_1`, instead of replacing 0, right ?

in any case we should explain in comments how all this work

Hmm yeah I don't understand how that work either. I did that to fix the calibration but it should fail when compiling the circuit. Unless the numpy.where never does anything? I need to change that indeed thanks.

so what should we do here then ?

Actually, here we are within a PBS and we assign values that are 0 to self.min_non_zero_value. That should work fine within a PBS.

yes but my observation was that, since we replace all zeros with self.min_non_zero_value :

since this value is computed through numpy.min(numpy.abs(inputs[1])) , does that mean we expect input_1 to always be positive ?

if not, for example, [-2, 4, 0] would be replaced by [-2, 4, 2] and I don't think it makes much sense

if so, how can we be sure that input_1 is >= 0 ? more that having asserts I mean (like my comment in https://github.com/zama-ai/concrete-ml/pull/690/files#r1677747936)

also, aren't we breaking the values' distribution by doing something like this ? initially I suggested to instead do something like input_1 += min_non_zero_value (or input_1 += if positive) but don't think that's desirable as well. What about requantizing on [1, max_val] ? Anyway, I feel just replacing 0 by this new val is not exactly right

I totally understand your worry on this however! Let's find the best solution.

oh ok sorry I don't know why I kept seeing argmax and not argmin, so yeah ok we just get the closest value to 0, makes much more sense indeed

would it make sense to instead add an epsilon to this 0 ? or simply to all values ? not sure it'll change much since we re-quantize after the 1/x

apart from that, I'm not sure I have better ideas here . If you want we could discuss this tomorrows yes

would it make sense to instead add an epsilon to this 0 ?

Yes that's what I proposed. An epsilon would make sense if it's represented by the quantized values. The scale is basically the smallest represented floating point. So we could use this instead of the min(abs(x)). But I am not entirely sure we can get the actual quantizer of the input. Also I am afraid that for large bit-width this epsilon is going to be super small and will lead to numerical errors.

yeah numerical errors could be an issue indeed. But actually this epsilon should be added to all values not just the 0 maybe ? in order to keep the same distribution ? not sure how the quant params will handle this though

src/concrete/ml/quantization/quantized_ops.py

RomanBredehoft · 2024-06-11T12:07:34Z

tests/quantization/test_quantized_ops.py

@@ -363,8 +363,9 @@ def test_all_arith_ops(
        # Compute the quantized operator result
        quantized_output_vv = q_op(q_inputs_0, q_inputs_1).dequant()

-        # Check the R2 of raw output and quantized output
-        check_r2_score(raw_output_vv, quantized_output_vv)
+        if n_bits > 16:


are we sure about this change ?

We were running that check twice.

not sure I see where we were doing the second chck 🤔

check_r2_score(raw_output_vv, quantized_output_vv) is right below the if

but you added that one 😅 , my question was about the if statement that you added, like mentioned below

Ah yeah what am saying -_-'. Sorry. So yes the problem is as I said, correctness were fine before with univariate LUT but in QuantizedDiv|Mul we have quant requant which impact the final accuracy.

do we know by how much it can impact ? how "strong" do we expect this impact to be ? I'm just being a bit suspicious here because of things like https://github.com/zama-ai/concrete-ml/pull/690/files#r1634728461 or https://github.com/zama-ai/concrete-ml/pull/690/files#r1634723604 😅

tests/quantization/test_quantized_ops.py

RomanBredehoft

thanks for this feature ! I have several questions and observations, mostly about adding more comments to make the code more clear

andrei-stoian-zama

If the tests pass, this looks good

Soptq · 2024-07-14T11:14:33Z

Can we merge this PR? I am working on compiling a model that involves encrypted mul, currently it prompts "AssertionError: Do not support this type of operation between encrypted tensors". I think this PR could fix this error.

jfrery · 2024-07-15T08:50:50Z

Can we merge this PR? I am working on compiling a model that involves encrypted mul, currently it prompts "AssertionError: Do not support this type of operation between encrypted tensors". I think this PR could fix this error.

Yes sure. This PR has been open for too long already. We will merge it soon.

RomanBredehoft

sorry, I still have some remaining questions !

closes zama-ai/concrete-ml-internal#4163

closes zama-ai/concrete-ml-internal#4418 d

jfrery · 2024-07-22T12:20:22Z

@RomanBredehoft could you re-review this please.

RomanBredehoft

still think something is wrong sorry !

tests/quantization/test_quantized_ops.py

github-actions · 2024-07-26T08:31:34Z

Coverage passed ✅

Coverage details

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

60 files skipped due to complete coverage.

cla-bot bot added the cla-signed label May 23, 2024

jfrery changed the title ~~feat: support encrypted mul div~~ feat: support encrypted mul div [BLOCKED CP] May 23, 2024

jfrery force-pushed the feat/support_encrypted_mul_div branch 2 times, most recently from c458a8a to e370395 Compare May 24, 2024 08:34

jfrery force-pushed the feat/support_encrypted_mul_div branch from 03f7f1f to 4ee14f1 Compare May 30, 2024 12:44

jfrery force-pushed the feat/support_encrypted_mul_div branch 2 times, most recently from fdea8e2 to 2f60349 Compare June 10, 2024 15:28

jfrery changed the title ~~feat: support encrypted mul div [BLOCKED CP]~~ feat: support encrypted mul div Jun 11, 2024

jfrery marked this pull request as ready for review June 11, 2024 09:21

jfrery requested a review from a team as a code owner June 11, 2024 09:21