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

Fedmeta #2438

Merged
merged 157 commits into from
Oct 16, 2023
Merged

Fedmeta #2438

merged 157 commits into from
Oct 16, 2023

Conversation

JinsooKim-KR
Copy link
Contributor

@JinsooKim-KR JinsooKim-KR commented Oct 1, 2023

Description

Implements FedMeta baseline.

Related issues/PRs

See issue 2241

Checklist

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

jafermarq and others added 30 commits June 25, 2023 13:45
Co-authored-by: Daniel J. Beutel <[email protected]>
@JinsooKim-KR
Copy link
Contributor Author

@jafermarq, I have completed the list you suggested. Please let me know what further steps I need to take.

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.

@jinsoogod , thanks for making those changes. You'll notice removed some files that were not part of your baseline/fedmeta (there are a couple more that i need to look into -- not sure what's going on exactly).

Could you in the meantime:

  • Run the format-baseline.sh script again? some things need to change (this will happen automatically)
  • make the suggested changes to the readme.
    Thank you.

baselines/fedmeta/README.md Outdated Show resolved Hide resolved
@JinsooKim-KR
Copy link
Contributor Author

@jafermarq , I've fixed your edits now. Please check

@jafermarq
Copy link
Contributor

@jinsoogod, I have added some updates to fix the formatting and also discard files that weren't part of the baseline. Until now i've only tested your code with Shakespeare (which i'm running now for all 400 rounds). I'm running FEMNIST for the first time and I noticed some errors in the LEAF preprocessing script. See below:

hashed 700000 write images
hashed 800000 write images
finished calculating image hashes
------------------------------
assigning class labels to write images
finished assigning class labels to write images
------------------------------
grouping images by writer
finished grouping images by writer
------------------------------
converting data to .json format
Traceback (most recent call last):
  File "/home/javier/projects/sor/fedmeta/baselines/fedmeta/leaf/data/femnist/preprocess/data_to_json.py", line 64, in <module>
    gray.thumbnail(size, Image.ANTIALIAS)
AttributeError: module 'PIL.Image' has no attribute 'ANTIALIAS'
finished converting data to .json format
------------------------------

[THEN THE REST FAILS ALSO]

Could you:

  • look into this errors? It seems these can be fixed if downgrading Pillow to 9.5, but i'm not sure if this will affect the results you already obtained. What version of Pillow did you have in your environment when running the code? Poetry installs Pillow when doing poetry install and uses version 10.0.1

@JinsooKim-KR
Copy link
Contributor Author

JinsooKim-KR commented Oct 12, 2023

@jafermarq, You are right. LEAF github was created with under pillow version 9.5. I downloaded this data from the existing LEAF Anaconda environment. The current error occurs during the download process. This does not occur during preprocessing that maintains raw data. My Pillow version is 9.5.0

@jafermarq
Copy link
Contributor

Ok, i'll add that as part of pyproject.toml. I'll re-run some of your experiments and if all looks good, we are ready to merge!

@JinsooKim-KR
Copy link
Contributor Author

Thank you for your hard work. I'm waiting for good news.

@jafermarq jafermarq marked this pull request as ready for review October 16, 2023 09:22
@jafermarq jafermarq enabled auto-merge (squash) October 16, 2023 09:23
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.

Looks great! @jinsoogod

@jafermarq jafermarq mentioned this pull request Oct 16, 2023
6 tasks
@jafermarq jafermarq merged commit 07d92a6 into adap:main Oct 16, 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