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

Dev caesar vfl #346

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Dev caesar vfl #346

wants to merge 17 commits into from

Conversation

qbc2016
Copy link
Collaborator

@qbc2016 qbc2016 commented Aug 24, 2022

an example for seCure lArge-scalE SpArse logistic Regression (caesar) vertical federated learning

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2022

CLA assistant check
All committers have signed the CLA.

@xieyxclack xieyxclack self-requested a review August 24, 2022 06:49
@xieyxclack xieyxclack added the enhancement New feature or request label Aug 24, 2022
Copy link
Collaborator

@xieyxclack xieyxclack left a comment

Choose a reason for hiding this comment

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

  • Maybe we can merge the caesar_v_fl and vertical_fl together, so that some functionalities can be reused, such as paillier, dataloader, and utils.py.
  • Please resolve the conflicts in federatedscope/core/auxiliaries/data_builder.py
  • A unittest is required for evaluating the correctness.
  • Some minor suggestions are listed inline, and I would review the details of server/client later

@@ -0,0 +1,47 @@
# You can refer to pyphe for the detail implementation. (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is the same as federatedscope/vertical_fl/Paillier/abstract_paillier.py, maybe you can reuse it.

@@ -0,0 +1,13 @@
### Caesar Vertical Federated Learning

We provide an example for seCure lArge-scalE SlArse logistic Regression (caesar) vertical federated learning, you can run with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the references here.

Copy link
Collaborator

@xieyxclack xieyxclack left a comment

Choose a reason for hiding this comment

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

LGTM, please see the inline comments, thanks!

super(SecretSharing, self).__init__()
assert shared_party_num > 1, "AdditiveSecretSharing require " \
"shared_party_num > 1"
self.shared_party_num = shared_party_num
self.maximum = 2**size
self.mod_number = 2 * self.maximum + 1
self.epsilon = 1e8
self.epsilon = 1e4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is 1e-4 enough for ensuring precise results?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, setting 1e-8 will not be better, and sometimes worse depending on the size below

@@ -24,14 +24,14 @@ class AdditiveSecretSharing(SecretSharing):
AdditiveSecretSharing class, which can split a number into frames and
recover it by summing up
"""
def __init__(self, shared_party_num, size=60):
def __init__(self, shared_party_num, size=20):
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, setting size=20 is not secure here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how about 50? I tried epsilon=1e4 and size=50, and the acc is 0.82. Making size larger, the acc will decrease.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure why the acc would be affected by the size

X[:, j] = 0
return X

def normalize(X):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant?

@@ -39,15 +68,17 @@ def load_vertical_data(config=None, generate=False):
# For Client #1
data[1] = dict()
data[1]['train'] = {
'x': x[:train_num, :config.vertical.dims[0]],
'y': y[:train_num]
'x': x[:train_num, :config.caesar_vertical.dims[0]]
}
data[1]['val'] = None
data[1]['test'] = test_data

# For Client #2
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we assume that client_2 owns the labels? maybe we can add some annotations here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get it

@@ -0,0 +1,18 @@
### Caesar Vertical Federated Learning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can move this README.md to /federatedscope/vertical/ and merge it with that of secure_LR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok


return secret_seq

def secret_split_for_piece_of_ss(self, secret):
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the differences between secret_sharing and simple_secret_sharing is the function secret_split and secret_split_for_piece_of_ss? So maybe the class AdditiveSecretSharing in simple_secret_sharing can be inherited from that of secret_sharing. Or just add a config (e.g., vertical.use_for_pieceof_ss)?

if not self.own_label:
self.a_computes()

# A computes <z>_1 = <z_a>_1 + <<z_a>_2>_1 + <<z_b>_1>_1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use client_with_label/client_without_label rather than A/ B to make this more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I'll fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants