-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Falcon - Select share + evaluating #4474
Conversation
af7d52d
to
a5206f2
Compare
@@ -82,13 +88,13 @@ def retrieve_pointers(self): | |||
return pointers | |||
|
|||
def __sum_shares(self, shares): | |||
return sum(shares) % self.ring_size | |||
return sum(shares) % self.field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abogaziah changed the name to field to be consistent with the AdditiveSharingTensor
+ it seems also in CrypTen they use the same name attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree with that, field stands for the whole arithmetic field ]0, Z [, which makes it an incorrect and misleading name for the variable that represents Z only. also, the paper uses "ring size" so I think we should stick to it
syft/frameworks/torch/mpc/falcon.py
Outdated
when selecting between x or y | ||
|
||
Return: | ||
x_share or b_share depending on the value of b_share |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: x_share or y_share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
syft/frameworks/torch/mpc/falcon.py
Outdated
c_share_L = c.share(*workers, protocol="falcon", field=field) | ||
c_share_2 = c.share(*workers, protocol="falcon", field=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find the secret sharing scheme you are using (can you share a link?), but if we want to be consistent with the protocol it should be
- replicated secret sharing , same as for
b
- it should be maliciously secure secret sharing
syft/frameworks/torch/mpc/falcon.py
Outdated
field: int, workers: List[BaseWorker] | ||
) -> (ReplicatedSharingTensor, ReplicatedSharingTensor): | ||
""" | ||
TODO: Need to move this in Primitive Store: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is returning two different sharing which is not indicated from the name of the function and also makes it less general. why not return a single sharing and call the function twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm..in that case I think I can completely remove this function - doing that now
syft/frameworks/torch/mpc/falcon.py
Outdated
If the bit b_share is 0, x_sh is returned | ||
If the bit b_share is 1, y_sh is returned | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a general comment: The sharing used in ReplicatedSharingTensor (https://github.com/OpenMined/PySyft/pull/3856/files) is for semi-honest adversary and falcon is secure against malicious adversary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - currently the RST and the Falcon protocol are "combined" inside the RST (the \pi_mul operation is encapsulated in the RST) - if we add more protocols over RST we can have something like we have in the aditive_sharing_tensor.py
@crypto_protocl("fss")
def add(self, other):
# here we define how our operation over the AST should work if we use the FSS protocol
syft/frameworks/torch/mpc/falcon.py
Outdated
shape = x_share.shape | ||
|
||
pow_sh = ( | ||
ReplicatedSharingTensor.one_shares(players, field=field, shape=shape) - beta_share * 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that you need to share a 1 for that. a much more efficient way will be to do (see Falcon paper, Linear operations):
party1 share = 1-2[beta1]
party2 share = -2[beta2]
party3 share = -2[beta3]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I left a comment there - basically doing this I think becomes a "little" intrusive with the RST - but it is worth thinking if we can rewrite this - hopefully, we will finish falcon soon and we will start benchmarking it and see where we can "get some juice"
syft/frameworks/torch/mpc/falcon.py
Outdated
@staticmethod | ||
def _share_random_tensor( | ||
field: int, workers: List[BaseWorker] | ||
) -> (ReplicatedSharingTensor, ReplicatedSharingTensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional): When using annotations, it is cleaner to use Tuple
from typing instead of the tuple syntax from python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this function
@@ -33,6 +33,18 @@ def __share_secret(self, plain_text, workers): | |||
PRZS.setup(workers) | |||
return shares_map | |||
|
|||
@staticmethod | |||
def one_shares(players, field, shape=(1,)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional): Ret type annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an issue for adding types annotations for all stuff related to falcon here
) | ||
|
||
@staticmethod | ||
def zero_shares(players, field, shape=(1,)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional): Ret type annotations
return ReplicatedSharingTensor( | ||
plain_text=torch.zeros(shape, dtype=torch.int64), players=players, field=field | ||
) | ||
|
||
@staticmethod | ||
def __arrange_workers(workers): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional): Ret type annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will create a ticket for adding annotations to all the elements in the falcon related implementation
a846a11
to
2422fb9
Compare
syft/frameworks/torch/mpc/przs.py
Outdated
@@ -5,7 +5,7 @@ | |||
import torch | |||
import syft | |||
|
|||
RING_SIZE = 2 ** 32 | |||
MAX_LIMIT = 2 ** 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really the ring size - is the maximum value for the random value (for the common randomness)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you know they going to sum up to ring size if you don't know what is the ring size
( alpha1 + alpha2+ alpha3 )% ring size should be zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep...it is. will redo
Codecov Report
@@ Coverage Diff @@
## master #4474 +/- ##
==========================================
+ Coverage 94.79% 94.82% +0.02%
==========================================
Files 209 209
Lines 21377 21419 +42
==========================================
+ Hits 20265 20310 +45
+ Misses 1112 1109 -3
|
syft/frameworks/torch/mpc/przs.py
Outdated
@@ -5,7 +5,7 @@ | |||
import torch | |||
import syft | |||
|
|||
RING_SIZE = 2 ** 32 | |||
MAX_LIMIT = 2 ** 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you know they going to sum up to ring size if you don't know what is the ring size
( alpha1 + alpha2+ alpha3 )% ring size should be zero
@@ -11,3 +14,71 @@ def conv2d(filters: syft.ReplicatedSharingTensor, image, padding=0): | |||
output_size = (image_height - filter_height + 2 * padding) + 1 | |||
result = result.view(-1, channels_out, output_size, output_size) | |||
return result | |||
|
|||
|
|||
def select_share( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a subroutine, it should be moved to the helper class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
x_share: ReplicatedSharingTensor, | ||
y_share: ReplicatedSharingTensor, | ||
) -> ReplicatedSharingTensor: | ||
"""Performs select share protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select share is a subroutine not protocol, it would be better to describe the arguments and the return directly
like
return: x if b=0. y if b=1
and remove the rest of the comment
|
||
|
||
def select_share( | ||
b_share: ReplicatedSharingTensor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a share, it's a shared value, it would be better to call them x, y, b
|
||
# TODO: Think of a method to do this better (1 - c_share) without being to | ||
# "intrusive" with the operation | ||
ones_share = ReplicatedSharingTensor.one_shares(players, ring_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not coherent with the interface please remove it and just use torch.ones.share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
return selected_share | ||
|
||
|
||
def evaluate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evaluate is like the worst name ever a routine can have 😄 that's basically what every routine in the world does, it evaluates some expression, it doesn't tell you anything about what this routine does.
this also should be moved to the helper class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you noticed this routine determines the negativity of x, if beta is an even number this will return x, if beta is odd it will return - x, you can choose a name that explains this functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will rename it
"""Performs the computation (-1)^beta * x | ||
|
||
Args: | ||
x_share (ReplicatedSharingTensor): share to "evaluate" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the "share" suffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
value: Union[int, ReplicatedSharingTensor], other: Union[int, ReplicatedSharingTensor] | ||
) -> ReplicatedSharingTensor: | ||
""" | ||
Compute the XOR value between value and other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the docstring, xor is self-documented it doesn't need comments, "you should see every comment as a failure to express yourself with code"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to do docstring because if we move to syft_0.3.0
that will fail if we do not have every function documented. For a newcomer, the documentation might help.
The XOR computation between value and other | ||
""" | ||
|
||
if not any(isinstance(x, ReplicatedSharingTensor) for x in (value, other)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same expression will work on ints, this expression adds not-needed overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use FalconHelper
for simple ints. We should use int_a
^ int_b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a check to make sure we are doing everything correctly - we could remove this after we finish the implementation.
@@ -34,6 +34,18 @@ def __share_secret(self, plain_text, workers): | |||
PRZS.setup(workers) | |||
return shares_map | |||
|
|||
@staticmethod | |||
def one_shares(players, ring_size, shape=(1,)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this, we need the interface to be as small as possible, you can just use torch.ones().share()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - tried to follow the model from AST
, but we might refactor that
selected_val = (y - x) * d + x | ||
return selected_val | ||
|
||
def negate_cond( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of a conditional negate - I do not know if this is the best name for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that negate_con doesn't mean conditional negative, do not use abbreviations just use the whole word until you come up with a better name
There was a problem hiding this 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 changes, it improved significantly 🎉 just few more changes
|
||
|
||
class FalconHelper: | ||
XOR_EXPECTED_RING_SIZE = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this a global variable thought it's only in xor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm..this is not a global variable, it is accessed only using the class, but I think you are right, we can move it inside the xor method
If value and other are both ints we should use the "^" operator. | ||
|
||
Args: | ||
value (ReplicatedSharingTensor or Int): RST with ring size of 2 or Int value in {0, 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value should be always RST, other could be RST for private xor, or long tensor or int for public xor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I made it like this to be able to have xor(int, RST)
- I can remove it
): | ||
raise ValueError("If both arguments are RST then they should be in ring size 2") | ||
|
||
# Make sure we have a ReplicatedSharingTensor as value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comments before merging to the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
# This should be generated by a crypto provider or create a 0 RST shared value and make | ||
# each worker add a random number at an offline step | ||
# (Idea): Use the correlated randomness shared at the beginning | ||
c = torch.randint(high=2, size=(1,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the size be 1 or x.shape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading that part of the paper again - yeah it should be x.shape
ring_size = x.ring_size | ||
players = x.players | ||
|
||
# This should be generated by a crypto provider or create a 0 RST shared value and make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove
selected_val = (y - x) * d + x | ||
return selected_val | ||
|
||
def negate_cond( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that negate_con doesn't mean conditional negative, do not use abbreviations just use the whole word until you come up with a better name
players = x.players | ||
shape = x.shape | ||
|
||
pow_sh = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove abbreviations
shape = x.shape | ||
|
||
pow_sh = ( | ||
torch.ones(size=shape).share(*players, protocol="falcon", field=ring_size) - beta * 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use public multiplication and addition here to save some communication rounds: (beta*-2)+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - we can do. I had a feeling that it would require multiple multiple rounds of communications if we reorder the operations, but the public multiplication is done locally.
# "intrusive" with the operation | ||
ones = torch.ones(size=(1,)).share(*players, protocol="falcon", field=ring_size) | ||
if xor_b_c.reconstruct().item(): | ||
d = ones - c_L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use public operations here: d= -1*c_L +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on RST to enable executing 1-c_L directly, it should be ready shortly
e3a1250
to
65e9baf
Compare
The XOR computation between value and other | ||
""" | ||
|
||
if not isinstance(value, ReplicatedSharingTensor) or value.ring_size != 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have all those types in place - which look pretty bad - let's keep them in place only to make sure we define the protocol correctly and then remove them
@@ -178,8 +178,8 @@ def __public_linear_operation(self, plain_text, operator): | |||
shares_map[players[0]][1], | |||
) | |||
shares_map[players[-1]] = ( | |||
operator(shares_map[players[-1]][-1], remote_plain_text[-1]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 has share x1, x2; P2 has x2,x3 and P3 has x3 and x1 - when adding a constant we will have
P1 has share x1 + c, x2, P2 has share x2,x3 (nothing changed) and P3 has x3 and x1 + c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! just two final notes on the code
@@ -11,6 +11,18 @@ def test_sharing(workers): | |||
assert type(secret.child) == dict | |||
|
|||
|
|||
def test_share_float(workers): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RST only supports torch.long, deterministic, little, and fast interface is better than a slow wide one, we will use FPT to support floating-point arithmetic (which will pass torch.long to this class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...in that case when generating the shares we should raise an exception is the passed Tensor is a different type than long, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that in my last PR
import torch | ||
from syft.frameworks.torch.mpc.falcon.falcon_helper import FalconHelper | ||
|
||
import itertools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests should not include any iterations nor conditions (from clean coding book 😄) just simple expressions that tell what the interface does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I will manually create the TEST_VALS
list with all values we except
assert (FalconHelper.xor(y, y).reconstruct() == torch.tensor(0)).all() | ||
assert (FalconHelper.xor(y, x).reconstruct() == torch.tensor(1)).all() | ||
|
||
with pytest.raises(ValueError) as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last request: let's move the assertion inside the body of the code, the tests should only test the interface and helper functions, we can use assertions in code during construction it would make it easier to find and remove them before moving it to production
for example, put (assert x.ring_size == 2) inside the xor function instead of if statements and remove the corresponding test case
also better manage whitespace, related expressions should be in a continuous chunk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not ok to have asserts in the "real" code because they can be removed if you run with the -O
or -OO
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's the point, you should remove the asserts before pushing to production, asserts makes it easier to do that if you want your checks not to be moved to production to improve performance
@@ -48,7 +48,7 @@ def __arrange_workers(workers): | |||
|
|||
def generate_shares(self, plain_text, number_of_shares=3): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If generate_shares
is public shouldn't we move the __validation
function here?
I will also remove line 51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not part of the interface, I left it public just to test it
Co-authored-by: Muhammed Abogazia <33666625+abogaziah@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!! thank you for the PR and the changes ❤🎉
We make a good team! 👍 |
""" | ||
|
||
assert ( | ||
isinstance(value, ReplicatedSharingTensor) and value.ring_size == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: This looks kind of ugly but this is how black formatted it :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem :D
assert ( | ||
isinstance(value, ReplicatedSharingTensor) and value.ring_size == 2 | ||
), "First argument should be a RST with ring size 2" | ||
assert any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Do you prefer this to be split into 3 asserts? (one for each type?) @abogaziah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no difference, anyone would work
* Add select share * Black formatting * Ring size to field * Add evaluation * Remove print * Basic operations + tests * Remove falcon test * Fix flake8 * Rename test * Comments fix * Comments fix * Remove the changes from przs * Remove changes from przs test * More changes from przs * Fix flake8 * Add select share and determine sign * Apply suggestions from code review Co-authored-by: Muhammed Abogazia <33666625+abogaziah@users.noreply.github.com> * Formatting * Test fix * Remove notebook * Cleanup xor and tests * Fix formatting * Remove itertools Co-authored-by: Muhammed Abogazia <33666625+abogaziah@users.noreply.github.com>
Description
This PR implements the share selection from the Falcon paper and the evaluation part.
Fixes the first point and the last from from #3852
How has this been tested?
Checklist