Feature/add vpc config #17
Conversation
…expanded the list of supported ECR repos containing pre-built Docker images maintained by AWS
…requirements and added an example
jaipreet-s
left a comment
jaipreet-s
left a comment
There was a problem hiding this comment.
Looks good overall! A few comments that we should address before merging and releasing.
README.md
Outdated
|
|
||
| ```bash | ||
| sm-docker build . --repository mynewrepo:1.0 --role MyRoleName | ||
| sm-docker build . --repository mynewrepo:1.0 --role MyRoleName --bucket MyBucketName --vpc-id MyVpcId --subnets MySubnetId1,MySubnetId2 --security-groups MySecurityGroup1,MySecurityGroup2 |
There was a problem hiding this comment.
| sm-docker build . --repository mynewrepo:1.0 --role MyRoleName --bucket MyBucketName --vpc-id MyVpcId --subnets MySubnetId1,MySubnetId2 --security-groups MySecurityGroup1,MySecurityGroup2 | |
| # Basic usage | |
| sm-docker build . | |
| # Specify a custom ECR repository to push to and custom role | |
| sm-docker build . --repository mynewrepo:1.0 --role MyRoleName | |
| # Connect the CodeBuild job to a VPC | |
| sm-docker build . --repository mynewrepo:1.0 --role MyRoleName --bucket MyBucketName --vpc-id MyVpcId --subnets MySubnetId1,MySubnetId2 --security-groups MySecurityGroup1,MySecurityGroup2 |
There was a problem hiding this comment.
My recommendation is that we keep the basic example as-is, but add more "advanced" usage examples like using VPC.
There was a problem hiding this comment.
Addressed as part of the change recommended on the second comment below, where the suggestion was made to use the extended example with sample values in place of the earlier sample example
README.md
Outdated
| ```bash | ||
| sm-docker build . --repository mynewrepo:1.0 --role SampleDockerBuildRole --bucket sagemaker-us-east-1-326543455535 --vpc-id vpc-0c70e76ef1c603b94 --subnets subnet-0d984f080338960bb,subnet-0ac3e96808c8092f2 --security-groups sg-0d31b4042f2902cd0 |
There was a problem hiding this comment.
nit-pick: I believe you can just add this in the example command on line #28 itself.
| build_parser.add_argument( | ||
| "--subnets", | ||
| help="The comma-separated list of subnet ids for the CodeBuild Project (such as subnet-0b31f1863e9d31a67)", | ||
| ) | ||
| build_parser.add_argument( | ||
| "--security-groups", | ||
| help="The comma-separated list of security group ids for the CodeBuild Project (such as sg-0ce4ec0d0414d2ddc).", |
There was a problem hiding this comment.
In the VPC parameter, we're calling it vpc-id explicitly whereas in the subnets and security groups we're just calling them subnets/security-groups though we're expecting the user to pass IDs. I recommend we make the terminology here consistent
- --vpc, --subnets, --security-groups
OR - --vpc-id, --subnet-ids, --security-groups-ids
My preference is the latter as it makes it clear to the user that the IDs are expected and not any other attribute of the VPC/Subnet/SecurityGroups like name
There was a problem hiding this comment.
Standardized the input arguments using the latter of the two options
| - $(aws ecr get-login --no-include-email --region $AWS_DEFAULT_REGION --registry-ids 683313688378) | ||
| - $(aws ecr get-login --no-include-email --region $AWS_DEFAULT_REGION --registry-ids 520713654638) | ||
| - $(aws ecr get-login --no-include-email --region $AWS_DEFAULT_REGION --registry-ids 462105765813) |
There was a problem hiding this comment.
Please add a comment on what Deep Learning Container or Algorithm container these correspond to.
There was a problem hiding this comment.
Please see the details below:
- 683313688378 - Prebuilt Amazon SageMaker Docker Images for Scikit-learn and Spark ML in us-east-1
- 520713654638 & 462105765813 - Amazon SageMaker RL Containers
2 participants
Issue #, if available: 15
Description of changes: Added support for running the AWS CodeBuild Project within a VPC and expanded the list of supported ECR repos containing pre-built Docker images maintained by AWS. Updated the README with details regarding how to use VPC arguments and listed the changes required to the access policy on the execution role.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.