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

Migrate tutorial-series-get-started-with-flower-pytorch to FDS #2480

Merged
merged 15 commits into from
Dec 21, 2023

Conversation

adam-narozniak
Copy link
Member

@adam-narozniak adam-narozniak commented Oct 6, 2023

Issue

The tutorial series uses the custom dataset partitioning which might be implied.

Proposal

Migrate the 1st tutorial to use the Flower Datasets.

Discussion

I've updated the example so the structure remains the same - there is a list of dataloaders. There might be an alternative approach to just create a FederatedDataset abstraction and use it in the client function to fetch a partition, split train test and transform to DataLoader.

@adam-narozniak adam-narozniak marked this pull request as ready for review October 11, 2023 12:29
jafermarq
jafermarq previously approved these changes Dec 20, 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.

Looks good to me. I just added some small changes.

@@ -640,9 +620,21 @@
"toc_visible": true
},
"kernelspec": {
"display_name": "flower-3.7.12",
"display_name": "flwr-rtwXnbAq-py3.10",
Copy link
Member

Choose a reason for hiding this comment

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

@adam-narozniak just to be sure, is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

that change probably comes from my side when re-running the notebook. i think this is not important. Can be discarded or kept.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tanertopal both "display_name" and "name" are required. I changed it to "flwr" now.
When they are not present I get "[E 09:57:23.746 NotebookApp] Notebook JSON is invalid: 'display_name' is a required property". And in UI, when starting the notebook I get the error (if not present). Similarly when "name" was not present in google colab, it complained and defaulted to "python3"

Comment on lines 626 to 637
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.10.6"
Copy link
Member

Choose a reason for hiding this comment

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

@adam-narozniak do we need this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we don't need it. Our current nbstripout removed it. That was added in Javier's run and I didn't notice it.

@danieljanes danieljanes enabled auto-merge (squash) December 21, 2023 20:49
@danieljanes danieljanes merged commit 34fd0b3 into main Dec 21, 2023
27 checks passed
@danieljanes danieljanes deleted the fds-migrate-tutorial-series-1 branch December 21, 2023 20:57
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.

4 participants