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

Refine API #74

Merged
merged 15 commits into from
May 15, 2018
Merged

Refine API #74

merged 15 commits into from
May 15, 2018

Conversation

YujiOshima
Copy link
Contributor

Related #72 #66 #68

  • Add low-level API and refine APIs
  • Split Trial to Worker(an instance of evaluating processes) and Trial (a parameter set).
  • Migrate dkl worker to kubernetes worker.
  • Save suggestion and Early stopping configs to DB
  • Add MinikubeDemo

HyperBand and Bayes optimization suggestion are not supported in this PR.
Random and Grid Support for now.

@gaocegege
@ddysher

Signed-off-by: YujiOshima [email protected]

@gaocegege
Copy link
Member

🎉 Thanks for your contribution

It is a little monolithic, I will reivew it ASAP

@YujiOshima YujiOshima mentioned this pull request Apr 30, 2018
@gaocegege
Copy link
Member

/assign @gaocegege @ddysher

@gaocegege
Copy link
Member

Generally LGTM, I will have a try on it.

BTW, @ddysher had a talk on KubeCon and I think he may have no time these days.

@YujiOshima
Copy link
Contributor Author

YujiOshima commented May 3, 2018

@gaocegege Thank you!
OK, we can wait for his review,
And I'm going to address @libbyandhelen 's comments.

@gaocegege
Copy link
Member

Hi @YujiOshima

ddysher may be busy, I think. And we could merge it and go ahead if you think it blocks your work.

@YujiOshima
Copy link
Contributor Author

@gaocegege Thanks. Yes, This block some work.
I will push a new commit applied @libbyandhelen 's comments soon.
Then please review.

@gaocegege
Copy link
Member

👌

@YujiOshima
Copy link
Contributor Author

I update

  • You can get worker_ids from study ID, trial ID or worker ID.
  • You can get suggestion/earlystopping parameter list from study ID.
  • Trials are created in suggestion service.

@gaocegege PTAL

@gaocegege
Copy link
Member

Cool I will take a look.

@YujiOshima
Copy link
Contributor Author

/retest

1 similar comment
@gaocegege
Copy link
Member

/retest

@YujiOshima
Copy link
Contributor Author

Is the test working? 🤔

@YujiOshima
Copy link
Contributor Author

@gaocegege @jlewi I'm not sure why the test goes time-out. Do you have any ideas?

@ddysher
Copy link
Member

ddysher commented May 13, 2018

Thanks! Generally LGTM 👍

@gaocegege
Copy link
Member

/retest

1 similar comment
@gaocegege
Copy link
Member

/retest

Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
YujiOshima added 10 commits May 15, 2018 22:47
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
Signed-off-by: YujiOshima <[email protected]>
@YujiOshima YujiOshima mentioned this pull request May 15, 2018
@gaocegege
Copy link
Member

/lgtm

@gaocegege
Copy link
Member

Is it ready to merge?

@YujiOshima
Copy link
Contributor Author

Yes, we can merge this!

@gaocegege
Copy link
Member

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 0a95175 into kubeflow:master May 15, 2018
@gaocegege gaocegege mentioned this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants