Skip to content
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

feat: Support placeholders for TuningStep #173

Merged
merged 6 commits into from
Nov 3, 2021

Conversation

ca-nguyen
Copy link
Contributor

Description

Add support to define Hyperparameters Tuning parameters dynamically using placeholders

Fixes #85

Why is the change necessary?

Currently, it is not possible to use placeholders for Sagemaker Hyperparameters Tuning properties. The properties cannot be defined dynamically, as they need to be defined in the HyperparameterTuner which does not accept placeholders.
This change makes it possible to use placeholders for HyperparameterTuner properties by using the parameters field that are passed down from the TuningStep.

Solution

Use the parameters field that is compatible with placeholders to define TuningStep properties.
Merge the parameters that were generated from the HyperparameterTuner with the input parameters:

  • The input parameters will overwrite the parameters generated from the HyperparameterTuner if the properties were defined in both
  • All TuningStep properties will be placeholder compatible except for data - which requires RecordSets as data depending on the estimator used to define the tuner.
  • The input parameters should follow the schema described in CreateHyperParameterTuningJob API doc

Testing

Added unit and integration tests


Pull Request Checklist

Please check all boxes (including N/A items)

Testing

  • Unit tests added
  • Integration test added
  • Manual testing - why was it necessary? could it be automated? - N/A

Documentation

  • docs: All relevant docs updated
  • docstrings: All public APIs documented

Title and description

  • Change type: Title is prefixed with change type: and follows conventional commits
  • References: Indicate issues fixed via: Fixes #xxx

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@ca-nguyen ca-nguyen marked this pull request as draft October 19, 2021 14:58
@ca-nguyen ca-nguyen force-pushed the support-placeholders-for-tuning-step branch from 8569fa0 to 5991cf8 Compare October 19, 2021 15:01
@ca-nguyen ca-nguyen force-pushed the support-placeholders-for-tuning-step branch from 5991cf8 to 39640f8 Compare October 19, 2021 15:05
@ca-nguyen ca-nguyen marked this pull request as ready for review October 19, 2021 17:01
@ca-nguyen ca-nguyen requested review from evscott and jormello October 20, 2021 17:01
evscott
evscott previously approved these changes Oct 20, 2021
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor questions

tests/integ/test_sagemaker_steps.py Outdated Show resolved Hide resolved
tests/unit/test_sagemaker_steps.py Outdated Show resolved Hide resolved
@ca-nguyen ca-nguyen requested review from shivlaks and evscott October 27, 2021 16:42
@ca-nguyen ca-nguyen dismissed shivlaks’s stale review October 27, 2021 16:43

Comments were addressed and PR was updated per suggestions in the last commit

Co-authored-by: Shiv Lakshminarayan <shivlaks@amazon.com>
@ca-nguyen ca-nguyen requested a review from shivlaks October 27, 2021 18:59
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! 🎉

@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

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

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

Copy link

@evscott evscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🚀

@ca-nguyen ca-nguyen merged commit 1ea8346 into aws:main Nov 3, 2021
@ca-nguyen ca-nguyen deleted the support-placeholders-for-tuning-step branch November 3, 2021 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Estimator Output Path cannot accept Execution Input placeholder
5 participants