-
Notifications
You must be signed in to change notification settings - Fork 8
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
DockerImageBuild state machine implementation #65
Conversation
|
||
|
||
def handle_set_model_to_creating(event: Dict[str, Any], context: Any) -> Dict[str, Any]: | ||
"""Set DDB entry to CREATING status.""" | ||
output_dict = deepcopy(event) | ||
output_dict["create_infra"] = True | ||
output_dict["create_infra"] = 'modelConfig' in event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean now- for these, we can use the names as they're defined in the CreateModelRequest, which we can pass as direct input into the state machine. I would suggest a few things here:
- Let's define any aws clients at the top of the file, similar to how they're done here:
LISA/lambda/models/state_machine/delete_model.py
Lines 28 to 30 in 283dd8f
cloudformation = boto3.client("cloudformation", region_name=os.environ["AWS_REGION"], config=retry_config) dynamodb = boto3.resource("dynamodb", region_name=os.environ["AWS_REGION"], config=retry_config) ddb_table = dynamodb.Table(os.environ["MODEL_TABLE_NAME"]) - For this create_infra variable, let's make it be
True
if all of AutoScalingConfig, ContainerConfig, InferenceContainer, InstanceType, and LoadBalancerConfig are notNone
. For False, all of these need to be None, and then let's throw an exception if they're partially configured (some null, some not). Basically we have two use cases:- We define the ECS infrastructure which is all of those fields, and ModelUrl must also be none
- We define a LiteLLM-only entry, which requires all of the others to be null, and we'd set the ModelUrl as specified in the request. For the initial revision here, let's just worry about the first case.
- In this step specifically, let's make sure that we set the model state to
Creating
in DDB, and we can add other details, like last modified date, the entire request blob, and anything else that seems like it'll be useful to have around. We can also add all that to the output_dict so that future states can have that info without calling out to ddb
return { | ||
"instance_id": instances[0].instance_id, | ||
"image_tag": image_tag | ||
} | ||
except ClientError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which errors we're expecting, but I think we can just let them boil out to the state machine. From the state machine definition after it's deployed, it automatically handles some form of sdk errors, but I'm not 100% sure if those are strictly lambda or if they're any sdk call:
"Retry": [
{
"ErrorEquals": [
"Lambda.ClientExecutionTimeoutException",
"Lambda.ServiceException",
"Lambda.AWSLambdaException",
"Lambda.SdkClientException"
],
"IntervalSeconds": 2,
"MaxAttempts": 6,
"BackoffRate": 2
}
],
return output_dict | ||
|
||
|
||
def handle_poll_docker_image_available(event: Dict[str, Any], context: Any) -> Dict[str, Any]: | ||
"""Check that Docker image is available in account or not.""" | ||
output_dict = deepcopy(event) | ||
|
||
ecrClient = boto3.client("ecr") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note for clients, let's define them at the top of the file with the region and retry config
4acff3c
to
092c531
Compare
.pre-commit-config.yaml
Outdated
@@ -80,7 +80,7 @@ repos: | |||
- --docstring-convention=numpy | |||
- --max-line-length=120 | |||
- --extend-immutable-calls=Query,fastapi.Depends,fastapi.params.Depends | |||
- --ignore=B008 # Ignore error for function calls in argument defaults | |||
- --ignore=B008,W503 # Ignore error for function calls in argument defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably keep W503 to keep with the pep standards https://www.flake8rules.com/rules/W503.html
where did this happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happened in create model state machine implementation. One of our formatters wants to put each condition on its own line, and flake8 doesn't. Couldn't make them both happy at the same time.
lambda/models/state-machine/create-model.py:38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh for that we could try putting the and
's at the end of the line. i don't think there's anything wrong with needing the backslash at the end to complete the line either. otherwise we can just do an inline ignore so we ignore the one line, but not the rest. so adding a comment like # noqa: W503
on the specific line could work
12567c3
to
a15b7d1
Compare
a15b7d1
to
5e86546
Compare
ec479b6
to
f0ba830
Compare
f0ba830
to
bdbac48
Compare
bdbac48
to
8cbfa9b
Compare
Description of changes:
Implement docker image build piece of state machine