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 DepthFL baseline #2295

Merged
merged 62 commits into from
Oct 30, 2023
Merged

Add DepthFL baseline #2295

merged 62 commits into from
Oct 30, 2023

Conversation

Peterpan828
Copy link
Contributor

@Peterpan828 Peterpan828 commented Sep 6, 2023

Related issues/PRs

Implementation for #2038

Checklist

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

Any other comments?

@jafermarq jafermarq added the summer-of-reproducibility About a baseline for Summer of Reproducibility label Sep 6, 2023
@Peterpan828 Peterpan828 marked this pull request as ready for review September 8, 2023 07:43
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 @Peterpan828, I took an initial pass through your DepthFL code following the instructions in your readme. I was able to run it so that's great! I have added some small comments below, mostly making suggestions to the pyproject.toml, the readme and one question regarding our previous discussion for stateful clients (I believe none of the prev_grad is needed in the strategies?)

baselines/depthfl/depthfl/strategy.py Outdated Show resolved Hide resolved
baselines/depthfl/pyproject.toml Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Show resolved Hide resolved
baselines/depthfl/README.md Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
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 @Peterpan828 ,

Thanks for incorporating the changes in the previous pass i did. I have left a few more (very minor).

Also, could you run the formatting script? it will detect quite a few issues. You can do so by: (1) sourcing your environment from your baseline directory, (2) cd .., (3) ./dev/format-baseline.sh depthfl

After that you can run the automatic tests with ./dev/test-baseline.sh depthfl

baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/pyproject.toml Outdated Show resolved Hide resolved
baselines/depthfl/pyproject.toml Outdated Show resolved Hide resolved
Peterpan828 and others added 6 commits September 20, 2023 16:44
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
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 @Peterpan828,

Thanks for applying those suggestions. I have a few more. Mostly cosmetic.

Let me know if you need some advice on how to deal with the formatting errors/warning that appear when you run the formatting script. They are generally easy to address.

baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
@jafermarq
Copy link
Contributor

Hi @Peterpan828 , hope things are going well. Please let me know if there is anything you'd like discuss about your baseline. From our last discussion it seems everything is in place result-wise. But i saw you added some extra commits regarding heterofl. Happy to help if some parts need a second pair of eyes or if you are struggling making the ./dev/test-baseline.sh script pass. Just let me know~

@Peterpan828
Copy link
Contributor Author

Hi @jafermarq,

While modifying the code to pass the test, I figured out that the heterofl-related code was not operating properly, so I modified it. As of now, all code seems to be working as intended and the script test has also passed.

@jafermarq
Copy link
Contributor

Hi @jafermarq,

While modifying the code to pass the test, I figured out that the heterofl-related code was not operating properly, so I modified it. As of now, all code seems to be working as intended and the script test has also passed.

By "heterofl-related code was not operating properly" do you mean there was a bug or it was just an issue with tests? If the former, probably some experiments need to be re-run?

You'll notice I pushed some changes to your branch (just some formatting and added a bit of text).

Could you do a final pass through the code ? I think now it is the time to ensure everything is documented.

@Peterpan828
Copy link
Contributor Author

While modifying the code to pass the test, a problem arose, so I fixed it, and actually re-run most of experiments to confirm that there were no more problems.

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 @Peterpan828 ,

Thanks for clarifying the above. Just one final suggestion to edit how the Expected Results section is presented. I think because you have a lot of results (this is great!) there are many commands included in the readme and some are applicable for more than one table. It would be best if we re-arrange the Expected Results section adding a sub-section for each table showing first the command to generate it entirely, then present the table. Then repeat this for all three tables.

What do you think?

baselines/depthfl/README.md Outdated Show resolved Hide resolved
baselines/depthfl/README.md Outdated Show resolved Hide resolved
@Peterpan828
Copy link
Contributor Author

I modified the expected results section as suggested.

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 @Peterpan828, thanks for making those changes.

I have started running the experiment on my side as part of the reviewing process and managed to reproduce Table2. Great!

I have just some questions on the commands for Table 3,4. Please take a look. I'm not entirely sure if those are all people will need to generate those tables.

You'll notice i have also added a commit including reflecting your baseline in the changelog of the repo.

baselines/depthfl/README.md Show resolved Hide resolved
baselines/depthfl/README.md Show resolved Hide resolved
baselines/depthfl/README.md Show resolved Hide resolved
jafermarq
jafermarq previously approved these changes Oct 30, 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.

@Peterpan828 , this looks great! 💯 We'll merge your baseline shortly

@danieljanes danieljanes enabled auto-merge (squash) October 30, 2023 09:50
@danieljanes danieljanes merged commit da9b326 into adap:main Oct 30, 2023
26 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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants