- Notifications
You must be signed in to change notification settings - Fork89
fix: supplying hyperparameters to training step constructor drops hyperparameters specified in estimator#144
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…erparameters specified in estimator
src/stepfunctions/steps/sagemaker.py Outdated
| parameters['HyperParameters']=hyperparameters | ||
| merged_hyperparameters= {} | ||
| ifestimator.hyperparameters()isnotNone: | ||
| merged_hyperparameters.update(estimator.hyperparameters()) |
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.
re:
should we warn/error instead of an in-place merge when we find duplicate keys?
Throwing an error wouldn't be helpful; it's more breaking (you can't create what you could before) and doesn't address the desired functionality in#99. Logging might at an INFO level may be helpful for duplicate keys.
We should also document if not already thatparameters gets totally overridden bytraining_config. This isn't the case for all service integration steps. Maybe we should adopt an update strategy there too?
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 was originally thinking that we should log it as a warning sinceINFO tends to generally also include a lot of junk but not strongly opinionated.
I'll re-spin theupdate calls to assemble the dicts so that they log something on duplicate keys.
absolutely agree that we need to document.
This isn't the case for all service integration steps. Maybe we should adopt an update strategy there too?
great call. wasn't on my radar, but I'm in favour of adopting the update strategy. The most consistent it is across things in the SDK, the more intuitive and idiomatic it will feel for users.
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'll make the changes to service integration steps in a separate PR. let me know if you had a different thought/idea of where we should be documenting this behaviour@wong-a
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 meant the other way around actually. In all service integration steps besides sagemaker, the constructor accepts aparameters argument that becomesParameters . We can update sagemaker step classes to accept aparameters dict which can be an escape hatch for full API coverage or override any explicitly exposed arguments.
For example, DynamoDBUpdateItem the caller must specify all parameters in theparameters field. The constructor doesn't have atable_name argument to set or other required fields:
https://github.com/aws/aws-step-functions-data-science-sdk-python/blob/main/src/stepfunctions/steps/service.py#L140-L167
Whereas the sagemaker steps always construct parameters using the sagemaker SDK and some special arguments in the constructor. You could provideparameters because the constructor accepts**kwargs, but it won't do anything.
https://github.com/aws/aws-step-functions-data-science-sdk-python/blob/main/src/stepfunctions/steps/sagemaker.py#L477-L479
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 see what you mean. I'm in favour of adding that parameters property and will address it in another PR. Escape hatches are powerful because it'll give users a path forward without requiring first class support to be developed and released.
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.
Yeah, that's out of scope of this PR. Can you create an issue for tracking?
Uh oh!
There was an error while loading.Please reload this page.
wong-a left a comment
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.
LGTM besides making documentation clearer
Uh oh!
There was an error while loading.Please reload this page.
wong-a left a comment
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.
< ship it > --------- \ ^__^ \ (oo)\_______ (__)\ )\/\ ||----w | || ||StepFunctions-Bot commentedJun 18, 2021
AWS CodeBuild CI Report
Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository |
bump for `v2.2.0` which includes the following changes:Features* Placeholders in TrainingStep to set S3 location for InputDataConfig and OutputDataConfig (#142)* EventBridge service integration (#147)Fixes* supplying hyperparameters to training step constructor drops hyperparameters specified in estimator (#144)
Uh oh!
There was an error while loading.Please reload this page.
Summary
Hyperparameterscan be specified in theestimatorobject andhyperparametersproperty.Both of which are taken in the constructor of the
TrainingStepclass.The current behaviour drops any hyperparameters that were specified in the estimator if the property
is set in the
TrainingStepconstructor. This is undesirable as the estimators often specify algorithm specific hyperparameters out of the box that we don't want to drop.This change merges the hyperparameters in the constructor as well as the estimator that is used in
TrainingStep.If there are duplicate keys, the hyperparameters specified in the constructor will be used.
Closes#99,#72
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.