fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep #160

ca-nguyen · 2021-09-09T16:21:31Z

Issue #, if available: #82

Description of changes:
1. fix: environment variables are overwritten and not passed through to SageMaker ModelStep

The environment variables are not passed from the model to ModelStep parameters (they are overwritten here) for models without instance type (models other than sagemake.model.FrameworkModel)
With this change, the Model env parameters are passed to the ModelStep.

New bug uncovered:
2. fix: env variables defined in the Estimator are not translated to Model when calling TrainingStep.get_expected_model()

Fixed by passing the env from the estimator to the expected Model
Added a new estimator (pca_estimator_with_env) with defined env parameters to use in test and confirm that the env variables are passed through to the Model
With this change, the env variables defined in the estimator are passed to the expected Model when calling TrainingStep.get_expected_model()

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

…thout instance type

shivlaks

thanks for putting this fix together. had one small comment around test coverage

tests/unit/test_sagemaker_steps.py

wong-a · 2021-09-09T18:51:39Z

src/stepfunctions/steps/sagemaker.py

            parameters = model_config(model=model, instance_type=instance_type, role=model.role, image_uri=model.image_uri)
            if model_name:
                parameters['ModelName'] = model_name
-        elif isinstance(model, Model):


Is this backwards compatible?

yes, this generated the same parameters as the old way of doing, but takes into account the env variables as well

Not quite. If model is the Model base class, parameters will have new fields that weren't there before. It's not just PrimaryContainer.Evnironment, but VpcConfig and other fields in the container definition.

https://github.com/aws/sagemaker-python-sdk/blob/e78d0ead32ebd163a21996be44c8cd9a5ee31379/src/sagemaker/workflow/airflow.py#L566-L605

I don't have enough context to know what effect that has, but instantiating a ModelStep with the same model will not produce the same parameters as before.

You are right - it shouldn't produce the same parameters if vpc_config or an image_config (see here) were provided in the Model as they would be included in the parameters as well.

It is a good point to consider why those params were initially omitted, but I think it makes sense to have consistency between parameters generated from FrameworkModel with the ones generated from Model

With a FrameworkModel, params would also include vpc_config, but not image_config (container_def() is called without image_config arg here)

it shouldn't produce the same parameters if vpc_config or an image_config (see here) were provided

can we add the tests that would have caught this. our coverage might be limited

It is a good point to consider why those params were initially omitted,

Do the commits where they were introduced provide that context? In any case, we need to preserve existing behaviour. Customers who upgrade without changing any of their code should be able to do that without unexpected mutations.

can we add the tests that would have caught this. our coverage might be limited

Yes, i will add test to ensure there is no regression and/or breaking changes

Do the commits where they were introduced provide that context? In any case, we need to preserve existing behaviour. Customers who upgrade without changing any of their code should be able to do that without unexpected mutations.

It was included in the initial commit, so no insight on why they were omitted

shivlaks · 2021-09-10T01:00:37Z

src/stepfunctions/steps/sagemaker.py

-                'ExecutionRoleArn': model.role,
-                'ModelName': model_name or model.name,
-                'PrimaryContainer': {
-                    'Environment': {},


the original change that this one superseded (#84) only modified this line. the rationale/need for this change isn't quite captured in the issue this is resolving. Although it's supported, we need to get into why we are making this change.

As it modifies your control flow, also suggest adding tests for "all model types"
edit: do we have tests that pass in a FrameworkModel and a Model?

I opted for this change since I saw it was supported in sagemaker sdk since the issue was opened, but I agree that we should not introduce breaking changes - even more if the issue does not capture the need to.

I'll revert the changes and go for the solution that was proposed in #84 and add test to validate that behaviour is the same

edit: do we have tests that pass in a FrameworkModel and a Model?

We have tests that use a FrameworkModel (ex: test_training_step_creation_with_framework() and others that use a Model (ex: test_training_step_creation_with_model()), but none that pass both.

but none that pass both.

I didn't mean in a single test (is that even possible). was just verifying that both parts of the control flow are being tested. I would assume that if they already existed, one of them would have broken with the attempt to introduce a breaking change.

if that didn't happen, i think it surfaces a gap in testing and we should use this opportunity to plug it in

I added a test in the latest commit that confirms we are consistent
The test will help catch future breaking changes

tests/unit/test_sagemaker_steps.py

ca-nguyen · 2021-09-10T22:39:48Z

tests/unit/test_sagemaker_steps.py

+@patch('botocore.client.BaseClient._make_api_call', new=mock_boto_api_call)
+@patch.object(boto3.session.Session, 'region_name', 'us-east-1')
+def test_training_step_creation_with_model_with_env(pca_estimator_with_env):
+    training_step = TrainingStep('Training', estimator=pca_estimator_with_env, job_name='TrainingJob')


This test is using a new model with a defined env, vpc_config and image_config
vpc_config and image_config are not passed to ModelStep - this confirms that it is consistent with the current behaviour and will allow us to catch a breaking change.

(not blocking: but please follow this guidance for future contributions)

You're following the existing conventions here, but let's try to structure the tests as documentation so they are easier to read in the future.

Name the test cases in a way that describes the functionality you are verifying. e.g. test_training_step_get_expected_model_returns_model_with_environment

Scope down the assertions to specific pieces if you're testing specific things. e.g. assert training_step.get_expected_model(...).env == # the environment. You can do have multiple and smaller asserts in a single test.

If something is already covered in other test cases, you don't to repeat it every time.

ACK
I'll take note to add these conventions in the CONTRIBUTING guide as well to ensure contributors have access to these

shivlaks · 2021-09-10T23:42:39Z

src/stepfunctions/steps/sagemaker.py

+        if self.estimator.environment:
+            model.env = self.estimator.environment


CR description should also capture why we're doing this. It's not strictly related to the bug reported, but is solving an issue. Typically better to fix things separately, but when there are multiple fixes, they need to be captured in the commit summary, along with rationale and any testing that was performed.

Updated the description with the uncovered bug and what was done to fix it
I am open to move this fix to a separate PR - will go with what reviewers prefer since the review process has already begun

What do you think? @shivlaks @wong-a

Given where we currently are, i'm not strongly opinionated because it feels more practical to go with it. It feels like it might just be busy work splitting them up in the current state.

Generally and going forward, I think we should keep changes contained and specific to the bug they address / feature they introduce for a few reasons:

reverts are simpler if there's a problem with one fix and not with another

the discovered bug may not have been triaged with a process of repro steps, expected behaviour, actual behaviour to converge on a candidate fix.

simpler for reviewers and contributors - simplifies life for everyone involved

helps to clearly see what was fixed for inclusion in changelog when we perform next release.

this PR grows the scope of the change that it supersedes

having said that, i would probably still split it up if I were the one doing it.

If it's already implemented and works, let's keep it in here, since it's a small enough change. Just update the PR description accordingly.

StepFunctions-Bot · 2021-09-11T02:35:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
Commit ID: dda1dfb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

ca-nguyen added 2 commits September 9, 2021 09:04

Use model_config to generate CreateModelStep parameters for models wi…

597fe1d

…thout instance type

Update estimator env check when updating model env

ffbcf53

ca-nguyen requested review from wong-a and shivlaks September 9, 2021 16:21

ca-nguyen mentioned this pull request Sep 9, 2021

Made it so that underlying Model object makes use of environmental va… #84

Closed

shivlaks suggested changes Sep 9, 2021

View reviewed changes

tests/unit/test_sagemaker_steps.py Show resolved Hide resolved

shivlaks changed the title ~~fix: Pass environment variables to ModelStep~~ fix: environment variables are overwritten and not passed through to SageMaker ModelStep Sep 9, 2021

Adding separate test for model with env

04cd813

ca-nguyen requested a review from shivlaks September 9, 2021 18:05

shivlaks previously approved these changes Sep 9, 2021

View reviewed changes

yuan-bwn previously approved these changes Sep 9, 2021

View reviewed changes

wong-a reviewed Sep 9, 2021

View reviewed changes

Merge branch 'main' into fix-model-step-env

ea3e482

ca-nguyen requested a review from wong-a September 9, 2021 19:09

shivlaks reviewed Sep 10, 2021

View reviewed changes

Update env param directly and add test

f8008e8

ca-nguyen dismissed stale reviews from yuan-bwn and shivlaks via f8008e8 September 10, 2021 11:01

shivlaks and others added 2 commits September 10, 2021 12:49

Merge branch 'main' into fix-model-step-env

6a4a826

Merge branch 'main' into fix-model-step-env

dda1dfb

ca-nguyen commented Sep 10, 2021

View reviewed changes

tests/unit/test_sagemaker_steps.py Show resolved Hide resolved

ca-nguyen commented Sep 10, 2021

View reviewed changes

ca-nguyen requested a review from shivlaks September 10, 2021 22:40

shivlaks reviewed Sep 10, 2021

View reviewed changes

ca-nguyen requested a review from shivlaks September 11, 2021 00:32

wong-a approved these changes Sep 11, 2021

View reviewed changes

shivlaks approved these changes Sep 11, 2021

View reviewed changes

shivlaks changed the title ~~fix: environment variables are overwritten and not passed through to SageMaker ModelStep~~ fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep Sep 11, 2021

shivlaks merged commit 79e930b into aws:main Sep 11, 2021

ca-nguyen mentioned this pull request Sep 11, 2021

Problem with environment variables in modelstep #82

Closed

wong-a mentioned this pull request Oct 6, 2021

fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers #169

Merged

6 tasks

ca-nguyen deleted the fix-model-step-env branch October 27, 2021 01:12

ca-nguyen mentioned this pull request Dec 4, 2021

chore: Bump version to v2.3.0 #183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep #160

fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep #160

ca-nguyen commented Sep 9, 2021 •

edited

Loading

shivlaks left a comment •

edited

Loading

wong-a Sep 9, 2021

ca-nguyen Sep 9, 2021

wong-a Sep 9, 2021

ca-nguyen Sep 9, 2021

ca-nguyen Sep 9, 2021

shivlaks Sep 10, 2021

ca-nguyen Sep 10, 2021

shivlaks Sep 10, 2021 •

edited

Loading

ca-nguyen Sep 10, 2021

ca-nguyen Sep 10, 2021

shivlaks Sep 10, 2021

ca-nguyen Sep 10, 2021

ca-nguyen Sep 10, 2021

wong-a Sep 11, 2021

ca-nguyen Sep 11, 2021

shivlaks Sep 10, 2021

ca-nguyen Sep 11, 2021

shivlaks Sep 11, 2021 •

edited

Loading

wong-a Sep 11, 2021

ca-nguyen Sep 11, 2021

StepFunctions-Bot commented Sep 11, 2021

		if self.estimator.environment:
		model.env = self.estimator.environment

fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep #160

fix: environment variables specified in Model or Estimator are not passed through to SageMaker ModelStep #160

Conversation

ca-nguyen commented Sep 9, 2021 • edited Loading

shivlaks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivlaks Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivlaks Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StepFunctions-Bot commented Sep 11, 2021

AWS CodeBuild CI Report

ca-nguyen commented Sep 9, 2021 •

edited

Loading

shivlaks left a comment •

edited

Loading

shivlaks Sep 10, 2021 •

edited

Loading

shivlaks Sep 11, 2021 •

edited

Loading