BFVTensor #285

comidan · 2021-04-19T20:41:56Z

Description

This PR tries to add support for BFVTensor.

Checklist

I have followed the Contribution Guidelines and Code of Conduct
I have commented my code following the OpenMined Styleguide
I have labeled this PR with the relevant Type labels
My changes are covered by tests

bcebere

Thank you for the contribution. It is a major step in the right direction!

One significant problem is that we should reuse the encrypted_tensor interface, which should be the root class for BFVTensor.
There is also a bit of code duplication between CKKSTensor and BFVTensor, but we can think in another PR about templating the classes.

Regarding the failing tests, it looks like a linting problem. Make sure that you run
.github/workflows/scripts/lint_cpp.sh
and
.github/workflows/scripts/lint_python.sh

and they are successful before pushing. You might need to install clang-format, flake8 and black locally for that.

Other than that, awesome work so far!

bcebere · 2021-04-20T05:29:53Z

cmake/tests.cmake

@@ -16,7 +16,8 @@ if(${BUILD_TEST})
      ${TENSEAL_TESTS_BASEDIR}/tensors/ckksvector_test.cpp
      ${TENSEAL_TESTS_BASEDIR}/tensors/ckkstensor_test.cpp
      ${TENSEAL_TESTS_BASEDIR}/tensors/plaintensor_test.cpp
-      ${TENSEAL_TESTS_BASEDIR}/tensors/bfvvector_test.cpp)
+      ${TENSEAL_TESTS_BASEDIR}/tensors/bfvvector_test.cpp


Why the duplicate cpp file?

No sorry probably I just copy pasted and changed again after having saved, those lines should be appearing:

${TENSEAL_TESTS_BASEDIR}/tensors/bfvvector_test.cpp ${TENSEAL_TESTS_BASEDIR}/tensors/bfvtensor_test.cpp)

bcebere · 2021-04-20T05:32:51Z

tenseal/cpp/tensors/BUILD

        "ckkstensor.h",
        "ckksvector.h",
        "encrypted_tensor.h",
+        "encrypted_tensor_bfv.h",


encrypted_tensor should be a generic interface for all tensors in TenSEAL? Why do we need encrypted_tensor_bfv too?

Yes you are right of course, sorry.
I just got a little confused with the scale method in encrypted_tensor.h given that it is not supposed to be used in BFV and indeed only now I've discovered that it's declared as not implemented for BFV (for the BFVVector counterpart I mean).
Also the same for polyval_inplace method which was using double so I initially changed the data type to the template plain_data_t but it was messing with BFVVector which was using double as template but the method itself was again declared as not implemented, so I did the same now for BFVTensor.

Sorry for the confusion

bcebere · 2021-04-20T05:37:25Z

tenseal/cpp/tensors/encrypted_tensor_bfv.h

@@ -0,0 +1,263 @@
+#ifndef TENSEAL_TENSOR_ENCRYPTED_TENSOR_BFV_H


Why do we need a separate interface from encrypted_tensor?

No we don't sorry again, the wrong reason was the one mentioned above

bcebere · 2021-04-20T05:38:14Z

tenseal/proto/tensors.proto

+    // The serialized ciphertexts
+    repeated bytes ciphertexts = 2;
+    // Optional batch size. Exists only if batching is enabled
+    uint32 batch_size = 4;


Why 4? this can be index 3.

I should stop copy-pasting to be faster and forgetting about it, sorry.
I deleted the scale which was not needed but forgot indeed to change the index to 3.

bcebere · 2021-04-20T05:39:14Z

tests/cpp/tensors/bfvtensor_test.cpp

+using namespace std;
+
+template <class Iterable>
+bool are_close(const Iterable& l, const std::vector<int64_t>& r) {


are_close is relevant only for CKKS. For BFV, you should get the exact result. See the BFVVector C++ tests for reference

Right thank you, I changed it now to assert/expect the exact result indeed

bcebere · 2021-04-20T05:40:12Z

tests/python/tenseal/tensors/test_bfv_tensor.py

+]
+
+
+def _almost_equal_number(v1, v2, m_pow_ten):


Similarly, _almost_equal_number is only for CKKS. Here you should test for the exact result. See the test_bfv_vector tests for reference.

Yes right, thank you

comidan · 2021-04-20T16:15:14Z

Thank you for the contribution. It is a major step in the right direction!

One significant problem is that we should reuse the encrypted_tensor interface, which should be the root class for BFVTensor.
There is also a bit of code duplication between CKKSTensor and BFVTensor, but we can think in another PR about templating the classes.

Regarding the failing tests, it looks like a linting problem. Make sure that you run
.github/workflows/scripts/lint_cpp.sh
and
.github/workflows/scripts/lint_python.sh

and they are successful before pushing. You might need to install clang-format, flake8 and black locally for that.

Other than that, awesome work so far!

Ok thank you for the detailed explanation.
I run both of the scripts and it does not seem to give me any kind of errors (I run them after compiling and building everything).
The only output of the first script is the diff output from git, as the script indeed does after calling clang-format.
While the second one it tells me "All done!".

So I think it's ok and I'll proceed to push the changes.

bcebere · 2021-04-20T16:20:59Z

Thank you for the contribution. It is a major step in the right direction!
One significant problem is that we should reuse the encrypted_tensor interface, which should be the root class for BFVTensor.
There is also a bit of code duplication between CKKSTensor and BFVTensor, but we can think in another PR about templating the classes.
Regarding the failing tests, it looks like a linting problem. Make sure that you run
.github/workflows/scripts/lint_cpp.sh
and
.github/workflows/scripts/lint_python.sh
and they are successful before pushing. You might need to install clang-format, flake8 and black locally for that.
Other than that, awesome work so far!

Ok thank you for the detailed explanation.
I run both of the scripts and it does not seem to give me any kind of errors (I run them after compiling and building everything).
The only output of the first script is the diff output from git, as the script indeed does after calling clang-format.
While the second one it tells me "All done!".

So I think it's ok and I'll proceed to push the changes.

The diff you are seeing contains the actual linting errors that break the build.

But my bad, it can be a bit confusing:

lint_cpp.sh runs clang-format inplace, you should commit the diff, if any.
lint_python.sh does a dry-run, but it doesn't change anything. If lint_python.sh outputs anything at all, just run in the root of the repository black . to lint the py scripts.

comidan · 2021-04-20T16:49:09Z

Thank you for the contribution. It is a major step in the right direction!
One significant problem is that we should reuse the encrypted_tensor interface, which should be the root class for BFVTensor.
There is also a bit of code duplication between CKKSTensor and BFVTensor, but we can think in another PR about templating the classes.
Regarding the failing tests, it looks like a linting problem. Make sure that you run
.github/workflows/scripts/lint_cpp.sh
and
.github/workflows/scripts/lint_python.sh
and they are successful before pushing. You might need to install clang-format, flake8 and black locally for that.
Other than that, awesome work so far!

Ok thank you for the detailed explanation.
I run both of the scripts and it does not seem to give me any kind of errors (I run them after compiling and building everything).
The only output of the first script is the diff output from git, as the script indeed does after calling clang-format.
While the second one it tells me "All done!".
So I think it's ok and I'll proceed to push the changes.

The diff you are seeing contains the actual linting errors that break the build.

But my bad, it can be a bit confusing:

lint_cpp.sh runs clang-format inplace, you should commit the diff, if any.

lint_python.sh does a dry-run, but it doesn't change anything. If lint_python.sh outputs anything at all, just run in the root of the repository black . to lint the py scripts.

Ok thank you, I run black . at the root of the repository and it considered two more files than the ones considered by the script to reformat. It also prints at then end of the log "All done!".
Regarding the diff, ok I'll then commit them!

comidan · 2021-04-20T17:51:22Z

Ok now with the new commit it shows that by running the python tests some of them pass but then it stops at one giving indeed the same error I get locally: ValueError: SEAL: couldn't find context_data from params_id,

add polyval to BFVTensor BFVTensor tests bugs: remove scaling factor for BFV, use int64_t instead of int, fix context construction

bcebere · 2021-04-21T06:33:50Z

Ok now with the new commit it shows that by running the python tests some of them pass but then it stops at one giving indeed the same error I get locally: ValueError: SEAL: couldn't find context_data from params_id,

I pushed some fixes, you can see the summary from the commit.

you don't need rescaling or coeff modulus for BFV
BFV doesn't need modulus switch, that's why you saw that error.
the C++ tests weren't compiling because you were using vector instead of vector<int64_t>

I haven't checked the python tests, though.

comidan · 2021-04-21T06:38:34Z

Ok now with the new commit it shows that by running the python tests some of them pass but then it stops at one giving indeed the same error I get locally: ValueError: SEAL: couldn't find context_data from params_id,

I pushed some fixes, you can see the summary from the commit.

you don't need rescaling or coeff modulus for BFV

BFV doesn't need modulus switch, that's why you saw that error.

the C++ tests weren't compiling because you were using vector instead of vector<int64_t>

I haven't checked the python tests, though.

Thank you!

Yes my bad, I indeed removed rescaling and coefficient modulus from various methods but not everywhere, sorry for the confusion.

Ok I'll wait for the tests to run here on GitHub and then I'll further check the python tests for problems, if any.

comidan · 2021-04-21T10:30:02Z

I pushed fixes to the python tests of BFVTensor, now locally they all work! Should also then work everything here in theory.
There were few issues with the shapes passed to numpy randint and some tests thought to be for CKKS polyval which did not make sense here.

bcebere

LGTM! Great work! Thank you for your contribution, @comidan!

I've left just one minor observation, after that we can merge the PR.

bcebere · 2021-04-21T12:15:42Z

tenseal/cpp/tensors/bfvtensor.h

+   public:
+    using EncryptedTensor<int64_t, shared_ptr<BFVTensor>>::decrypt;
+    /**
+     * Create a new BFVTensor from an 1D vector.


The comment here is not correct, the BFVTensor can be created from tensors of any shape

LGTM! Great work! Thank you for your contribution, @comidan!

I've left just one minor observation, after that we can merge the PR.

Thank you for the help!

Sure ok, I've just pushed the new header file with the fixed comment.

bcebere · 2021-04-21T14:27:49Z

tenseal/cpp/tensors/bfvtensor.h

+     * @param[in] input vector.
+     * @param[in] input vector.
+     * @param[in] input vector.
+     */


Sorry, but the comments still don't make sense.

Why the

* @param[in] input vector. * @param[in] input vector. * @param[in] input vector.

3 times as a vector ?

Ok @bcebere I'll change it accordingly, I just left the same comments which were already present in CKKSTensor.h assuming they were correct.

Should I then change also the ones for CKKSTensor?

Yes, please. Thank you so much for improving this!

bcebere · 2021-04-22T05:25:48Z

Thank you for your contribution, @comidan!

comidan · 2021-04-22T08:03:23Z

Thank you for your contribution, @comidan!

Thank you for the help in making it!

* BFVTensor * Fix BFVTensor python tests * Update bfvtensor.h * Update ckkstensor.h Co-authored-by: Bogdan Cebere <bogdan.cebere@gmail.com>

BFVTensor

1e0ac98

comidan mentioned this pull request Apr 19, 2021

BFV Tensor #280

Closed

bcebere requested changes Apr 20, 2021

View reviewed changes

BFVTensor

522b2d9

disable mod_switch for BFV

29db4aa

add polyval to BFVTensor BFVTensor tests bugs: remove scaling factor for BFV, use int64_t instead of int, fix context construction

Fix BFVTensor python tests

5dc5110

bcebere approved these changes Apr 21, 2021

View reviewed changes

Update bfvtensor.h

0c878fb

bcebere reviewed Apr 21, 2021

View reviewed changes

comidan and others added 3 commits April 21, 2021 21:19

Update bfvtensor.h

f0fdce5

Update ckkstensor.h

5ff2ac0

update Bazel timeouts

60bc432

bcebere merged commit 0e5f119 into OpenMined:master Apr 22, 2021

pierreeliseeflory pushed a commit to pierreeliseeflory/TenSEAL that referenced this pull request Apr 27, 2022

BFVTensor (OpenMined#285)

885a9d5

* BFVTensor * Fix BFVTensor python tests * Update bfvtensor.h * Update ckkstensor.h Co-authored-by: Bogdan Cebere <bogdan.cebere@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BFVTensor #285

BFVTensor #285

comidan commented Apr 19, 2021 •

edited

bcebere left a comment

bcebere Apr 20, 2021

comidan Apr 20, 2021

bcebere Apr 20, 2021

comidan Apr 20, 2021

bcebere Apr 20, 2021

comidan Apr 20, 2021

bcebere Apr 20, 2021

comidan Apr 20, 2021

bcebere Apr 20, 2021

comidan Apr 20, 2021

bcebere Apr 20, 2021

comidan Apr 20, 2021

comidan commented Apr 20, 2021

bcebere commented Apr 20, 2021 •

edited

comidan commented Apr 20, 2021

comidan commented Apr 20, 2021

bcebere commented Apr 21, 2021 •

edited

comidan commented Apr 21, 2021 •

edited

comidan commented Apr 21, 2021

bcebere left a comment

bcebere Apr 21, 2021

comidan Apr 21, 2021 •

edited

bcebere Apr 21, 2021 •

edited

comidan Apr 21, 2021

bcebere Apr 21, 2021

bcebere commented Apr 22, 2021

comidan commented Apr 22, 2021

		@@ -0,0 +1,263 @@
		#ifndef TENSEAL_TENSOR_ENCRYPTED_TENSOR_BFV_H

BFVTensor #285

BFVTensor #285

Conversation

comidan commented Apr 19, 2021 • edited

Description

Checklist

bcebere 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

comidan commented Apr 20, 2021

bcebere commented Apr 20, 2021 • edited

comidan commented Apr 20, 2021

comidan commented Apr 20, 2021

bcebere commented Apr 21, 2021 • edited

comidan commented Apr 21, 2021 • edited

comidan commented Apr 21, 2021

bcebere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comidan Apr 21, 2021 • edited

Choose a reason for hiding this comment

bcebere Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcebere commented Apr 22, 2021

comidan commented Apr 22, 2021

comidan commented Apr 19, 2021 •

edited

bcebere commented Apr 20, 2021 •

edited

bcebere commented Apr 21, 2021 •

edited

comidan commented Apr 21, 2021 •

edited

comidan Apr 21, 2021 •

edited

bcebere Apr 21, 2021 •

edited