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

Remove protocol parameter in decrypt #3599

Merged
merged 6 commits into from
May 28, 2020
Merged

Remove protocol parameter in decrypt #3599

merged 6 commits into from
May 28, 2020

Conversation

youben11
Copy link
Member

@youben11 youben11 commented May 23, 2020

Description

The PR remove the need to a protocol parameter in decrypt and warn the user to not use it, without raising an error.

Resolves #3329

Checklist:

  • My changes are covered by tests.
  • I have run the pre-commit hooks to format and check my code for style issues.
  • I have commented my code following Google style.

(See the the contribution guidelines for additional tips.)

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #3599 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3599      +/-   ##
==========================================
+ Coverage   94.58%   94.62%   +0.04%     
==========================================
  Files         160      160              
  Lines       17179    17192      +13     
==========================================
+ Hits        16248    16268      +20     
+ Misses        931      924       -7     
Impacted Files Coverage Δ
...ft/frameworks/torch/tensors/interpreters/native.py 91.30% <100.00%> (+0.07%) ⬆️
test/torch/tensors/test_tensor.py 100.00% <100.00%> (ø)
syft/execution/plan.py 94.70% <0.00%> (+2.18%) ⬆️

@youben11 youben11 requested a review from LaRiffle May 23, 2020 16:05
Copy link
Contributor

@LaRiffle LaRiffle left a comment

Choose a reason for hiding this comment

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

Thanks a lot!
Can you check if there is a test to update/add to illustrate your change?

protocol = kwargs.get("protocol", None)
if protocol:
warnings.warn("protocol should no longer be used in decrypt")

Copy link
Contributor

Choose a reason for hiding this comment

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

Beware of the case where we have an AutogradTensor

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that passing tests meant it's working fine, but yeah there was no tests for decrypting an AutogradTensor. Fixed that and added test for decrypting with/without grad

@youben11 youben11 requested a review from LaRiffle May 26, 2020 09:33
Copy link
Contributor

@LaRiffle LaRiffle left a comment

Choose a reason for hiding this comment

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

🙌

@youben11 youben11 merged commit 50031be into master May 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the youben11/decrypt branch May 28, 2020 15:41
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.

Call decrypt() without specifying the protocol
2 participants