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.

Sign up for GitHub

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

Jump to bottom

feat: add mlflow modules #5

Merged
merged 21 commits into from
Feb 20, 2024
Merged

feat: add mlflow modules #5

merged 21 commits into from
Feb 20, 2024

Conversation

kukushking
Copy link
Contributor

@kukushking kukushking commented Feb 14, 2024 •
edited
Loading

Description of changes:

  • Add mlflow-image module that builds mlflow container image and pushes to ECR.
  • Add mlflow-fargate module to run mlflow on AWS Fargate from the image above. By default, uses EFS for persistence.

Refer to the architecture and module README.MD for details.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kukushking
add mlflow image module
92c1a4c 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
formatting & readme fix
f337308 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
update Dockerfile & minor fixes
e0d7714 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add mlflow fargate module - checkpoint
6b4e2db 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
update manifests
2f25408 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
update readme & housekeeping
2930b1c 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking kukushking self-assigned this Feb 14, 2024
@kukushking
update docs
9a34e40 
Signed-off-by: Anton Kukushkin <[email protected]>
dgraeber
dgraeber reviewed Feb 14, 2024
View reviewed changes
Copy link

@dgraeber dgraeber left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things:

  • unit tests?
  • you need to add a change log to the repo and begin to track all PR's (see the other module repos)
  • looks like we need to add in the git workflows to executing the linting checks and the pytest

modules/mlflow/mlflow-fargate/stack.py Outdated
environment={
"BUCKET": f"s3://{artifacts_bucket_name}",
# TODO: Add persistence
# "HOST": database.db_instance_endpoint_address,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be configurable (i.e. add in RDS persistence if configured)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Also in case user wants to use the default file store for persistence we probably need an EBS volume -- checking this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added EFS by default

modules/mlflow/mlflow-fargate/stack.py Outdated Show resolved Hide resolved
@kukushking
use provided task subnets, scope down s3 permissions
f2f2269 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add basic unit tests
42199f7 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add cdk-nag & enable LB access logs
634d0cb 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add autoscale_max_capacity param
66c69d2 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add CHANGELOG.MD
8f3b9f7 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
bump cdk version
a2109fb 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
add EFS
16ae342 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
update docs
c3fceed 
Signed-off-by: Anton Kukushkin <[email protected]>
@LeonLuttenberger LeonLuttenberger mentioned this pull request Feb 16, 2024
Merged
@kukushking
fix cdk version
935a41a 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking kukushking marked this pull request as ready for review February 16, 2024 16:40
srinivasreddych
srinivasreddych reviewed Feb 20, 2024
View reviewed changes
modules/mlflow/mlflow-image/src/Dockerfile Outdated Show resolved Hide resolved
modules/mlflow/mlflow-fargate/stack.py Outdated Show resolved Hide resolved
modules/mlflow/mlflow-fargate/stack.py

Tags.of(scope=cast(IConstruct, self)).add(key="Deployment", value=app_prefix[:64])

role = iam.Role(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to name this role and prefix with the dep spec name to allow multiple instances of deployment of same module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am relying on the autogenerated name that will be unique across stacks/deployments. Does that work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test multiple instances of this module deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just did.

modules/mlflow/mlflow-fargate/stack.py
container.add_port_mappings(port_mapping)

# Add EFS
fs = efs.FileSystem(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to externalize EFS from this module and consume from IDF kind of a generic repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it s a good idea, but only after we have a second use case for it, to make sure we actually have a generalized version of it

modules/mlflow/mlflow-fargate/stack.py Outdated Show resolved Hide resolved
@kukushking
add lb access logs bucket an prefix parameters
aba7ca0 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
update docs
7e98fe7 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
lock boto3 version
df59d38 
Signed-off-by: Anton Kukushkin <[email protected]>
@kukushking
fix stack tests
c42d360 
Signed-off-by: Anton Kukushkin <[email protected]>
@dgraeber dgraeber self-requested a review February 20, 2024 18:58
dgraeber
dgraeber approved these changes Feb 20, 2024
View reviewed changes
modules/mlflow/mlflow-fargate/stack.py
vpc=vpc,
encrypted=True,
throughput_mode=efs.ThroughputMode.ELASTIC,
performance_mode=efs.PerformanceMode.GENERAL_PURPOSE,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the retain policy? will we have efs volumes out there when the module destroys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's RETAIN by default

@kukushking kukushking merged commit 20500a7 into awslabs:main Feb 20, 2024
5 checks passed
@kukushking kukushking deleted the feat/mlflow branch February 20, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgraeber dgraeber dgraeber approved these changes

@srinivasreddych srinivasreddych srinivasreddych left review comments

Assignees

@kukushking kukushking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kukushking @dgraeber @srinivasreddych