Add APIs to export Airflow model config #492
Conversation
src/sagemaker/workflow/airflow.py
Outdated
|
|
||
| def prepare_framework_container_def(model, instance_type, s3_operations): | ||
| """Prepare the framework model container information. Specify related s3 operations for Airflow to perform. | ||
| (Upload source_dir) |
There was a problem hiding this comment.
nitpicks: s/s3/S3 and backticks around source_dir. same below
There was a problem hiding this comment.
don't forget about making "source_dir" into ``source_dir``
Codecov Report
@@ Coverage Diff @@
## master #492 +/- ##
==========================================
+ Coverage 94.15% 94.15% +<.01%
==========================================
Files 59 59
Lines 4498 4552 +54
==========================================
+ Hits 4235 4286 +51
- Misses 263 266 +3
Continue to review full report at Codecov.
|
src/sagemaker/workflow/airflow.py
Outdated
|
|
||
| def prepare_framework_container_def(model, instance_type, s3_operations): | ||
| """Prepare the framework model container information. Specify related s3 operations for Airflow to perform. | ||
| (Upload source_dir) |
There was a problem hiding this comment.
don't forget about making "source_dir" into ``source_dir``
| try: | ||
| if model.model_server_workers: | ||
| deploy_env[sagemaker.model.MODEL_SERVER_WORKERS_PARAM_NAME.upper()] = str(model.model_server_workers) | ||
| except AttributeError: |
There was a problem hiding this comment.
is there no better way to check for this?
There was a problem hiding this comment.
Yep because model_server_workers is not a member of FrameworkModel class. So exception will be thrown if we try doing model.model_server_workers if model is a FrameworkModel but not a Deep Learning framework model. And here in the codes, we don't know whether it's just a FrameworkModel or Deep Learning framework model.
src/sagemaker/workflow/airflow.py
Outdated
|
|
||
| container_def = sagemaker.container_def(deploy_image, model.model_data, deploy_env) | ||
|
|
||
| return container_def |
There was a problem hiding this comment.
you can combine ll. 325-327
src/sagemaker/workflow/airflow.py
Outdated
| role (str): The ``ExecutionRoleArn`` IAM Role ARN for the model | ||
| image (str): An container image to use for deploying the model | ||
|
|
||
| Returns (dict): |
There was a problem hiding this comment.
this should be:
Returns:
dict: blah blah blah
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.