MLOps modules

[jacksandom]

• @aaronsteers: Fix issue with unknown count of resources, driven by dynamically-calculated s3_triggers on python-lambda module. (P1)
• Auto-build and auto-upload whl file for glue transformation dependency. (P2)
• Use aws credential helper for ECR login.

[aaronsteers]

I just did a quick prelim review. More to follow in later discussions but two quick items:

1. I've called out a few places where you can use name_prefix to avoid potential naming collisions
2. I understand the csv files are valuable in git as a training dataset, but would be good to replace the zip (binary) files with their code equivalent if and when possible. As a general best practice, it's a good idea to avoid binary files since their diffs can't be readily evaluated by humans.

If these zips are for lambda functions, #2 will get a lot easier after refactoring onto the existing lambda-python module - since the zipping and packaging become automated during the terraform apply.

• Resolved

[jacksandom]

I just did a quick prelim review. More to follow in later discussions but two quick items:

1. I've called out a few places where you can use name_prefix to avoid potential naming collisions
2. I understand the csv files are valuable in git as a training dataset, but would be good to replace the zip (binary) files with their code equivalent if and when possible. As a general best practice, it's a good idea to avoid binary files since their diffs can't be readily evaluated by humans.

If these zips are for lambda functions, #2 will get a lot easier after refactoring onto the existing lambda-python module - since the zipping and packaging become automated during the terraform apply.

Hi @aaronsteers ,

I've added in the name prefix's. Yes, the ZIPs are the Lambda functions (not CSVs) so yes I hope that will become easier with the lambda module!

One question, are there any plans to add "force destroy" to the S3 mod? Sagemaker training jobs will store their models in S3 buckets which makes clean-up harder if we want to destroy said bucket.

Jack

[aaronsteers]

One question, are there any plans to add "force destroy" to the S3 mod? Sagemaker training jobs will store their models in S3 buckets which makes clean-up harder if we want to destroy said bucket.

I would like to avoid using force_destroy - but I have been thinking about this a bunch. The challenge is that we have to be very careful not to inadvertantly delete the data lake due to bucket rename or some other Terraform code change. I just added a feature to cataog/aws/data-lake master which is the option to provider a data_bucket_override input variable. This would be useful in cases where you already have a data lake bucket and want to add the surrounding features around an existing bucket. I believe you should be able to use this BYOB (bring-your-own-bucket) feature to pass in a bucket that does have the force_destroy property set. This should open up test/development patterns while not creating a dangerous default for actual production usage.

What do you think? Do you think this would work for what you are looking for?

[jacksandom]

One question, are there any plans to add "force destroy" to the S3 mod? Sagemaker training jobs will store their models in S3 buckets which makes clean-up harder if we want to destroy said bucket.

I would like to avoid using force_destroy - but I have been thinking about this a bunch. The challenge is that we have to be very careful not to inadvertantly delete the data lake due to bucket rename or some other Terraform code change. I just added a feature to cataog/aws/data-lake master which is the option to provider a data_bucket_override input variable. This would be useful in cases where you already have a data lake bucket and want to add the surrounding features around an existing bucket. I believe you should be able to use this BYOB (bring-your-own-bucket) feature to pass in a bucket that does have the force_destroy property set. This should open up test/development patterns while not creating a dangerous default for actual production usage.

What do you think? Do you think this would work for what you are looking for?

@aaronsteers Completely get the rationale and thinking about it, it probably makes sense for me not to use force_destroy on my buckets also.. I tested the feature and it works fine. However, it would only work with an existing bucket right? Is there a way to create a dependency if you wanted to create that bucket in the same Terraform apply?

[aaronsteers]

@jacksandom - When I merged in master, it started including documentation metadata in the CI/CD tests. Here's the output.

Basically, this just means we need to add a new comment block at the top of the main.tf file. You can copy-paste the below and then customize the description as needed.

/*
* This is my short description about the module and how to use it. 
* _Markdown_ formatting *is* supported.
*
*/

Do you mind taking a stab at this?

If you are interested, you can also test the auto-documentation by running the following:

# for Windows:
choco install terraform-docs  
# for Mac:
brew install terraform-docs 

cd docs
python build.py

The above will update the two *_index.md files in the docs folder, and will also update the README.md files in each component and catalog module.

[aaronsteers]

@jacksandom - I went ahead and added the comment header in catalog/aws/ml-ops-on-aws and I've updated the docs using the new auto-docs feature. You can checkout the new README.md files and the two %_index.md files in the docs folder.

[aaronsteers]

@jacksandom - I am almost done with the full review, and I wanted to send this over since I'm already late sending you this feedback. I may have a few other smaller comments but I think this covers the bulk of it. Thanks!

[aaronsteers]

Round 2 of feedback. I think this is everything. Thanks!

[aaronsteers]

@jacksandom - I have good news! It believe I've resolved the resource count issue in this commit: a65ac67

The problem-area was a distinct list of bucket names which was then being used to drive the count of resource permission objects created for iam policies. Instead, there is now only one iam/policy object of each type. While the objects the policy refers to are still driven by the distinct list of buckets, that count no longer affects the number of resources terraform is creating.

For readability, I also migrated a policy JSON string into the aws_iam_policy_document data resource - which just improves readability and reduces the need for escaping that is related to creating dynamic strings. More info on that here if you are interested: https://www.terraform.io/docs/providers/aws/d/iam_policy_document.html

[aaronsteers]

@jacksandom - I sent you a direct message in regards to the ECR login error. I would like to use the AWS Credential Helper if possible - here: https://github.com/awslabs/amazon-ecr-credential-helper

I remember I got this working before but I don't see where/if I had logged any documentation on that here in this repo. The desired behavior would be that we could use the credential helper instead of having to run aws ecr get-login.

[jacksandom]

@aaronsteers - thanks for reviewing - all my changes are in. Two main things left:

• Zip up Glue function
• AWS credential helper for ECR login

[aaronsteers]

Regarding Glue jobs:

• This might make a better glue PR after the fact.

Complexities:

• zip method puts the custom code in the same zip as the packaged dependencies
• whl method keeps dependencies separate from the custom source code
• when it runs, I think the platform/architecture need to match the aws (linux) environment
  • unclear if it will work to build this on windows and then upload to run under aws (linux)