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 niid-Bench baseline #2428

Merged
merged 83 commits into from
Nov 7, 2023
Merged

Conversation

aashishkolluri
Copy link
Contributor

@aashishkolluri aashishkolluri commented Sep 27, 2023

Issue

Description

Originally, the task has been to reproduce the results from SCAFFOLD paper (Karimireddy el al. 2019). The results in that paper are difficult to reproduce due to missing details on the experimental setup. However, SCAFFOLD has been benchmarked in another work (https://arxiv.org/pdf/2102.02079.pdf) along with other FL protocols FedAvg, FedProx, and FedNova. Therefore, in the best interests of demonstrating SCAFFOLD on Flower, I have implemented the other FL protocols too and reproduced results from this work instead.

Related issues/PRs

Proposal

Explanation

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation
  • Update changelog
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hi @aashishkolluri ,

Thanks for including the changes. I have highlighted a few minor ones. I also run all the experiments. It looks good.

But it seems the ./dev/test-baseline.sh ... command still shows quite a few errors. For those in the form too-many- you can disable them by placing a line like #pylint: disable=too-many-<...> next or above the line triggering the error. But please don't use the disable option for other errors. If there is some error you are having a hard time to fix, let me know and I can probably help.

Once that test passes, we can merge the baseline.

baselines/niid_bench/README.md Outdated Show resolved Hide resolved
baselines/niid_bench/README.md Outdated Show resolved Hide resolved
baselines/niid_bench/README.md Outdated Show resolved Hide resolved
baselines/niid_bench/README.md Outdated Show resolved Hide resolved
baselines/niid_bench/README.md Outdated Show resolved Hide resolved
@aashishkolluri aashishkolluri marked this pull request as ready for review November 6, 2023 12:28
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hi @aashishkolluri ,

Thanks for applying those many changes since my last review. Your code and results look good so i'm happy to merge this baseline. But before we do this just one small thing: part of the tests check for the existence of client.py, server.py, strategy.py (which you didn't have). I added them as pretty much empty files. But i used that opportunity to add your custom strategies into strategy.py and I made the necessary changes elsewhere in the code (including the configs) so everything should work as before.

➡️ Could you please double check the changes i did in my last commit?

Also, if there is anything you'd like to add or modify to your readme, now it's the time.

Let me know when you have a couple of minutes to double check the above. Maybe running a couple of experiments to see everything remains the same won't hurt.

All test in the GitHub CI pass

@aashishkolluri
Copy link
Contributor Author

Hi @aashishkolluri ,

Thanks for applying those many changes since my last review. Your code and results look good so i'm happy to merge this baseline. But before we do this just one small thing: part of the tests check for the existence of client.py, server.py, strategy.py (which you didn't have). I added them as pretty much empty files. But i used that opportunity to add your custom strategies into strategy.py and I made the necessary changes elsewhere in the code (including the configs) so everything should work as before.

➡️ Could you please double check the changes i did in my last commit?

Also, if there is anything you'd like to add or modify to your readme, now it's the time.

Let me know when you have a couple of minutes to double check the above. Maybe running a couple of experiments to see everything remains the same won't hurt.

All test in the GitHub CI pass

Hi @jafermarq ,

I have checked your changes in the last commit and everything seems fine to me. I have also run a few commands from the Readme again to check the results and they are as expected.

@jafermarq jafermarq changed the title NIID-bench Add niid-Bench baseline Nov 7, 2023
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

@aashishkolluri , thanks for confirming. This baseline is ready to merge then! amazing work 🚀

@jafermarq jafermarq enabled auto-merge (squash) November 7, 2023 09:44
@jafermarq jafermarq merged commit 5252f67 into adap:main Nov 7, 2023
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
summer-of-reproducibility About a baseline for Summer of Reproducibility
Projects
Development

Successfully merging this pull request may close these issues.

3 participants