Address most recent pr feedback (#5)

kldavis4 · web-flow · commit faf055b3ab31 · 2021-05-26T19:09:51.000-04:00
* update readme based on feedback - generate metaflow config

* pr feedback: make custom batch ecr registry optional

* pr feedback: use full AWS brand names in docs

* pr feedback: remove datasets s3 bucket

* pr feedback: drop flow log on vpc

* pr feedback: comment clean-up

* pr feedback: configurable vpc and subnet cidr blocks

* pr feedback: reword comment

* pr feedback: output command for imporitng configuration

* pr feedback: use same min terraform version

* specify value in doc

* pr feedback: typos / grammar

* pr feedback: naming
diff --git a/aws/terraform/modules/metaflow/README.md b/aws/terraform/modules/metaflow/README.md
@@ -10,9 +10,9 @@ Provides the core functionality for Metaflow which includes:
 - an API to record and query past executions (`metadata-service`)
 - orchestrated processing (`step-functions`)
 
-Depends on a VPC that has been previously set up. The output of the project `infra` is an example 
-configuration of a VPC that can be passed to this module.
+Depends on an Amazon VPC that has been previously set up. The output of the project `infra` is an example
+configuration of an Amazon VPC that can be passed to this module.
 
-## ECR
+## Amazon ECR
 
-Sets up an AWS ECR to hold the Docker image we wish to use with Metaflow.
+Sets up an Amazon Elastic Container Registry (ECR) to hold the Docker image we wish to use with Metaflow.
diff --git a/aws/terraform/modules/metaflow/ecr.tf b/aws/terraform/modules/metaflow/ecr.tf
@@ -1,4 +1,6 @@
 resource "aws_ecr_repository" "metaflow_batch_image" {
+  count = var.enable_custom_batch_container_registry ? 1 : 0
+
   name = local.metaflow_batch_image_name
 
   tags = var.tags
diff --git a/aws/terraform/modules/metaflow/modules/README.md b/aws/terraform/modules/metaflow/modules/README.md
@@ -4,7 +4,7 @@ Our Metaflow Terraform code has been separated into separate modules based on th
 
 ## Computation
 
-Sets up remote computation resources so flows can be run on EC2 instances. These resources do not perform 
+Sets up remote computation resources so flows can be run on Amazon EC2 instances. These resources do not perform 
 orchestration and rely on the data scientist's computer to perform this coordination.
 
 ## Datastore
@@ -23,4 +23,4 @@ Datastore to explore historic runs.
 
 Sets up remote computation resources that come with orchestration. This allows data scientists to schedule flows 
 using crons as well as being able to kick off flows and shut down their machine, as the remote resources will handle 
-all coordination.
+all coordination.
diff --git a/aws/terraform/modules/metaflow/modules/computation/batch.tf b/aws/terraform/modules/metaflow/modules/computation/batch.tf
@@ -48,7 +48,7 @@ resource "aws_batch_compute_environment" "cpu" {
       var.subnet_private_2_id
     ]
 
-    # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
+    # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
     type = "EC2"
 
     tags = var.standard_tags
@@ -115,7 +115,7 @@ resource "aws_batch_compute_environment" "large-cpu" {
       var.subnet_private_2_id
     ]
 
-    # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
+    # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
     type = "EC2"
 
     tags = var.standard_tags
@@ -182,7 +182,7 @@ resource "aws_batch_compute_environment" "gpu" {
       var.subnet_private_2_id
     ]
 
-    # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
+    # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available
     type = "EC2"
 
     tags = var.standard_tags
diff --git a/aws/terraform/modules/metaflow/modules/computation/ec2.tf b/aws/terraform/modules/metaflow/modules/computation/ec2.tf
@@ -7,7 +7,7 @@ resource "aws_launch_template" "cpu" {
   */
   name = "${var.resource_prefix}batch-launch-tmpl-cpu-100gb${var.resource_suffix}"
 
-  # Defines what IAM Role to assume to grant an EC2 instance
+  # Defines what IAM Role to assume to grant an Amazon EC2 instance
   # This role must have a policy to access the kms_key_id used to encrypt the EBS volume
   iam_instance_profile {
     arn = aws_iam_instance_profile.ecs_instance_role.arn
@@ -37,7 +37,7 @@ resource "aws_launch_template" "gpu" {
   */
   name = "${var.resource_prefix}batch-launch-tmpl-gpu-100gb${var.resource_suffix}"
 
-  # Defines what IAM Role to assume to grant an EC2 instance
+  # Defines what IAM Role to assume to grant an Amazon EC2 instance
   # This role must have a policy to access the kms_key_id used to encrypt the EBS volume
   iam_instance_profile {
     arn = aws_iam_instance_profile.ecs_instance_role.arn
diff --git a/aws/terraform/modules/metaflow/modules/computation/variables.tf b/aws/terraform/modules/metaflow/modules/computation/variables.tf
@@ -49,61 +49,61 @@ variable "standard_tags" {
 
 variable "batch_compute_environment_cpu_max_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our CPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our CPU Batch Compute Environment can reach."
   default     = 32
 }
 
 variable "batch_compute_environment_cpu_min_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our CPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our CPU Batch Compute Environment should maintain."
   default     = 0
 }
 
 variable "batch_compute_environment_cpu_desired_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 0
 }
 
 variable "batch_compute_environment_large_cpu_max_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our large CPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our large CPU Batch Compute Environment can reach."
   default     = 128
 }
 
 variable "batch_compute_environment_large_cpu_min_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our large CPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our large CPU Batch Compute Environment should maintain."
   default     = 0
 }
 
 variable "batch_compute_environment_large_cpu_desired_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our large CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our large CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 0
 }
 
 variable "batch_compute_environment_gpu_max_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our GPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our GPU Batch Compute Environment can reach."
   default     = 32
 }
 
 variable "batch_compute_environment_gpu_min_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our GPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our GPU Batch Compute Environment should maintain."
   default     = 0
 }
 
 variable "batch_compute_environment_gpu_desired_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our GPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our GPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 0
 }
 
 variable "batch_cpu_instance_types" {
   type        = list(string)
-  description = "EC2 instance types allowed for CPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
+  description = "Amazon EC2 instance types allowed for CPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
   default = [
     "r5.large",
     "r5.xlarge",
@@ -117,7 +117,7 @@ variable "batch_cpu_instance_types" {
 
 variable "batch_large_cpu_instance_types" {
   type        = list(string)
-  description = "EC2 instance types allowed for larger CPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
+  description = "Amazon EC2 instance types allowed for larger CPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
   default = [
     "c4.8xlarge",
     "r5.4xlarge",
@@ -128,7 +128,7 @@ variable "batch_large_cpu_instance_types" {
 
 variable "batch_gpu_instance_types" {
   type        = list(string)
-  description = "EC2 instance types allowed for GPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
+  description = "Amazon EC2 instance types allowed for GPU Batch jobs. Types can be explicitly or implicitly requested by data scientists."
   default = [
     "p3.2xlarge"
   ]
diff --git a/aws/terraform/modules/metaflow/outputs.tf b/aws/terraform/modules/metaflow/outputs.tf
@@ -45,22 +45,26 @@ output "METAFLOW_SFN_DYNAMO_DB_TABLE" {
 
 output "metaflow_profile_json" {
   value = jsonencode(
-    {
-      "METAFLOW_BATCH_CONTAINER_REGISTRY"   = element(split("/", aws_ecr_repository.metaflow_batch_image.repository_url), 0),
-      "METAFLOW_BATCH_CONTAINER_IMAGE"      = element(split("/", aws_ecr_repository.metaflow_batch_image.repository_url), 1),
-      "METAFLOW_DATASTORE_SYSROOT_S3"       = module.metaflow-datastore.METAFLOW_DATASTORE_SYSROOT_S3,
-      "METAFLOW_DATATOOLS_S3ROOT"           = module.metaflow-datastore.METAFLOW_DATATOOLS_S3ROOT,
-      "METAFLOW_BATCH_JOB_QUEUE"            = module.metaflow-computation.METAFLOW_BATCH_JOB_QUEUE,
-      "METAFLOW_ECS_S3_ACCESS_IAM_ROLE"     = module.metaflow-computation.METAFLOW_ECS_S3_ACCESS_IAM_ROLE,
-      "METAFLOW_SERVICE_URL"                = module.metaflow-metadata-service.METAFLOW_SERVICE_URL,
-      "METAFLOW_SERVICE_INTERNAL_URL"       = module.metaflow-metadata-service.METAFLOW_SERVICE_INTERNAL_URL,
-      "METAFLOW_SFN_IAM_ROLE"               = module.metaflow-step-functions.metaflow_step_functions_role_arn,
-      "METAFLOW_SFN_STATE_MACHINE_PREFIX"   = replace("${local.resource_prefix}${local.resource_suffix}", "--", "-"),
-      "METAFLOW_EVENTS_SFN_ACCESS_IAM_ROLE" = module.metaflow-step-functions.metaflow_eventbridge_role_arn,
-      "METAFLOW_SFN_DYNAMO_DB_TABLE"        = module.metaflow-step-functions.metaflow_step_functions_dynamodb_table_name,
-      "METAFLOW_DEFAULT_DATASTORE"          = "s3",
-      "METAFLOW_DEFAULT_METADATA"           = "service"
-    }
+    merge(
+      var.enable_custom_batch_container_registry ? {
+        "METAFLOW_BATCH_CONTAINER_REGISTRY" = element(split("/", aws_ecr_repository.metaflow_batch_image[0].repository_url), 0),
+        "METAFLOW_BATCH_CONTAINER_IMAGE"    = element(split("/", aws_ecr_repository.metaflow_batch_image[0].repository_url), 1)
+      } : {},
+      {
+        "METAFLOW_DATASTORE_SYSROOT_S3"       = module.metaflow-datastore.METAFLOW_DATASTORE_SYSROOT_S3,
+        "METAFLOW_DATATOOLS_S3ROOT"           = module.metaflow-datastore.METAFLOW_DATATOOLS_S3ROOT,
+        "METAFLOW_BATCH_JOB_QUEUE"            = module.metaflow-computation.METAFLOW_BATCH_JOB_QUEUE,
+        "METAFLOW_ECS_S3_ACCESS_IAM_ROLE"     = module.metaflow-computation.METAFLOW_ECS_S3_ACCESS_IAM_ROLE,
+        "METAFLOW_SERVICE_URL"                = module.metaflow-metadata-service.METAFLOW_SERVICE_URL,
+        "METAFLOW_SERVICE_INTERNAL_URL"       = module.metaflow-metadata-service.METAFLOW_SERVICE_INTERNAL_URL,
+        "METAFLOW_SFN_IAM_ROLE"               = module.metaflow-step-functions.metaflow_step_functions_role_arn,
+        "METAFLOW_SFN_STATE_MACHINE_PREFIX"   = replace("${local.resource_prefix}${local.resource_suffix}", "--", "-"),
+        "METAFLOW_EVENTS_SFN_ACCESS_IAM_ROLE" = module.metaflow-step-functions.metaflow_eventbridge_role_arn,
+        "METAFLOW_SFN_DYNAMO_DB_TABLE"        = module.metaflow-step-functions.metaflow_step_functions_dynamodb_table_name,
+        "METAFLOW_DEFAULT_DATASTORE"          = "s3",
+        "METAFLOW_DEFAULT_METADATA"           = "service"
+      }
+    )
   )
   description = "Metaflow profile JSON object that can be used to communicate with this Metaflow Stack. Store this in `~/.metaflow/config_[stack-name]` and select with `$ export METAFLOW_PROFILE=[stack-name]`."
 }
@@ -91,7 +95,7 @@ output "metaflow_api_gateway_rest_api_id" {
 }
 
 output "metaflow_batch_container_image" {
-  value       = aws_ecr_repository.metaflow_batch_image.repository_url
+  value       = var.enable_custom_batch_container_registry ? aws_ecr_repository.metaflow_batch_image[0].repository_url : ""
   description = "The ECR repo containing the metaflow batch image"
 }
 
diff --git a/aws/terraform/modules/metaflow/variables.tf b/aws/terraform/modules/metaflow/variables.tf
@@ -13,57 +13,63 @@ variable "enable_step_functions" {
   description = "Provisions infrastructure for step functions if enabled"
 }
 
+variable "enable_custom_batch_container_registry" {
+  type        = bool
+  default     = false
+  description = "Provisions infrastructure for custom Amazon ECR container registry if enabled"
+}
+
 variable "cpu_max_compute_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our CPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our CPU Batch Compute Environment can reach."
   default     = 64
 }
 
 variable "cpu_min_compute_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our CPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our CPU Batch Compute Environment should maintain."
   default     = 16
 }
 
 variable "cpu_desired_compute_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 16
 }
 
 variable "large_cpu_max_compute_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our large CPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our large CPU Batch Compute Environment can reach."
   default     = 128
 }
 
 variable "large_cpu_min_compute_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our large CPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our large CPU Batch Compute Environment should maintain."
   default     = 0
 }
 
 variable "large_cpu_desired_compute_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our large CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our large CPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 0
 }
 
 variable "gpu_max_compute_vcpus" {
   type        = string
-  description = "Maximum number of EC2 vCPUs that our GPU Batch Compute Environment can reach."
+  description = "Maximum number of Amazon EC2 vCPUs that our GPU Batch Compute Environment can reach."
   default     = 64
 }
 
 variable "gpu_min_compute_vcpus" {
   type        = string
-  description = "Minimum number of EC2 vCPUs that our GPU Batch Compute Environment should maintain."
+  description = "Minimum number of Amazon EC2 vCPUs that our GPU Batch Compute Environment should maintain."
   default     = 0
 }
 
 variable "gpu_desired_compute_vcpus" {
   type        = string
-  description = "Desired number of EC2 vCPUS in our GPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
+  description = "Desired number of Amazon EC2 vCPUS in our GPU Batch Compute Environment. A non-zero number will ensure instances are always on and avoid some cold-start problems."
   default     = 0
 }
 

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ resource "aws_batch_compute_environment" "cpu" {`
`48`	`48`	`var.subnet_private_2_id`
`49`	`49`	`]`
`50`	`50`
`51`		`- # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
	`51`	`+ # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
`52`	`52`	`type = "EC2"`
`53`	`53`
`54`	`54`	`tags = var.standard_tags`
`@@ -115,7 +115,7 @@ resource "aws_batch_compute_environment" "large-cpu" {`
`115`	`115`	`var.subnet_private_2_id`
`116`	`116`	`]`
`117`	`117`
`118`		`- # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
	`118`	`+ # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
`119`	`119`	`type = "EC2"`
`120`	`120`
`121`	`121`	`tags = var.standard_tags`
`@@ -182,7 +182,7 @@ resource "aws_batch_compute_environment" "gpu" {`
`182`	`182`	`var.subnet_private_2_id`
`183`	`183`	`]`
`184`	`184`
`185`		`- # Type of instance EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
	`185`	`+ # Type of instance Amazon EC2 for on-demand. Can use "SPOT" to use unused instances at discount if available`
`186`	`186`	`type = "EC2"`
`187`	`187`
`188`	`188`	`tags = var.standard_tags`