Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Adding proto definitions for supporting SageMaker TrainingJob (built-in algorithms) and HyperparameterTuningJob #66

Merged
merged 69 commits into from
Jul 30, 2020

Conversation

bnsblue
Copy link
Contributor

@bnsblue bnsblue commented May 29, 2020

TL;DR

This PR adds the essential proto messages for the support of SageMaker TrainingJob and HyperparameterTuningJob.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

This PR adds the essential proto messages for the support of SageMaker TrainingJob and HyperparameterTuning.
The major messages added include HyperparameterTuningJob, TrainingJob, and ParameterRanges.

The additions allow users to:

  1. configure, create, and invoke a TrainingJob
  2. configure, create, and invoke a HyperparameterTuningJob
  3. specify type-safe hyperparameter ranges for HyperparameterTuningJobs from Flytekit

Tracking Issue

flyteorg/flyte#255

Follow-up issue

flyteorg/flyte#431


package flyte.plugins.sagemaker;

option go_package = "github.com/kumare3/awsflyteplugins/gen/pb-go/proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I think you should follow the regular path here... of other plugins...

option go_package = "github.com/lyft/flyteidl/gen/pb-go/flyteidl/plugins";

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this has a lot of messages, can you actually add one extra level of nesting?
option go_package = "github.com/lyft/flyteidl/gen/pb-go/flyteidl/plugins/sagemaker";

If this works, can you also remove the SageMaker prefix of the messages below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure let me give it a try

HPOJobObjective Objective = 2;
int64 MaxNumberOfTrainingJobs = 3;
int64 MaxParallelTrainingJobs = 4;
ParameterRanges ParameterRanges = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really a "config"? not an input of the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was trying to follow the CRD definition of the amazon-sagemaker-operator-for-k8s. But I think what you suggested here makes more sense.

ParameterRanges ParameterRanges = 5;
}

message SagemakerHPOJob {
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume you meant for this to go into the custom field of the task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

int64 max_wait_time_in_seconds = 2;
}

message VpcConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave this out. This should be a backend configuration IMO


message TrainingJob {
string region = 1;
string role_arn = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be the same role that we are using for the workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. That means we do not need to define this as a part of the message?

@bnsblue bnsblue requested a review from EngHabu July 28, 2020 22:58
@kumare3
Copy link
Contributor

kumare3 commented Jul 29, 2020

Lets just mostly remove the TODO or word it better we are then good to go!

Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Awesome! I think just make sure all messages and fields have docs and it's good to go!

@bnsblue bnsblue requested a review from kumare3 July 29, 2020 17:23
kumare3
kumare3 previously approved these changes Jul 29, 2020
EngHabu
EngHabu previously approved these changes Jul 29, 2020
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

👍

wild-endeavor
wild-endeavor previously approved these changes Jul 29, 2020
EngHabu
EngHabu previously approved these changes Jul 29, 2020
@bnsblue bnsblue dismissed stale reviews from EngHabu and wild-endeavor via 8c5e60c July 30, 2020 16:23
@bnsblue bnsblue merged commit 73f4d1b into master Jul 30, 2020
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
…in algorithms) and HyperparameterTuningJob (#66)

* adding sagemaker protos

Co-authored-by: Haytham AbuelFutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants