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

Flower Baseline: FedAvg MNIST #1497

Merged
merged 72 commits into from
Jan 9, 2023
Merged

Flower Baseline: FedAvg MNIST #1497

merged 72 commits into from
Jan 9, 2023

Conversation

charlesbvll
Copy link
Member

What does this implement/fix? Explain your changes.

Adds a new baseline for MNIST using a CNN in a federated setting.

@charlesbvll
Copy link
Member Author

@danieljanes What I've already tried and tested for the mypy error :
(note that everything is working with Python, the error only pops up with mypy)

  • After import dataset, dir(dataset) contains load_datasets, as expected, but mypy doesn't seem to recognize it
  • Replacing import dataset and dataset.load_datasets by from dataset import load_datasets
  • Replacing import dataset by import dataset as dataset or replacing from dataset import load_datasets by from dataset import load_datasets as load_datasets
  • Playing around with the __init__.py even though all the files are in the same directory, this could be where I could have missed something

@charlesbvll
Copy link
Member Author

@danieljanes What I've already tried and tested for the mypy error : (note that everything is working with Python, the error only pops up with mypy)

  • After import dataset, dir(dataset) contains load_datasets, as expected, but mypy doesn't seem to recognize it
  • Replacing import dataset and dataset.load_datasets by from dataset import load_datasets
  • Replacing import dataset by import dataset as dataset or replacing from dataset import load_datasets by from dataset import load_datasets as load_datasets
  • Playing around with the __init__.py even though all the files are in the same directory, this could be where I could have missed something

I just changed the name of the dataset.py file and it worked, mypy probably had some collision issue with the dataset name.

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

Looks good overall, I left a few comments on some details. In addition to that:

  1. Remove the .5 from the distributed validation plot
  2. Add two empty lines after each module docstring (before the imports)
  3. Add tests, esp. for model and dataset

@danieljanes danieljanes changed the title Mnist baseline Flower Baseline: FedAvg MNIST Dec 8, 2022
@charlesbvll charlesbvll linked an issue Jan 6, 2023 that may be closed by this pull request
20 tasks
@danieljanes danieljanes enabled auto-merge (squash) January 9, 2023 23:01
@danieljanes danieljanes merged commit da1c8f7 into main Jan 9, 2023
@danieljanes danieljanes deleted the mnist_baseline branch January 9, 2023 23:01
tanertopal added a commit that referenced this pull request Jan 12, 2023
* main:
  Fixes colab links (#1560)
  Updates Ray to 2.2.0 and fixes Mypy errors (#1555)
  Upgrade to Poetry 1.3.2 (#1549)
  Add FedAvg MNIST Baseline to README (#1552)
  Fix badge on README (#1553)
  Flower Baseline: FedAvg MNIST (#1497)
  Fix mt-pytorch example (#1545)
  Upgrade Poetry to 1.2.2 (#1547)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Flower Baseline: FedAvg+MNIST
2 participants