add ABY3 protocol #721
add ABY3 protocol #721
Conversation
jvmncs
left a comment
jvmncs
left a comment
There was a problem hiding this comment.
just some brief preliminary notes from my end
| sess.run(fit_batch_op, tag="fit-batch") | ||
|
|
||
| def loss(self, sess, x, y, player_name): | ||
| def print_loss(y_hat, y): |
There was a problem hiding this comment.
do these outputs in cleartext break privacy? (cc @mortendahl)
There was a problem hiding this comment.
do these outputs in cleartext break privacy? (cc @mortendahl)
Yes, I leave it here only for debugging. Normal runs should not reveal loss.
There was a problem hiding this comment.
a comment to that effect would be much appreciated :)
mortendahl
left a comment
mortendahl
left a comment
There was a problem hiding this comment.
Partial review but looks amazing so far -- lots of great stuff in here, very excited!
A few general notes:
- we should update formatting to match with rest of the code base
- we should stick with TF naming when possible; this will make it easier for a user to get the semantics
| fit_batch_op = self.backward(x, dy, self.init_learning_rate) | ||
| return fit_batch_op | ||
|
|
||
| def fit(self, sess, x, y, num_batches): |
There was a problem hiding this comment.
Should it be num_epochs instead of num_batches, since each iteration is using the entire training set?
There was a problem hiding this comment.
could also follow sklearn's lead and use something like num_iters https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html
There was a problem hiding this comment.
num_iters might be a better choice. Each iteration is using only a batch of data, not the entire training set.
| import numpy as np | ||
|
|
||
|
|
||
| def close(x, y, threshold = 0.01): |
There was a problem hiding this comment.
This seems to be the same as eg np.testing.assert_allclose(x, x, rtol=0.0, atol=threshold); if so we should just use that instead.
| """ | ||
| return self.prot.reduce_max(self, axis) | ||
|
|
||
| def consistency_check(self, shares, inst_type): |
There was a problem hiding this comment.
Is this a leftover? Doesn't seem to be used anywhere.
There was a problem hiding this comment.
Is this a leftover? Doesn't seem to be used anywhere.
Indeed, should have deleted it :(
|
|
||
| def _sub_public_private(prot, x, y): | ||
| assert isinstance(x, ABY3PublicTensor), type(x) | ||
| assert isinstance(y, ABY3PrivateTens |
There was a problem hiding this comment.
Note that the with contexts won't do anything here since z is a Python array.
|
|
||
| def _sub_public_private(prot, x, y): | ||
| assert isinstance(x, ABY3PublicTensor), type(x) | ||
| assert isinstance(y, ABY3PrivateTens |
There was a problem hiding this comment.
I suggest we either leave these out (in which case dispatch will raise an error) or add a # TODO + exception, instead of just a pass (which will silently fail by returning None).
There was a problem hiding this comment.
Agree with raising an error in the current version.
I think we should update this to match with the rest of the code base; I'd be happy to take this on. |
|
|
||
| def _sub_public_private(prot, x, y): | ||
| assert isinstance(x, ABY3PublicTensor), type(x) | ||
| assert isinstance(y, ABY3PrivateTens |
There was a problem hiding this comment.
Indeed, might be legacy code. Will solve it in the next commit
| print(sess.run(z.reveal())) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
pytest seems to pick up these tests on its own so perhaps this listing of tests could be avoided altogether (or eg replaced by unittest.main() as in pond_test.py)? see #736
| import numpy as np | ||
|
|
||
|
|
||
| def close(x, y, threshold = 0.01): |
| @@ -0,0 +1,1043 @@ | |||
| import tensorflow as tf | |||
There was a problem hiding this comment.
File should be renamed to aby3_test.py to match rest of codebase; see #736
| sess.run(fit_batch_op, tag="fit-batch") | ||
|
|
||
| def loss(self, sess, x, y, player_name): | ||
| def print_loss(y_hat, y): |
mortendahl
left a comment
mortendahl
left a comment
There was a problem hiding this comment.
Let's merge this one up! Amazing work! 🥇
There are a few unresolved comments left that we can address in another PR; I have opened issues for all of them.
3 participants
I have added a new protocol "ABY3" to the library, and made minimal necessary changes in
tf_encrypted/tensor/native.pyto support the protocol.The current ABY3 implementation contains computation for replicate arithmetic sharing and replicate boolean sharing. For Yao sharing, it is not supported yet.
NOTE. Boolean sharing is used in two ways:
tf_encrypted/tensor/boolfactory.pyto support this.The tests are included in the file
tf_encrypted/protocol/aby3/test_aby3.py.BTW, I use 4-space indent for my new files, which is different from yours. Although this does not affect the correctness, if 2-space indent is absolutely necessary, please let me know and I can update to your standard.