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

FedStar #2482

Merged
merged 118 commits into from
Feb 19, 2024
Merged

FedStar #2482

merged 118 commits into from
Feb 19, 2024

Conversation

Raj-Parekh24
Copy link
Contributor

Issue

#2042

Description

Implementation of FedStar in flower.

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?

@jafermarq jafermarq added the summer-of-reproducibility About a baseline for Summer of Reproducibility label Oct 6, 2023
@Raj-Parekh24 Raj-Parekh24 mentioned this pull request Oct 6, 2023
6 tasks
@jafermarq jafermarq marked this pull request as draft October 6, 2023 19:59
@Raj-Parekh24
Copy link
Contributor Author

Hi @jafermarq ,
I have replicated all codes and suggestions in this PR and also checked the working of the code. Please review the code.

Thanks for the support!

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 @Raj-Parekh24,

I have now completed a first pass through your code. I have highlighted a few formatting issues in the README.md. In general the main points are:

  • It's better not to rely on tensorflow_addons since TF warns to not use.
  • I ran your code for 100 rounds with the default config (i.e. ambient_context dataset) but I only saw ~40% accuracy on the server side.
  • I'm not entirely sure if the logic around your GPU selection for clients is the best. On my system (with just one big GPU) only the server made use of it even though less than 5% of the VRAM was used. Wouldn't it be better to use Flower's simulation engine (i.e. use start_simulation as oppose to start_server+start_client?

Ping me again over here or Slack once you had a chance to look into the suggestions mentioned above and also those highlighted below. It would be good to start seeing some results on the README.md (even if still aren't fully reproducing those in the FedStar paper).

baselines/fedstar/README.md Outdated Show resolved Hide resolved
baselines/fedstar/README.md Outdated Show resolved Hide resolved
baselines/fedstar/README.md Show resolved Hide resolved
baselines/fedstar/README.md Outdated Show resolved Hide resolved
baselines/fedstar/README.md Outdated Show resolved Hide resolved
baselines/fedstar/fedstar/model.py Outdated Show resolved Hide resolved
@jafermarq jafermarq changed the title FedStar Recreate FedStar Oct 8, 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.

Hi @Raj-Parekh24 , just a couple of points based on our last sync on slack.

baselines/fedstar/fedstar/models.py Outdated Show resolved Hide resolved
gc.collect()
return (
self.model.get_weights(),
self.num_examples_train,
Copy link
Contributor

Choose a reason for hiding this comment

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

is it correct reporting only the number of labelled examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of putting on labeled samples because our semi-supervised architecture is run on the client side and only the unlabelled samples are available there. The server only knows about labeled samples.

@jafermarq jafermarq marked this pull request as ready for review February 19, 2024 21:21
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.

Amazing job @Raj-Parekh24 reproducing FedStar, a FL method for semi-supervised audio recognition! It's great including a new TensorFlow baseline too.

@jafermarq jafermarq enabled auto-merge (squash) February 19, 2024 21:32
@jafermarq jafermarq merged commit 4d96a39 into adap:main Feb 19, 2024
28 checks passed
@Raj-Parekh24
Copy link
Contributor Author

Thanks a lot @jafermarq,

For your guidance and support throughout the project!

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.

2 participants