-
Notifications
You must be signed in to change notification settings - Fork 442
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
Katib UI for v1alpha2 #486
Conversation
@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: MattSich, alex. Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@andreyvelich Is this PR blocked on something? |
@jlewi Backend for the UI is done. |
Create a new build with frontend
Frontend and Backend is done. |
@andreyvelich Good Job! I will talk a look in my day. Thanks! |
pkg/ui/v1alpha2/backend.go
Outdated
conn, c := k.connectManager() | ||
defer conn.Close() | ||
|
||
defer conn.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
kuh := ui.NewKatibUIHandler() | ||
|
||
frontend := http.FileServer(http.Dir("/app/build/")) | ||
http.Handle("/katib/", http.StripPrefix("/katib/", frontend)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is only one deployment for UI, in a single container, both http server and logic to call Katib-manager to operate experiment/trial/metrics.. and converting the results are running, right?
I had thought there would be two deployments (frontend and backend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be one deployment with frontend and backend.
All JS for frontend will be store in build
folder here: https://github.com/andreyvelich/katib/tree/v1alpha2-new-ui/pkg/ui/v1alpha2/frontend/build/static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the container works too heavy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be fine, no reason to divide frontend and backend sides.
All frontend builds in a 3 JS files by React.
pkg/ui/v1alpha2/backend.go
Outdated
} | ||
|
||
// TODO: Add delete job to Katib Client | ||
func (k *KatibUIHandler) DeleteJob(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete experiment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it
|
||
jobs := make([]JobView, 0) | ||
|
||
el, err := k.katibClient.GetExperimentList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I perfer we add filter
in GetExperimentList
db interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean filter
for HP or NAS jobs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, filter can help us filter we need in db server instead of our code implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we use Katib client, not db interface function. We can add filter in GetExperimentList
in Katib Client, but is it make sense?
} | ||
|
||
func (k *KatibUIHandler) SubmitHPJob(w http.ResponseWriter, r *http.Request) { | ||
//enableCors(&w) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems there is no difference between SubmitNASJob and SubmitHPJob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete one
I suggest removing WIP and try to merge it ASAP. Or we will meet with many conflicts since we are in active development. |
@gaocegege I can fix Dockerfile structure and we can merge it, I think. |
Should we wait for #527 ? |
@gaocegege Yes, after that I can add builders for the UI. |
SGTM |
I have added tests for the UI. I removed WIP status, after I pass the tests, I think, we can merge this PR. |
Please, check it. |
/lgtm @andreyvelich Lets merge this. We will have fixes in targeted PRs |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardsliu 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 |
This PR about new UI for v1alpha2 version of Katib built with React.
The UI supports HP and NAS jobs.
You can find all functionality of this UI in the Youtube link:
https://www.youtube.com/watch?v=WAK37UW7spo
Fixes: #373, #443.
Backend is done, but need to be tested and fixed small issues.
Frontend needs to be changed according to new Backend. I marked in Backend what needs to be done. I have started to work on Frontend.
As well, I refactored folder structure to support both UI for v1alpha1 and v1alpha2 version.
Please, start to review it.
/assign @richardsliu @hougangliu @johnugeorge @YujiOshima
/cc @gaocegege @MattSich @jlewi
Developed by: @Akado2009 @andreyvelich
This change is