TensealContext serialization #101
Conversation
bcebere
added
the
Type: New Feature ➕
bcebere
changed the title
bcebere
changed the title
bcebere
changed the title
bcebere
changed the title
fix make_public_context crash
bcebere
changed the title
There was a problem hiding this comment.
This is truly amazing! I'm really happy to see serialization working perfectly!
I left some comments and I have also a general comment about how different des/serialization methods are used, ones using strings and others using streams. I see that for the one using strings, you are first saving it into a stream than returning the string, isn't this operation more consuming than maybe calling SerializeToString on the buffer? Some function save function might than only fill the buffer, with two other function either saving using SerializeToString or SerializeToOstream, but I'm not sure if this is the right way, so your call.
| @@ -52,6 +52,7 @@ jobs: | |||
| pytest -v --ignore=tests/sealapi ./tests | |||
| - name: Test with gtest | |||
| run: | | |||
| cd tenseal/proto && cmake . && make && cd ../../ | |||
There was a problem hiding this comment.
isn't tenseal_protobuf built as included in the cmake file?
There was a problem hiding this comment.
Unfortunately, here I'm missing something about cmake builds. For some reason, it doesn't generate the proto headers for the build otherwise.
On the other hand, setup.py generates them just fine, and I cannot spot the actual difference.
There was a problem hiding this comment.
when I build the tenseal without BUILD_TEST argument, the build is successful.
example:
cmake .
make
we don't have to build tenseal_proto manually, but when passing the argument BUILD_TEST=TRUE, the build is failed, and we have to build tenseal_proto manually.
I think we are missing something in Unit Tests section in CMakeLists.txt file.
| #ifndef TENSEAL_UTILS_SCOPE_H | ||
| #define TENSEAL_UTILS_SCOPE_H | ||
|
|
||
| #include <functional> | ||
|
|
||
| namespace tenseal { | ||
|
|
||
| class scope_guard { | ||
| public: | ||
| template <class Callable> | ||
| scope_guard(Callable&& undo_func) try | ||
| : f(std::forward<Callable>(undo_func)) { | ||
| } catch (...) { | ||
| undo_func(); | ||
| throw; | ||
| } | ||
|
|
||
| scope_guard(scope_guard&& other) : f(std::move(other.f)) { | ||
| other.f = nullptr; | ||
| } | ||
|
|
||
| ~scope_guard() { | ||
| if (f) f(); // must not throw | ||
| } | ||
|
|
||
| void dismiss() noexcept { f = nullptr; } | ||
|
|
||
| scope_guard(const scope_guard&) = delete; | ||
| void operator=(const scope_guard&) = delete; | ||
|
|
||
| private: | ||
| std::function<void()> f; | ||
| }; | ||
| } // namespace tenseal | ||
| #endif |
There was a problem hiding this comment.
Can I know what this is doing?
There was a problem hiding this comment.
Scoped guards implement a classic C++ idiom - RAII, Resource Acquisition is Initialization. shared_ptr is another example of that.
Long story short, It will get a lambda function on the constructor, and it will call it on its destructor.
The use case is simple: I want to add some logic on a function return. For example, I want to free some resources, a create a scope guard with a lambda that frees the resources, and no matter how many returns I have in the function, the resources will get freed on exit, because the variable's destructor gets called.
As you can see, it calls on destrctuor the lambda received at creation
~scope_guard() {
if (f) f();
}
My issue here was that, while writing serialization test cases, I observed that when calling make_context_public with a true value for galois or relin keys, there was a crash
This is the reason, I am to blame
https://github.com/OpenMined/TenSEAL/blame/master/tenseal/tensealcontext.cpp#L124
I was making the secret key null before creating the galois/relin keys.
The fix is to create a scoped guard that will make the secret key null on exit, no matter how many conditional returns there will be. Something like this, and I'm sure it will make the secret key null everytime
https://github.com/OpenMined/TenSEAL/blob/protobuffs/tenseal/tensealcontext.cpp#L166
I hope the explanation makes sense.
There was a problem hiding this comment.
I could open another PR for this issue, it was just a "side effect" from the serialization tests
There was a problem hiding this comment.
Thanks for the explanation. It's fine to keep it in this PR, but feel free to open another one if it's convenient for you.
Co-authored-by: Bilal Retiat <bilalphilomath@gmail.com>
Co-authored-by: Bilal Retiat <bilalphilomath@gmail.com>
Co-authored-by: Bilal Retiat <bilalphilomath@gmail.com>
Co-authored-by: Bilal Retiat <bilalphilomath@gmail.com>
* add protobuffers * add serialization logic for SEAL objects * add serialization methods for context * overwrite deepcopy & pickle logic with protobuffers serialization * pytests>ests * make_public_context crash. Fixed with a scoped guard.
Description
Related to #72
How has this been tested?
Checklist