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

[DRAFT] feature: Support placeholders for Sagemaker parameters #154

Closed
wants to merge 7 commits into from

Conversation

ca-nguyen
Copy link
Contributor

@ca-nguyen ca-nguyen commented Aug 10, 2021

Issue #, if available: #117, #139, #94

Description of changes:
Currently, it is not possible to use placeholders for Sagemaker properties.
This change makes it possible to use placeholders and define SageMaker properties dynamically, after the state machine has been created (path to those params are defined in src/stepfunctions/steps/constants.py.)

With this change, it will be possible to define Sagemaker properties by passing them directly to the Sagemaker step constructor as additional args. The args that can be defined will depend on the type of step that you create.

TODO

  • Add support VpcConfig for TrainingStep to pass to Estimator
  • Add support for CheckpointConfig for TrainingStep to pass to Estimator
  • Add support for InputDataConfig for TrainingStep to pass to Estimator
  • Add tests for TuningStep placeholders
  • Add test to catch InvalidPathToPlaceholderParameter exception
  • Update doc

Open questions

  • We parse through kwargs for all SageMaker step tasks and all valid SageMaker properties are treated to make placeholder compatible. This could be confusing as depending on the type of step, the properties can be propagated to a processor, estimator or tuner. Is passing the properties in the kwargs simple to understand/use or is there a need to separate SageMaker properties per step type?
  • Should we add documentation for each configurable SageMaker parameters that can be passed down to the processor, estimator and tuner in the functions Args section? Would a sentence mentioning those configurable parameters suffice?

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

@ca-nguyen ca-nguyen changed the title feature: Support placeholders for Sagemaker parameters [DRAFT] feature: Support placeholders for Sagemaker parameters Aug 10, 2021
@ca-nguyen ca-nguyen requested review from wong-a and shivlaks August 10, 2021 23:26
@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

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

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

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.

still reviewing, but i'll mention a high level thought
is it possible to lay out the approach itself as a part of the description. What I'd be looking for is a summary of:

  1. approach we have taken to support this capability
  2. alternatives and any pros/cons
  3. open questions, along with your initial instincts?

I'd also say it's far more pragmatic to scaffold these out for each of the steps in separate PRs rather than all in one. It will be simpler to review as well as author. The collection of PRs can collectively resolve the larger issue of supporting "all" placeholders.

wdyt?

@ca-nguyen
Copy link
Contributor Author

I agree, this will be clearer and easier to review if each step had each own PR.
I'll break it into smaller ones

@ca-nguyen ca-nguyen closed this Aug 12, 2021
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.

3 participants