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

add FedBN Implementation on NVFlare research folder - a local batch normalization federated learning method #2524

Merged
merged 37 commits into from May 10, 2024

Conversation

MinghuiChen43
Copy link
Contributor

@MinghuiChen43 MinghuiChen43 commented Apr 20, 2024

Fixes # .

Description

Implementation of FedBN (https://arxiv.org/pdf/2102.07623.pdf) - local batch normalization with NVFlare

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

@chesterxgchen
Copy link
Collaborator

can you add description so we know what this PR is about ?

@MinghuiChen43 MinghuiChen43 changed the title add research/fedbn add research/fedbn to Implementation of FedBN - a local batch normalization federated learning method with NVFlare Apr 24, 2024
@MinghuiChen43 MinghuiChen43 changed the title add research/fedbn to Implementation of FedBN - a local batch normalization federated learning method with NVFlare add FedBN Implementation on NVFlare research folder - a local batch normalization federated learning method Apr 24, 2024
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

This PR request quite some of changes.
It seems to be the author simply copied some of the examples and put all the files into this PR, even most of them are not even used or needed.

I think it might be better to reference to ML2FL or step-by-step/cifar10 examples for the Client side training using Client API and the server side use BaseFedAvg as base.

Also leverage nvflare job CLI to update job config rather write python files.

delete un-used files, and avoid copy the files in FLARE src files.

Here is a PR using Client API on the client side, and BaseFedAvg API on the server side you can probably reference :

#2529

@YuanTingHsieh
Copy link
Collaborator

/build

chesterxgchen
chesterxgchen previously approved these changes May 9, 2024
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

Overall looks good, thank you for your contribution.

I add few minor suggestion, but its not critical, its update to you to change or not.

auto-merge was automatically disabled May 9, 2024 05:34

Head branch was pushed to by a user without write access

@MinghuiChen43 MinghuiChen43 dismissed stale reviews from ZiyueXu77 and chesterxgchen via 9d7fac5 May 9, 2024 05:34
@ZiyueXu77
Copy link
Collaborator

/build

ZiyueXu77
ZiyueXu77 previously approved these changes May 9, 2024
@ZiyueXu77
Copy link
Collaborator

Hi @MinghuiChen43 could you try signing your commits (setting up some keys etc. in your local machine)? We have this requirement for our repo. See instruction below:

The base branch requires all commits to be signed. Learn more about signing commits.

Thanks!

@ZiyueXu77 ZiyueXu77 self-requested a review May 9, 2024 20:20
@MinghuiChen43
Copy link
Contributor Author

Hi @MinghuiChen43 could you try signing your commits (setting up some keys etc. in your local machine)? We have this requirement for our repo. See instruction below:

The base branch requires all commits to be signed. Learn more about signing commits.

Thanks!

I have signed my recent commits. Do I need to have all my commits verified? It might be difficult to go back and sign the previous commits.

@ZiyueXu77
Copy link
Collaborator

/build

Hi @MinghuiChen43 could you try signing your commits (setting up some keys etc. in your local machine)? We have this requirement for our repo. See instruction below:
The base branch requires all commits to be signed. Learn more about signing commits.
Thanks!

I have signed my recent commits. Do I need to have all my commits verified? It might be difficult to go back and sign the previous commits.

Yes we do need to have all signed, I think we can squash the commits in that case, @YuanTingHsieh any suggestions on this?

@MinghuiChen43
Copy link
Contributor Author

/build

Hi @MinghuiChen43 could you try signing your commits (setting up some keys etc. in your local machine)? We have this requirement for our repo. See instruction below:
The base branch requires all commits to be signed. Learn more about signing commits.
Thanks!

I have signed my recent commits. Do I need to have all my commits verified? It might be difficult to go back and sign the previous commits.

Yes we do need to have all signed, I think we can squash the commits in that case, @YuanTingHsieh any suggestions on this?

I used the command git rebase --exec 'git commit --amend --no-edit -n -S' -i xxx to sign old commits, as suggested in an answer I found (https://superuser.com/questions/397149/can-you-gpg-sign-old-commits). Every commit seemed to be verified successfully, but I failed to verify the initial commit. I'm wondering if there is anything inappropriate about my approach and how I can fix the issue with verifying the initial commit.

@ZiyueXu77
Copy link
Collaborator

@chesterxgchen , we have some issue with signing requirements, ccould you merge it?

@ZiyueXu77
Copy link
Collaborator

/build

@YuanTingHsieh YuanTingHsieh merged commit 297a079 into NVIDIA:main May 10, 2024
16 checks passed
@YuanTingHsieh
Copy link
Collaborator

@MinghuiChen43 @ZiyueXu77 in this case, we can only force merge, or rebase the whole commit into one and sign that specific one.

But no worries, I just merge it, thanks for the effort!

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.

None yet

10 participants