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.

Sign up for GitHub

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

Jump to bottom

feat(optimizer): add support for weak function composition #629

Merged

BourgerieQuentin merged 4 commits into main from feat/function_composition

Jan 4, 2024

Collaborator

aPere3 commented Dec 8, 2023

This PR adds support for weak composition of functions to the optimizer>compiler>frontend.

cla-bot bot added the cla-signed label

aPere3 force-pushed the feat/function_composition branch from 3ff7a09 to fc93160 Compare

December 8, 2023 15:00

Collaborator Author

aPere3 commented Dec 11, 2023

@slab-ci compiler-cpu-build

Collaborator Author

aPere3 commented Dec 11, 2023

@slab-ci concrete-python-tests-linux

aPere3 force-pushed the feat/function_composition branch from fc93160 to 998e348 Compare

December 11, 2023 09:14

rudy-6-4 self-requested a review

December 12, 2023 14:09

aPere3 force-pushed the feat/function_composition branch from 998e348 to 403012f Compare

December 13, 2023 14:47

Collaborator Author

aPere3 commented Dec 13, 2023

@slab-ci compiler-cpu-build

Collaborator Author

aPere3 commented Dec 13, 2023

@slab-ci concrete-python-tests-linux

aPere3 force-pushed the feat/function_composition branch from 403012f to 8bd485a Compare

December 13, 2023 16:47

rudy-6-4 reviewed

View reviewed changes

compilers/concrete-compiler/compiler/include/concretelang/Support/V0Parameters.h Outdated Show resolved Hide resolved

rudy-6-4 reviewed

View reviewed changes

compilers/concrete-compiler/compiler/lib/Bindings/Python/CompilerAPIModule.cpp Outdated

                          [](CompilationOptions &options, double global_p_error) {
                            options.optimizerConfig.global_p_error = global_p_error;
                          })
+                    .def("set_allow_composition",

Contributor

rudy-6-4 Dec 19, 2023

maybe auto_composition instead of composition. How will it be called for composition of a set of function ?

Collaborator Author

aPere3 Jan 2, 2024

Why should it be a different option for a set of function ? If you prefer the auto_composition label I can change it yes. Do you want me to change it in the whole PR not just the python api ?

rudy-6-4 reviewed

View reviewed changes

compilers/concrete-compiler/compiler/src/main.cpp Outdated Show resolved Hide resolved

rudy-6-4 reviewed

View reviewed changes

compilers/concrete-optimizer/concrete-optimizer/src/dag/unparametrized.rs Show resolved Hide resolved

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs

+                      // Verify that there is no input symbol in the symbolic variances of the outputs.
+                      assert_no_input_var_in_out_var(&dag, &out_variances, nb_partitions);
+                      // Get the largest output out_variance
+                      let largest_output_variances = dag

Contributor

rudy-6-4 Dec 19, 2023

To put in a function (def conservative_final_variance),
And a for loop here would be a lot better. The max-like operation could be in symbolic_variance.

let mut conservative_final_variance = SymbolicVaraince::ZERO;
for (_, _, final_variance)  in extra_final_variances(...) {
    conservative_final_variance = conservative_final_variance.component_wise_max(final_variance);
}

Contributor

rudy-6-4 Dec 19, 2023

In fact this max already exists.
https://github.com/zama-ai/concrete/blob/main/compilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/symbolic_variance.rs#L170

Collaborator Author

aPere3 Jan 2, 2024

Yes it is the max used line 80 below. It is true that here the combinator expression is a bit complex, but this is probably due to the nestedness of out_variances that is a Vec<Vec<SymbolicVariance>>. I'll try to come with something leaner, but in any case we will still have the nestedness somewhere. I am not sure it is gonna be a lot better to have indirection in the middle of it, but I can change it if you are more comfortable with it.

Also, could you elaborate on what extra and conservative stand for here ? I am also not sure about what the word final stands for. It seems to be used to mean output but is there some more meaning to it in the codebase ?

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs

+                  symbolic_variances: &[Vec<SymbolicVariance>],
+                  nb_partitions: usize,
+              ) {
+                  dag.get_output_index()

Contributor

rudy-6-4 Dec 19, 2023

use the extracted variances that are to be re-injected. here conservative_final_variance. so you don't need to redo everything and duplicate code

Collaborator Author

aPere3 Jan 2, 2024

I don't see what duplication you refer to, but indeed, doing the assertion after computing the largest output variances for each partitions will save some ops.

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs

                   nb_partitions: usize,
                   instr_partition: &InstructionPartition,
+                  input_override: Option<Vec<SymbolicVariance>>,

Contributor

rudy-6-4 Dec 19, 2023

why copying the Option (and Vec) here ? can't you use a ref and only clone the symbolic variance if needed ?

Collaborator Author

aPere3 Jan 2, 2024

What copy do you refer to ?

Contributor

rudy-6-4 Jan 3, 2024

Vec is a copy, Option is a copy

Collaborator Author

aPere3 Jan 8, 2024

Well, in rust sense, there is no copy here, it is a move of an existing value. In the calling code, the input_override is just created for this function to call and not used after, so no copy occurs. I mean, in non-optimized code, maybe you have an on-stack copy of a pointer (which the Vec is) but nothing like allocating a new heap storage and copying the elements.

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs Outdated Show resolved Hide resolved

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs Outdated

@@ @@ -51,6 +51,7 @@ pub fn analyze( @@
                   noise_config: &NoiseBoundConfig,
                   p_cut: &Option<PrecisionCut>,
                   default_partition: PartitionIndex,
+                  allow_composition: bool,

Contributor

rudy-6-4 Dec 19, 2023

should be in noise_config, it could be added later to the other solvers, and when more complex composition will be added it will not change the function signature.

Collaborator Author

aPere3 Jan 2, 2024

Hmm ok, I will do it if you want, but I don't get how this is related to noise bounds and how as an argument it is so different from the p_cut and default_partition parameters.

rudy-6-4 reviewed

View reviewed changes

...concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/partitionning.rs Outdated Show resolved Hide resolved

aPere3 force-pushed the feat/function_composition branch 2 times, most recently from 16431af to 47271cf Compare

January 2, 2024 17:03

rudy-6-4 reviewed

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs Outdated

+                  let mut out_variances = self::out_variances(&dag, nb_partitions, &instrs_partition, &None);
+                  if allow_composition {
+                      // Verify that there is no input symbol in the symbolic variances of the outputs.
+                      assert_no_input_var_in_out_var(&dag, &out_variances, nb_partitions);

Contributor

rudy-6-4 Jan 3, 2024

should be an error here instead of an assert

Contributor

rudy-6-4 Jan 3, 2024

maybe a field could be added,
like support_composition=true/false so the error is not part of the analysis

BourgerieQuentin force-pushed the feat/function_composition branch 6 times, most recently from 91a7026 to 905f646 Compare

January 4, 2024 12:34

rudy-6-4 reviewed

View reviewed changes

.../concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/optimize/mod.rs Outdated Show resolved Hide resolved

BourgerieQuentin force-pushed the feat/function_composition branch from 905f646 to 4fcb632 Compare

January 4, 2024 13:35

rudy-6-4 approved these changes

View reviewed changes

...ilers/concrete-optimizer/concrete-optimizer/src/optimization/dag/multi_parameters/analyze.rs Outdated Show resolved Hide resolved

rudy-6-4 approved these changes

View reviewed changes

aPere3 and others added 4 commits

January 4, 2024 14:56


          feat(optimizer): adds support for function composition

e10d622


          fix(compiler/optimizer): Returns NotComposable error if not lookup ta…

c0740f1

…ble in dag


          test(compiler): Add disabled test on composition (to fix later)

1c2d77f


          fix(compiler/optimizer): Returns error for optimization strategy that…

7c80781

… doesn't support composition

BourgerieQuentin force-pushed the feat/function_composition branch from 4fcb632 to 7c80781 Compare

January 4, 2024 13:59

BourgerieQuentin merged commit 1583571 into main

26 of 29 checks passed

BourgerieQuentin deleted the feat/function_composition branch

January 4, 2024 14:00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels