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

SageMaker types changes #123

Merged
merged 9 commits into from
Oct 16, 2020
Merged

SageMaker types changes #123

merged 9 commits into from
Oct 16, 2020

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Oct 4, 2020

TL;DR

  • Use structs to unmarshal hyperparameterconfig,
  • Read parameter ranges from inputs

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

Tracking Issue

flyteorg/flyte#455

@EngHabu EngHabu changed the title SageMaker type changes SageMaker types changes Oct 6, 2020
@EngHabu EngHabu marked this pull request as ready for review October 6, 2020 09:39
@EngHabu EngHabu requested review from bnsblue and kumare3 October 6, 2020 09:39
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #123 into master will decrease coverage by 0.07%.
The diff coverage is 51.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   61.20%   61.13%   -0.08%     
==========================================
  Files         105      105              
  Lines        5941     5956      +15     
==========================================
+ Hits         3636     3641       +5     
- Misses       1930     1938       +8     
- Partials      375      377       +2     
Impacted Files Coverage Δ
go/tasks/plugins/k8s/sagemaker/utils.go 68.00% <42.30%> (-1.73%) ⬇️
...o/tasks/plugins/k8s/sagemaker/plugin_test_utils.go 95.15% <84.61%> (-0.70%) ⬇️
...sks/plugins/k8s/sagemaker/hyperparameter_tuning.go 61.49% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6148f4...7148363. Read the comment docs.

@EngHabu
Copy link
Contributor Author

EngHabu commented Oct 14, 2020

ping @bnsblue @kumare3

@kumare3
Copy link
Contributor

kumare3 commented Oct 14, 2020

Will loook tomorrow morning

@bnsblue
Copy link
Contributor

bnsblue commented Oct 14, 2020

Trying to run end2end in a sandbox to go through some details. Will continue tomorrow morning.

@bnsblue
Copy link
Contributor

bnsblue commented Oct 14, 2020

@EngHabu I ran the test workflow you put inside your flytekit PR locally and was stepping through the code with it. I noticed that the input static_hyperparameters, which is NOT a ParameterRangeOneOf, can get past this UnmarshalStruct check (err == nil).
https://github.com/lyft/flyteplugins/pull/123/files#diff-0860ebd6bc768e12560fd1608c1053f3c78899a4c3028e82e17b70f59d86ffe5R148-R149

I could be wrong but I don't think this is as expected?

@bnsblue
Copy link
Contributor

bnsblue commented Oct 15, 2020

Otherwise, this PR LGTM.

@EngHabu
Copy link
Contributor Author

EngHabu commented Oct 15, 2020

Excellent point. What will happen in that scenario is that the we will skip this input. (won't be considered a tunable hyperparameter)... That's why I added this TODO: https://github.com/lyft/flyteplugins/pull/123/files#diff-0860ebd6bc768e12560fd1608c1053f3c78899a4c3028e82e17b70f59d86ffe5R188-R190
We can't reliably throw an error unless we can inspect the interface and recognize certain inputs as ParameterRanges... this is a bit tricky today because we use Generic/Proto to represent those inputs...

@EngHabu EngHabu merged commit 6c2fb8c into master Oct 16, 2020
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* SageMaker: Use structs for protos and move tunable params to inputs

* Update hyperparams parsing

* logs, fixes

* fix unit tests

* lint

* lint
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