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

Lazy context #197

Merged
merged 7 commits into from
Dec 22, 2020
Merged

Lazy context #197

merged 7 commits into from
Dec 22, 2020

Conversation

bcebere
Copy link
Member

@bcebere bcebere commented Dec 21, 2020

Description

This PR introduces the capability of loading a protobuffer without having the context linked yet.
fixes #193

Checklist

@bcebere bcebere changed the title [WIP]Lazy context Lazy context Dec 21, 2020
@bcebere bcebere requested a review from youben11 December 21, 2020 19:14
Copy link
Member

@youben11 youben11 left a comment

Choose a reason for hiding this comment

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

Nice and clean! I'm now wondering what happens when we run an operation which requires the tenseal_context? A way to deal with this is to always fetch the tenseal_context using the get method tenseal_context(), which will handle the case when the tensor is still lazy loaded, throwing an error message to the user. What do you think?

tenseal/cpp/tensors/encrypted_tensor.h Show resolved Hide resolved
@bcebere
Copy link
Member Author

bcebere commented Dec 21, 2020

Nice and clean! I'm now wondering what happens when we run an operation which requires the tenseal_context? A way to deal with this is to always fetch the tenseal_context using the get method tenseal_context(), which will handle the case when the tensor is still lazy loaded, throwing an error message to the user. What do you think?

There was another attempt in the PR, but I reverted, you can see here that I was going for a "call guardian" at first b2533dc.
But the changes were too big for the gain.

Yeah, I will check to use tenseal_context everywhere, it is easier.

@bcebere bcebere added the Type: New Feature ➕ Introduction of a completely new addition to the codebase label Dec 21, 2020
@youben11
Copy link
Member

Nice and clean! I'm now wondering what happens when we run an operation which requires the tenseal_context? A way to deal with this is to always fetch the tenseal_context using the get method tenseal_context(), which will handle the case when the tensor is still lazy loaded, throwing an error message to the user. What do you think?

There was another attempt in the PR, but I reverted, you can see here that I was going for a "call guardian" at first b2533dc.
But the changes were too big for the gain.

Yeah, I will check to use tenseal_context everywhere, it is easier.

I think as far as it's clear that the tenseal context is a private attribute and shouldn't be used directly (we can mention that in the documentation => this makes me remember that I really need to start documenting my code :p ) we should be good with throwing an error on the getter.

@bcebere
Copy link
Member Author

bcebere commented Dec 21, 2020

Nice and clean! I'm now wondering what happens when we run an operation which requires the tenseal_context? A way to deal with this is to always fetch the tenseal_context using the get method tenseal_context(), which will handle the case when the tensor is still lazy loaded, throwing an error message to the user. What do you think?

There was another attempt in the PR, but I reverted, you can see here that I was going for a "call guardian" at first b2533dc.
But the changes were too big for the gain.
Yeah, I will check to use tenseal_context everywhere, it is easier.

I think as far as it's clear that the tenseal context is a private attribute and shouldn't be used directly (we can mention that in the documentation => this makes me remember that I really need to start documenting my code :p ) we should be good with throwing an error on the getter.

We can make the tenseal contexr private in the encrypted_tensor instead of protected, and this way we'd be sure that everyone is using it via tenseal_context(), even the derived classes.

@youben11
Copy link
Member

Yeah, this would work perfectly I guess

@bcebere
Copy link
Member Author

bcebere commented Dec 22, 2020

Yeah, this would work perfectly I guess

Actually, we already use tenseal_context in the derived classes, but we need to access _context for the lazy loading, instead of throwing an exception. So, there isn't much too do other than adding a guardian for all calls, to do sanity checks. But I think that is a bit overkill.

@bcebere bcebere requested a review from youben11 December 22, 2020 09:05
@youben11
Copy link
Member

Yeah, I also agree that we shouldn't go for the guardian thing. If there isn't any way to allow only the lazy loader to access that private attribute, then we should leave it protected but make it clear that getting that attribute is forbidden by the supreme court

@bcebere
Copy link
Member Author

bcebere commented Dec 22, 2020

Yeah, I also agree that we shouldn't go for the guardian thing. If there isn't any way to allow only the lazy loader to access that private attribute, then we should leave it protected but make it clear that getting that attribute is forbidden by the supreme court

updated

Copy link
Member

@youben11 youben11 left a comment

Choose a reason for hiding this comment

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

Even better this way! LGTM!

@bcebere bcebere merged commit 6232c06 into master Dec 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the lazy_context branch December 22, 2020 14:50
@bcebere bcebere restored the lazy_context branch January 21, 2021 15:19
@bcebere bcebere deleted the lazy_context branch January 21, 2021 15:33
pierreeliseeflory pushed a commit to pierreeliseeflory/TenSEAL that referenced this pull request Apr 27, 2022
* add lazy loading

* python bindings for lazy loading

* update tests

* make context private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: New Feature ➕ Introduction of a completely new addition to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy loading of TenSEALContext
2 participants