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

Updating the FLamby installation instructions to reflect some changes on their end. #275

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

emersodb
Copy link
Collaborator

PR Type

Other

Short Description

Clickup Ticket(s): Link

This PR drops the FLamby requirements file in favor of moving back to installing the library from the FLamby repository. I tested running several of our FLamby experiments for (FedHeartDisease, Fed-IXI, and FedISIC2019). There was a small bug fix that needed to be applied to the single node training code, but other than that the experiments ran as expected.

Tests Added

No specific tests. However, I tested the fixes etc. by running a sampling of the FLamby experimental code.

…Looks like FLamby upgraded their library this summer. So the install process appears to be smoother again.
… change that needs to be done for FLamby to run correctly after installation.
```
__NOTE__: We avoid installing Fed-KITS2019, as it requires a fairly old version on nnUnet, which we no longer support in our library.

In addition, you'll have to edit the code for `FedIXITiny` in `flamby/datasets/fed_ixi/datasets.py` replacing the following
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The work around below is required in order to run any of the experiments, since our dataset processing files handle all of the dataset preprocessing for the datasets of interest. It's pretty straightforward to do since we install the library as "editable" (i.e. the -e) option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FLamby corrected a few things with MonAI compatibility but apparently left this one behind.

@@ -95,7 +95,7 @@ def validate(self, val_metric_mngr: MetricManager) -> None:
for input, target in self.val_loader:
input, target = input.to(self.device), target.to(self.device)

preds = self.model(input)
preds = {"predictions": self.model(input)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Small fix that must have been missed when we migrated to dictionaries for predictions at some point.

Copy link
Collaborator

@jewelltaylor jewelltaylor left a comment

Choose a reason for hiding this comment

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

LGTM!

Base automatically changed from dbe/add_feddgga_compatibility to main November 11, 2024 14:59
@emersodb emersodb merged commit c2c3035 into main Nov 11, 2024
6 checks passed
@emersodb emersodb deleted the dbe/updating_flamby_install branch November 11, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants