-
Notifications
You must be signed in to change notification settings - Fork 11
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
Provision cross-device FL cloud infrastructure #336
Conversation
@laurentgrangeau Can you please split this PR into smaller ones? Let's focus on the cloud infrastructure in this one. You can open a draft one for the workloads that we'll review once we merge this one. Once you do that, please fix any linting errors the Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @laurentgrangeau !
A few preliminary comments:
- Can you refactor this to be a module under
/terraform
that a user can conditionally enable with a variable? That would greatly simplify this PR because you don't need to redefine providers and some data sources. - Can you refactor this to use Cloud Foundation Toolkit modules?
Thanks!
Everything should be fixed now. Please let me know if everything is good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add Kubernetes manifests to a different PR
- Resolve any linter errors
- Create a module under the
/terraform
folder - Use CFT modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @laurentgrangeau! I completed a first review pass.
Can you please include a README for the module that describes what it does? We're also going to need to extend the main README with a section (or point to a dedicated document) that explains what we're adding.
Back to you :)
@laurentgrangeau can you also rebase this branch on top of the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small things to check and we should be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few points left to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two minor comments still to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor finishing touches to address.
Example of a cross-device federated learning workflow
TODO:
/terraform
folder