fix: Support placeholders for hyperparameters passed to TrainingStep

[ca-nguyen]

Issue #, if available: #152

Description of changes:
A breaking change was introduced in V2.2.0 where it was no longer possible to use Placeholders as input for hyperparameters in TrainingStep.

With this solution, TrainingStep hyperparameters will be compatible with Placeholders with the same behaviour as in V2.1.0: if a Placeholder is provided as input to hyperparameters in TrainingStep constructor, the estimator hyperparameters are overwritten

A feature request to be able to merge the Placeholder hyperparameters was made in #152, which will be addressed in another PR.

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

[wong-a]

Doesn't this do the same thing as before? The only difference seems to be merge_dicts does a deep copy, but that shouldn't be an issue. Seems like merging was already supported before. HyperParameters doesn't have nested fields.

I don't think this actually addresses #152, where the requester wanted to pass a Placeholder to hyperparameters arg, not a dict containing Placeholders as values.

[ca-nguyen]

Merging was only supported for non Placeholder values before. We would only merge if the constructor hyperparameters was a dict as well, otherwise, we would overwrite with the Estimator hyperparameters would through an error (Edited)

[ca-nguyen]

I had also considered going back to what was done before the merge fix - if the hyperparameter passed in the constructor was a Placeholder, it would overwrite the estimator hyperparameters. But this would not satisfy the need for merging constructor and estimator hyperparameters.

Any suggestions?

[wong-a]

Merging was only supported for non Placeholder values before.

How so? The previous code doesn't do any type checks.

[wong-a]

I had also considered going back to what was done before the merge fix - if the hyperparameter passed in the constructor was a Placeholder, it would overwrite the estimator hyperparameters. But this would not satisfy the need for merging constructor and estimator hyperparameters.

I think we need to dive a little deeper here to understand the use case and what works today.

There are 3 scenarios:

1. hyperparameters is None - we use the hyperparameters from the estimator (already handled in v2.2.0)
2. hyperparameters is a dict (which may have Placeholder values) - we merge constructor's hyperparameters with the estimator's hyper parameters (already handled in v2.2.0)
3. hyperparameters is a Placeholder, not a dict with Placeholder values (raises Error)

In the initial comment, the requester is trying to use the following, which is case 3:

hyperparameters=execution_input["TrainingParameters"],

In 2.2.0 and this version, it doesn't work, because ExecutionInput doesn't have an items() method because it isn't a dict. This raises an Error:

AttributeError: 'ExecutionInput' object has no attribute 'items'

They used the following to convert it to a dict, but that doesn't work either.

hyperparameters=execution_input["TrainingParameters"].get_schema_as_dict(),

The result of execution_input["TrainingParameters"].get_schema_as_dict() is the schema, not the placeholder values. e.g.

{
   'C': str,
   'D': str
}

I think @ohbuchim is actually trying to merge $$.Execution.Input.TrainingParameters with the estimator's hyperparameters. There are two things we can do there.

1. @shivlaks' proposal to just use the JSONPath for HypeterParameters (totally ignoring the estimator)

'HyperParameters.$': "$$.Execution.Input['TrainingParameters']"

2. Try to merge the two by deriving values from the PlaceHolder schema. This requires all keys to be modelled in the Placeholder schema.

Suppose the estimator has these hyperparameters:

{
   'A': 'a',
   'B': 'b'
}

And the ExecutionInput schema is:

{
  'TrainingInput': {
     'B': str,
     'C': str
   },
}

If the caller passes hyperparameters={ 'B': execution_input['TrainingInput']['B'], 'C': execution_input['TrainingInput']['C'] to TrainingStep, then we can already merge the two to become:

{
   'A': 'a',
   'B.$': '$$.Execution.Input.B',
   'C.$': '$$.Execution.Input.C' 
}

If the caller passes hyperparameters=execution_input['TrainingInput'], then we need to handle this differently. B and C are not specified, so we would have to get all properties of the Placeholder schema at execution_input['TrainingInput'] and insert those keys into Parameters.

This requires the user to map all the properties they want to use in the schema. In ASL it's impossible to merge objects in cases where TrainingInput might have some arbitrary properties at runtime (e.g. D and E) not modelled in the schema.

[ca-nguyen]

How so? The previous code doesn't do any type checks.

When a Placeholder was passed, an exception would be thrown due to this line since Placeholder does not have items() attribute (rectifying my comment: it would not replace it with the estimator hyperparameters, but would through an error)

[ca-nguyen]

(I just read the following comment - please ignore previous one)^

[wong-a]

With this change, scenario 3 will not throw an error, because of the type check in merge_dicts:

aws-step-functions-data-science-sdk-python/src/stepfunctions/steps/utils.py

Line 60 in 01e18c3

if isinstance(target, dict) and isinstance(source, dict):

But that means we will ignore the hyperparameters arg entirely.

[ca-nguyen]

I like the solution you are proposing - this will provide the expected behaviour described in #152
Will make the changes in the next commit

[ca-nguyen]

This will be resolved in 2 steps:

1. Fix the breaking change:

   • We need to support the same behaviour as in V2.1.0 -> if a Placeholder is provided as input to hyperparameters in TrainingStep constructor, the estimator hyperparameters are overwritten

2. _Feature request (described in TrainingStep's hyperparameters does not work on v2.2.0 #152 comments): To be able to merge a Placeholder hyperparameters passed to TrainingStep with the estimator hyperparameters.
   This will introduce a breaking change since we need to support the behaviour described in V2.1.0. Proposing to add a utility that translates a Placeholder to a usable jsonpath dict (with $). That translated placeholder_dict can then be used as hyperparameters input to TrainingStep. With that, merging the constructor and estimator hyperparameters will be possible.

   • ex: A Placeholder with schema

{
  'TrainingInput': {
     'B': str,
     'C': str
   }
}

will output the following:

{
   'B.$': '$$.Execution.Input.B',
   'C.$': '$$.Execution.Input.C' 
}

[shivlaks]

🎉 I think this issue is a fix and not a feat. had a couple other small comments.
please also make sure that @wong-a has a look before merging.

[wong-a]

Looks good, just some minor comments for clarity

[ca-nguyen]

🎉 I think this issue is a fix and not a feat. had a couple other small comments.

Done!

Looks good, just some minor comments for clarity

Comments addressed :)

[StepFunctions-Bot]

AWS CodeBuild CI Report

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

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