Uh oh!
There was an error while loading. Please reload this page.
Add APIs to export transform and deploy config - #497
Conversation
Codecov Report
@@ Coverage Diff @@## master #497 +/- ##
==========================================
- Coverage 94.28% 94.28% -0.01%
==========================================
Files 59 59 Lines 4551 4603 +52 ==========================================
+ Hits 4291 4340 +49 - Misses 260 263 +3
Continue to review full report at Codecov.
|
| model (sagemaker.model.FrameworkModel): The framework model | ||
| instance_type (str): The EC2 instance type to deploy this Model to. For example, 'ml.p2.xlarge'. | ||
| s3_operations (dict): The dict to specify S3 operations (upload `source_dir`). | ||
| If None, server will use one worker per vCPU. Only effective when estimator is | ||
| SageMaker framework. | ||
| If None, server will use one worker per vCPU. Only effective when estimator is | ||
| SageMaker framework. |
| vpc_config_override=vpc_config_override) | ||
| else: | ||
| raise TypeError('Estimator must be one of BYO estimator, framework estimator or amazon algorithm' | ||
| 'estimator.') |
There was a problem hiding this comment.
it might be more helpful here to put the paths to the classes themselves, i.e. sagemaker.estimator.Estimator, etc.
| if transformer.output_path is None: | ||
| transformer.output_path = 's3://{}/{}'.format( | ||
| transformer.sagemaker_session.default_bucket(), transformer._current_job_name) |
There was a problem hiding this comment.
is this logic that's also in Transformer.transform? if so, I wonder if it'd be worth refactoring it into a private method that you can call on transformer here (like EstimatorBase._prepare_for_training)
There was a problem hiding this comment.
Yep it's similar except the naming method is different. One is airflow_name_from_base and the other is name_from_base. So if I wrap them in a method, that method needs to take a method as input arg which I don't want to do for just these small amounts of codes for now. If we apply the different methods to get name first, then there's just one line left which I guess no need to wrap. I am still targeting at what I mentioned in other comments that we could reformat the codes in session/estimator/transformer/etc in a way that we could wrap a lot codes in methods and couple airflow with them.
| return model_config(instance_type, model, role, image) | ||
| def transform_config(transformer, data, data_type='S3Prefix', content_type=None, compression_type=None, |
There was a problem hiding this comment.
are these kinds of methods potentially reusable for Session? (this is sort of out of scope of this PR)
There was a problem hiding this comment.
Yep I do want it to be reused in session. Now I cannot because I need to bypass all the validations/describe calls/etc to use Jinja templating.
If in session, we can construct the config first, and then apply all manipulations afterwards. That would be awesome and these two parts can be coupled. (which is really good since for now I am worried if session part got updated, for example, new entries in boto config introduced, airflow part cannot catch it)
iquintero
left a comment
There was a problem hiding this comment.
Just some minor comments.
| transform_env = model.env.copy() | ||
| if env is not None: | ||
| transform_env.update(env) | ||
| if self.latest_training_job is not None: |
There was a problem hiding this comment.
you can just do
if self.latest_training_job:
There was a problem hiding this comment.
Yep that looks nice. But the problem is we won't get intoif var
if var is things like [], or {}. Not just None. Hence I kind of want to explicitly say the only thing I don't want is None. Especially sometimes I do have some var = {} and need to do var.update() after. If there's an if in between, I do need to deal with the {} case.
Yep we probably should doif var
most of times andif var is not None
only when needed. But using the second one for now is kind of consistent to what we have in the existed classes (like estimator, model, etc). I prefer not to change for now. If later when we introduce style guide and can force it everywhere, we probably could do the change for all.
| logging.warning('No finished training job found associated with this estimator. Please make sure' | ||
| 'this estimator is only used for building workflow config') | ||
| model_name = self._current_job_name | ||
| transform_env = env if env is not None else {} |
There was a problem hiding this comment.
similar here,
transform_env = env or {}
| vpc_config = model.vpc_config | ||
| self.sagemaker_session.create_model(model_name, role, container_def, vpc_config) | ||
| transform_env = model.env.copy() | ||
| if env is not None: |
| transformer._current_job_name = job_name | ||
| else: | ||
| base_name = transformer.base_transform_job_name | ||
| transformer._current_job_name = utils.airflow_name_from_base(base_name) \ |
There was a problem hiding this comment.
transformer._current_job_name=utils.airflow_name_from_base(base_name) ifbase_nameelsetransformer.model_name| 'TransformResources': job_config['resource_config'], | ||
| } | ||
| if transformer.strategy is not None: |
There was a problem hiding this comment.
similar to my other comments, do all these as
iftransformer.strategy:| production_variant = sagemaker.production_variant(model.name, instance_type, initial_instance_count) | ||
| name = model.name | ||
| config_options = {'EndpointConfigName': name, 'ProductionVariants': [production_variant]} | ||
| if tags is not None: |
| # if there is s3 operations needed for model, move it to root level of config | ||
| s3_operations = model_base_config.pop('S3Operations', None) | ||
| if s3_operations is not None: |
| def test_transformer_config(sagemaker_session): | ||
| tf_transformer = transformer.Transformer( | ||
| model_name="tensorflow-model", | ||
| instance_count="{{ instance_count }}", |
There was a problem hiding this comment.
Im not sure I understand what this syntax is doing?
"{{ instance_count }}"There was a problem hiding this comment.
This is Jinja templates. It will be evaluated during Airflow runtime. For example, I can put something in database (using xcom in ariflow) and do "{{ task_instance.xcom_pull(task_id='task', key='key') }}" to get the record in the table 'task' with key 'key'. Here in unit tests I just make some random Jinja templating strings for testing purpose.
| assert config == expected_config | ||
| def test_transform_config_from_amazon_alg_estimator(sagemaker_session): |
There was a problem hiding this comment.
can you include in the test name that this also tests the code paths where you don't supply the optional args?
There was a problem hiding this comment.
Talked offline. No need to change.
| if env is not None: | ||
| transform_env.update(env) | ||
| else: | ||
| logging.warning('No finished training job found associated with this estimator. Please make sure' |
There was a problem hiding this comment.
are there unit tests for this change?
There was a problem hiding this comment.
Yep the transformer from estimator tests will cover this.
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.