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

First iteration of the new Katib UI #1427

Merged
merged 34 commits into from
Mar 3, 2021

Conversation

kimwnasptd
Copy link
Member

@kimwnasptd kimwnasptd commented Feb 5, 2021

What this PR does / why we need it

Adds a first iteration of a UI written in Angular. This UI will be utilizing common code between other Kubeflow web apps, like Notebooks and Tensorboard web apps, which will make it easier to maintain and fix bugs.

The UI also tries to provide an improvement over the UX of the existing one. The goal we want to constantly strive for is to be able to use this UI to fully manage Experiment, Trials etc without resorting to kubectl at all.

The link for the doc proposal is https://docs.google.com/document/d/14cfGYJiTl32GWBUTbYQHzXC4UWA7r_ysgWYSWsxcDvU/edit?usp=sharing

Which issue(s) this PR fixes

It is a first step for #1421

Let's keep the initial issue open so we can keep on linking new issues to it. This UI is not yet on par with the existing one. I will open follow up issues to help us track the remaining effort.

Special notes for your reviewer

I've created two new folders /pkg/new-ui and /cmd/new-ui and moved my code there, to ensure we don't break any existing functionality. I've also modified the /pkg/new-ui/v1beta1/README.md to include updated instructions on how to build this UI locally.

You can also try building a container image for the new UI by using the command

docker build -t ${IMG} -f cmd/new-ui/v1beta1/Dockerfile

You can then simply use this image in an existing Kubeflow cluster instead of the current one. NOTE that this UI cannot yet be deployed as a standalone web app, so you will need to test it in a Kubeflow cluster for now. It will by my immediate next goal to make the web app work in a standalone mode. We also try to document all the design decisions for these modes in kubeflow/kubeflow#5566

Lastly the unittests are broken in this PR. Once we have this PR merged I'll immediately create a follow up one that will fix them and also extend the current CI to run them automatically on subsequent PRs. We are doing this process for Notebooks WG kubeflow/kubeflow#5577 so I can handle and document this procedure.

I know this is a huge PR but please let me know if I can help in any way to review it
/cc @andreyvelich @gaocegege @johnugeorge
/assign @kimwnasptd

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kimwnasptd
To complete the pull request process, please assign johnugeorge after the PR has been reviewed.
You can assign the PR to them by writing /assign @johnugeorge in a comment when ready.

The full list of commands accepted by this bot can be found 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

@kimwnasptd
Copy link
Member Author

I'm not really sure why the presubmit e2e tests fails. Looking at the logs I can see the following relevant lines, but I'm not sure if this was broken from this PR

INFO|2021-02-05T18:01:53|/src/kubeflow/testing/py/kubeflow/testing/util.py|73| level=info msg="Applying workflows kubeflow-test-infra.kubeflow-katib-presubmit-e2e-v1beta1-1427-7a2c96f-1376-6ff8"
INFO|2021-02-05T18:01:53|/src/kubeflow/testing/py/kubeflow/testing/util.py|73| level=info msg="Creating non-existent workflows kubeflow-test-infra.kubeflow-katib-presubmit-e2e-v1beta1-1427-7a2c96f-1376-6ff8"
INFO|2021-02-05T18:01:54|/src/kubeflow/testing/py/kubeflow/testing/run_e2e_workflow.py|405| URL for workflow: https://argo.kubeflow-testing.com//workflows/kubeflow-test-infra/kubeflow-katib-presubmit-e2e-v1beta1-1427-7a2c96f-1376-6ff8?tab=workflow

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for this great contribution @kimwnasptd!

I will create a Kanban board for the new Katib UI soon and we can assign issue there.
In the meantime, I left few comments and I am still reviewing other files.

cmd/new-ui/v1beta1/Dockerfile Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/frontend/.editorconfig Outdated Show resolved Hide resolved
cmd/new-ui/v1beta1/.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! 🎉 👍

cmd/new-ui/v1beta1/main.go Outdated Show resolved Hide resolved
@andreyvelich
Copy link
Member

@kimwnasptd You also have to rebase your PR so e2e tests should be succeeded.

@google-cla
Copy link

google-cla bot commented Feb 16, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@kimwnasptd
Copy link
Member Author

@andreyvelich I rebased on top of latest master and force pushed, but now the cla-bot complains about needing consent from you as well. You are co-author in the commits that we added via GitHub UI during the review. Could you give a consent to the bot to not complain anymore?

Also the rebase made the tests pass, so hopefully we won't have too many notifications each time a new commit is added

@andreyvelich
Copy link
Member

@googlebot I consent.

cmd/new-ui/v1beta1/Dockerfile Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Outdated Show resolved Hide resolved
pkg/new-ui/v1beta1/README.md Show resolved Hide resolved
We will create a `new-ui` folder under the `pkg` dir to add the new UI.
This will ensure that we won't break any existing functionality.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
This PR introduces the new UI. We hope that this will be the last big PR
in this repo and all of the subsequent ones will be smaller bit-sized
PRs.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
We want the table in the main UI page to show more information for each
experiment. This information lives in the status of each Experiment CR,
so we expand the API to also return the entire status for each
Experiment.

In the future we will probably need to just send the entire CR to the
frontend and not parse it at all in the backend.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
We want to return the Pipeline UID for a Trial, if such exists.

When combined with Kale, a Trial initiates a KFP run. In this case,
there is an annotation with the KFP run ID, which we can use to navigate
the user to the KFP UI for the specific run.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
To serve an SPA the backend must return the index.html for any non-API
route. The index.html must be sent for any request to the app's page.
Then, once the javascript loads, the app will show to the user the
correct view.

In this commit we also completely remove any caching of the index.html,
for the browser to always request the latest version. This eliminates
the need to hard reload the page to view changes to the frontend code.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
kimwnasptd and others added 2 commits February 24, 2021 18:51
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
With Kale a Trial can launch a distinct KF Pipeline. The UID of this
pipeline will be also set as an annotation to the Trial owning the
Pipeline.

In this case the UI should have one extra column for this UID.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@andreyvelich
Copy link
Member

@kimwnasptd I think you resolved all of the comments which is great!
I have only once small nit: #1427 (comment).
I think we are ready to merge this PR and improve other features/problems in the following PRs, what do you think ?

@kimwnasptd
Copy link
Member Author

I think we are ready to merge this PR and improve other features/problems in the following PRs, what do you think ?

Let me just resolve the comment you mentioned as well as #1427 (comment) for being able to omit algorithm parameters and yes, we should be good to go!

kimwnasptd and others added 3 commits March 1, 2021 19:04
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@kimwnasptd
Copy link
Member Author

@andreyvelich I've pushed two commits that will most probably resolve the last open conversation we have, for this PR.

So I believe that we can merge this PR and as you mention coordinate on the next steps. We have opened issues for most of the pressing things we need to fix so we can document our decisions and next steps there. For reference I think the following three have the biggest priority:

  1. Add missing fields from the form to make it fully usable
  2. Add page for managing/editing the Trial ConfigMaps
  3. Work on the standalone/kubeflow modes of the app

Lastly I'd like to thank you for your thorough review. While the process took some time, I'm really happy with the end result as I believe we got on the same state wrt the app's state, next steps and improvements we can do from here.

I'm really looking forward to what's next!

@andreyvelich
Copy link
Member

Thank you @kimwnasptd for all of this great work and collaboration on this integration!
I am happy to work on this!

Sounds good from my side on this PR.
/lgtm

@gaocegege @johnugeorge Please give your approval on this PR and we can move forward.

@johnugeorge
Copy link
Member

/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, kimwnasptd

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

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.

7 participants