Skip to content
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.

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

Upgrade SEAL to 3.5.6 #105

Merged
merged 6 commits into from
Jul 21, 2020
Merged

Conversation

xueyumusic
Copy link
Contributor

Description

The versions of SEAL from 3.5.1 to 3.5.6 has fixed many bugs and some optimizations. This PR tried to upgrade SEAL from 3.5.1 to 3.5.6.
Then updated places are:

  1. Since SEAL 3.5.6 removed util/polyarith.h and util/polyarithmod.h, removing related parts.
  2. Updated some places related with util/polyarithsmallmod.h changes.
  3. Updated some function call params using ConstRNSIter and RNSIter because of seal function param changes
  4. Add MultiplyUIntModOperand python binding and update coeff_div_plain_modulus return type

Affected Dependencies

SEAL lib

How has this been tested?

pytest -v --ignore=tests/sealapi ./tests
cmake . -D BUILD_TEST=TRUE
make && CTEST_OUTPUT_ON_FAILURE=1 make test
pytest -v ./tests/sealapi

test cases under tests dir

Checklist

Copy link
Member

@bcebere bcebere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, great work!

I've left just some observations.
It seems that we had some issues with the pointer parameters all along, pybind won't map pointers as expected.

Also, it would be nice to have some basic tests for the new classes, just to see that the bindings are functional.

[](const RNSTool &obj, const std::uint64_t *input, std::size_t count,
std::uint64_t *destination) {
obj.sm_mrq(ConstRNSIter(input, count), RNSIter(destination, count), MemoryManager::GetPool());
[](const RNSTool &obj, const std::uint64_t *input,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pybind doesn't map uint64_t* to a vector, it will consider it a single value.

I think it better to use vector instead of (uint64, size_t). and call it using (input.data(), input.size()).

LE: Now I see that the issue was there all along, it's not new. It was documented here https://github.com/OpenMined/TenSEAL/blob/master/tenseal/sealapi/sealapi_util_namespace.cpp#L30

but we didn't fix it everywhere.

Please replace the (std::uint64_t *, size_t) parameters with std::vector, to have the full functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thans for review. I updated the codes to use std::vector as params, also since the seal's ConstRNSIter and RNSIter need a poly_modulus_degree param which is not equal with vector size, so add poly_modulus_degree in some places.

@@ -716,6 +505,12 @@ void bind_util_namespace(pybind11::module &m) {
* } "util/hash.h"
*******************/

py::class_<MultiplyUIntModOperand>(m, "MultiplyUIntModOperand",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have some sanity (py)tests for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test_util_multiplyuintmodoperand_sanity test case in test_util.py

std::size_t poly_modulus_degree) {
obj.fastbconv_sk(
ConstRNSIter(input.data(), poly_modulus_degree),
RNSIter(destination.data(), poly_modulus_degree),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't destination vector be resized to input.size()?

now, by default, it will be empty. isn't that going to lead to a crash?

Copy link
Member

@bcebere bcebere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! LGTM!

@bcebere bcebere requested a review from youben11 July 20, 2020 15:56
@youben11 youben11 merged commit 4f3ce8b into OpenMined:master Jul 21, 2020
pierreeliseeflory pushed a commit to pierreeliseeflory/TenSEAL that referenced this pull request Apr 27, 2022
* upgrade SEAL to 3.5.6

* black format

* clang-format sealapi_util_namespace.cpp

* address comment, update vector param, add testcase

* fix dest vector param and as return type

Co-authored-by: Ayoub Benaissa <ayouben9@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants