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

Provision cross-device FL cloud infrastructure #336

Merged
merged 48 commits into from
Dec 5, 2023
Merged

Conversation

laurentgrangeau
Copy link
Contributor

@laurentgrangeau laurentgrangeau commented Nov 14, 2023

Example of a cross-device federated learning workflow
TODO:

  • Add Kubernetes manifests to a different PR
  • Resolve any linter errors
  • Create a module under the /terraform folder
  • Use CFT modules

@ferrarimarco
Copy link
Member

@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 lint job reports.

Thanks!

Copy link
Member

@ferrarimarco ferrarimarco left a 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!

@laurentgrangeau laurentgrangeau marked this pull request as draft November 14, 2023 16:26
@laurentgrangeau laurentgrangeau marked this pull request as ready for review November 14, 2023 21:24
@laurentgrangeau
Copy link
Contributor Author

Everything should be fixed now. Please let me know if everything is good

Copy link
Contributor Author

@laurentgrangeau laurentgrangeau left a 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

Copy link
Member

@ferrarimarco ferrarimarco left a 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 :)

terraform/cross-device/pubsub.tf Show resolved Hide resolved
terraform/cross-device/spanner.tf Outdated Show resolved Hide resolved
terraform/cross-device/spanner.tf Outdated Show resolved Hide resolved
terraform/cross-device/storage.tf Outdated Show resolved Hide resolved
terraform/cross-device/storage.tf Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
terraform/variables.tf Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
@ferrarimarco
Copy link
Member

@laurentgrangeau can you also rebase this branch on top of the latest main? There is a fix for container image builds. Thanks!

terraform/main.tf Outdated Show resolved Hide resolved
terraform/cross-device/workload_identity.tf Outdated Show resolved Hide resolved
Copy link
Member

@ferrarimarco ferrarimarco left a 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.

terraform/terraform.tfvars Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
Copy link
Member

@ferrarimarco ferrarimarco left a 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.

terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
Copy link
Member

@ferrarimarco ferrarimarco left a 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.

Copy link
Member

@ferrarimarco ferrarimarco left a 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.

terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
terraform/cross-device/README.md Outdated Show resolved Hide resolved
@laurentgrangeau laurentgrangeau added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit a7a86a4 Dec 5, 2023
14 checks passed
@laurentgrangeau laurentgrangeau deleted the cross-device-fl branch December 5, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants