SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction #189

EngHabu · 2020-09-30T20:12:42Z

TL;DR

Creates custom types for ParameterRangeOneOf and HyperparameterJobConfig.
Use generics to serialize/deserialize protos to make UX visualization easier
Move tunable hyperparameters out of HyperparameterJobConfig to make it easier to bind (and consistent with Custom Container Training Job)

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Example:

simple_xgboost_hpo_job_task = hpo_job_task.SdkSimpleHyperparameterTuningJobTask(
    training_job=builtin_algorithm_training_job_task2,
    max_number_of_training_jobs=10,
    max_parallel_training_jobs=5,
    cache_version="1",
    retries=2,
    cacheable=True,
    tunable_parameters=["num_round", "max_depth", "gamma"],
)

    a = simple_xgboost_hpo_job_task(
        train=train_dataset,
        validation=validation_dataset,
        static_hyperparameters=static_hyperparameters,
        hyperparameter_tuning_job_config=hyperparameter_tuning_job_config,
        num_round=IntegerParameterRange(min_value=2, max_value=8, scaling_type=HyperparameterScalingType.LINEAR),
        max_depth=IntegerParameterRange(min_value=5, max_value=7, scaling_type=HyperparameterScalingType.LINEAR),
        gamma=ContinuousParameterRange(min_value=0.0, max_value=0.3, scaling_type=HyperparameterScalingType.LINEAR),
    )

Tracking Issue

flyteorg/flyte#455

…tom-type

flytekit/models/sagemaker/parameter_ranges.py

EngHabu · 2020-10-13T13:50:13Z

Ping @wild-endeavor @bnsblue

flytekit/sdk/types.py

bnsblue · 2020-10-14T05:23:31Z

tests/flytekit/common/workflows/sagemaker.py

+        validation=validation_dataset,
+        static_hyperparameters=static_hyperparameters,
+        hyperparameter_tuning_job_config=hyperparameter_tuning_job_config,
+        num_round=ParameterRangeOneOf(


Why is it ParameterRangeOneOf rather than ParameterRange (which is defined in flytekit.common.tasks.sagemaker.types https://github.com/lyft/flytekit/pull/189/files#diff-0a303f3854a09b767e67d5f6eb857cbc49a6714f8df987f65646b98c5ef4d194R8)

What's the purpose of flytekit.common.tasks.sagemaker.types.ParameterRange?

That (in common.tasks.sagemaker.types) is just the "type" definition.. this is the value...
just like you use Types.String to define the type of an input but you then use "hello world" as the value...

bnsblue · 2020-10-14T05:28:39Z

tests/flytekit/common/workflows/sagemaker.py

+        validation=validation_dataset,
+        static_hyperparameters=static_hyperparameters,
+        hyperparameter_tuning_job_config=hyperparameter_tuning_job_config,
+        num_round=ParameterRangeOneOf(


Also, is it possible to make this even simpler by getting rid of the ParameterRangeOneOf() part? For example, Is it possible to make it as simple as num_round=IntegerParameterRange(...) ?

I would love to... any ideas? @wild-endeavor might have ideas too..

pinging @wild-endeavor ^ please take a look

bnsblue · 2020-10-14T05:34:08Z

tests/flytekit/common/workflows/sagemaker.py

+        validation=validation_dataset,
+        static_hyperparameters=static_hyperparameters,
+        hyperparameter_tuning_job_config=hyperparameter_tuning_job_config,
+        num_round=ParameterRangeOneOf(


Sorry I don't think I understand the code so these might just be stupid questions. But when should the user use ParameterRangeOneOf and when should one use ParameterRange?

ParameterRange is the type... you use it when you declare the interface of a task/WF... etc...
ParameterRangeOneOf is the value you can set for that type....

We can choose to name them the same but keep them in two different packages... (the flytekit way as far as I understand)...

@wild-endeavor might help shed some light on how to properly do this...

bnsblue · 2020-10-14T05:35:17Z

I left some questions and comments. I don't think I understand the code fully so those questions might be stupid.

bnsblue · 2020-10-15T06:58:38Z

I don't have further comments besides I really like the user to be able to write
num_round=IntegerParameterRange(...) instead of num_round=ParameterRangeOneOf(IntegerParameterRange(...))
#189 (comment)
Unfortunately I am not able to come up with a way to solve it, so I am going to approve the PR. Feel free to merge it if you don't think this is possible.

EngHabu · 2020-10-15T07:02:32Z

Let me try something

bnsblue · 2020-10-15T07:37:44Z

Sure. I was hoping that creating a ParameterRangeBase and letting ParameterRangeOneOf and IntegerParameterRange etc all inherit from it can solve the problem, but I couldn't get it to work.

bnsblue · 2020-10-15T16:41:50Z

@EngHabu Are you done with what you wanted to try? Is that part ready for review?

bnsblue · 2020-10-15T17:58:31Z

flytekit/common/tasks/sagemaker/types.py


-ParameterRange = _sdk_types.Types.GenericProto(_pb2_parameter_ranges.ParameterRangeOneOf)
+ParameterRange = _sdk_types.Types.GenericProto(_parameter_range_models.ParameterRangeOneOf)


If we are wrapping a model instead, should we still call it Generic"Proto"?

models should be thought of as "glorified" protos (or pythonic-protos)... so same assumptions about them should apply...

makes sense

bnsblue · 2020-10-15T19:56:40Z

just a nit. LGTM

…ig. TunableParams extraction (flyteorg#189) Creates custom types for ParameterRangeOneOf and HyperparameterJobConfig. Use generics to serialize/deserialize protos to make UX visualization easier Move tunable hyperparameters out of HyperparameterJobConfig to make it easier to bind (and consistent with Custom Container Training Job) * Typing * ParameterRangeOneOf model * cleanup * lint * lint * unittests * unit * unit * isort * isort * py 3.5 * lint * lint * re-add generic types to protos * lint * Remove generic * remove T * reformat * fix import * PR Comments * lint * lint * PR Comments * PR Comments * lint * unittest * PR Comments * lint * remove deprecated fields * Support converting raw protos through Types.*Proto classes * lint * revert notebook.py * lint

EngHabu added 23 commits September 30, 2020 13:12

Typing

a248037

Merge branch 'master' into sm-custom-type

3883b03

ParameterRangeOneOf model

2198781

cleanup

bfc9991

lint

b83d810

Merge branch 'master' into sm-custom-type

9ebf5c9

lint

243d0e7

unittests

d32de2a

unit

ca2c8fb

unit

9c9646d

isort

105f58a

isort

3cb7a0c

py 3.5

7851288

Merge branch 'master' into sm-custom-type

6b7d89a

lint

29c4aa7

Merge branch 'master' into sm-custom-type

8830680

lint

454ec9d

re-add generic types to protos

e55ef83

lint

a5c44a5

Remove generic

9638801

Merge branch 'master' into sm-custom-type

6b554cb

remove T

defee18

Merge branch 'sm-custom-type' of github.com:lyft/flytekit into sm-cus…

6876251

…tom-type

EngHabu changed the title ~~Various SageMaker types cleanup~~ SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction Oct 5, 2020

EngHabu marked this pull request as ready for review October 5, 2020 05:36

EngHabu requested review from kumare3, matthewphsmith and wild-endeavor as code owners October 5, 2020 05:36

EngHabu requested a review from bnsblue October 5, 2020 05:36

Merge branch 'master' into sm-custom-type

93a1e29

EngHabu requested a review from wild-endeavor October 8, 2020 14:59

Merge master

3b122c5

bnsblue reviewed Oct 9, 2020

View reviewed changes

flytekit/models/sagemaker/parameter_ranges.py Show resolved Hide resolved

bnsblue reviewed Oct 9, 2020

View reviewed changes

flytekit/models/sagemaker/parameter_ranges.py Outdated Show resolved Hide resolved

bnsblue reviewed Oct 9, 2020

View reviewed changes

flytekit/models/sagemaker/parameter_ranges.py Show resolved Hide resolved

PR Comments

c026081

EngHabu requested a review from bnsblue October 10, 2020 09:40

EngHabu added 2 commits October 10, 2020 11:56

lint

23aa95c

remove deprecated fields

c48d496

bnsblue reviewed Oct 14, 2020

View reviewed changes

flytekit/sdk/types.py Show resolved Hide resolved

bnsblue reviewed Oct 14, 2020

View reviewed changes

bnsblue previously approved these changes Oct 15, 2020

View reviewed changes

Support converting raw protos through Types.*Proto classes

5c54947

EngHabu dismissed bnsblue’s stale review via 5c54947 October 15, 2020 12:23

EngHabu added 3 commits October 15, 2020 14:24

lint

800251f

revert notebook.py

c6b2667

lint

4807d5e

bnsblue reviewed Oct 15, 2020

View reviewed changes

bnsblue approved these changes Oct 15, 2020

View reviewed changes

EngHabu merged commit 17ca24f into master Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction #189

SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction #189

EngHabu commented Sep 30, 2020 •

edited

Loading

EngHabu commented Oct 13, 2020

bnsblue Oct 14, 2020 •

edited

Loading

bnsblue Oct 14, 2020

EngHabu Oct 14, 2020

bnsblue Oct 14, 2020

bnsblue Oct 14, 2020

EngHabu Oct 14, 2020

bnsblue Oct 15, 2020

bnsblue Oct 14, 2020

EngHabu Oct 14, 2020

bnsblue commented Oct 14, 2020

bnsblue commented Oct 15, 2020 •

edited

Loading

EngHabu commented Oct 15, 2020

bnsblue commented Oct 15, 2020

bnsblue commented Oct 15, 2020 •

edited

Loading

bnsblue Oct 15, 2020

EngHabu Oct 16, 2020

bnsblue Oct 16, 2020

bnsblue commented Oct 15, 2020


		ParameterRange = _sdk_types.Types.GenericProto(_pb2_parameter_ranges.ParameterRangeOneOf)
		ParameterRange = _sdk_types.Types.GenericProto(_parameter_range_models.ParameterRangeOneOf)

SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction #189

SageMaker custom types for ParameterRangeOneOf and HyperparameterConfig. TunableParams extraction #189

Conversation

EngHabu commented Sep 30, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Tracking Issue

EngHabu commented Oct 13, 2020

bnsblue Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnsblue commented Oct 14, 2020

bnsblue commented Oct 15, 2020 • edited Loading

EngHabu commented Oct 15, 2020

bnsblue commented Oct 15, 2020

bnsblue commented Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnsblue commented Oct 15, 2020

EngHabu commented Sep 30, 2020 •

edited

Loading

bnsblue Oct 14, 2020 •

edited

Loading

bnsblue commented Oct 15, 2020 •

edited

Loading

bnsblue commented Oct 15, 2020 •

edited

Loading