Add missing form fields to new UI #1463

kimwnasptd · 2021-03-10T15:59:43Z

What this PR does / why we need it:
It adds support to the new UI's #1421 form for

resumePolicy
Early Stopping
Trial Parameters

With this PR the form will be exposing all of the fields required to create a new Experiment. But, at the same time the only limitation I bumped into is the fact that the new UI doesn't currently allow us to edit the source of the Metric Collector. I'll comment this on our issue where we track the missing features.

Videos of the changes

algorithm-settings.mp4

early-stopping.mp4

trial-parameters.mp4

Which issue(s) this PR fixes
It's a first step for #1443. As mentioned above it won't close the issue because the new UI's form still doesn't allow us to configure the source of the Metrics Collector.

Special notes for your reviewer:

Since the PR is a bit big I'll also try to document my changes here, as well as in the commits.

Early Stopping
- a new module and component for this setting
- needed to create a common component from the form's algorithm component for showing the algorithm settings
- use the common component to show the Early Stopping algorithm's settings
Resume Policy
- just add one extra control to the Trial Thresholds
Trial Parameters
- create a component for each Trial Parameter
- extend the component that shows the Trial Template to parse the yaml contents, when they change, and create the corresponding Trial Parameters
Small fixes
- previously the algorithm's settings would only load once the algorithm would change. This resulted in not showing the random_state setting for the Random HP algorithm [ which the value selected once the form loads ]
- there were some places was still using the string value None for the random_state. I should use an empty value instead

/cc @andreyvelich @gaocegege @johnugeorge

andreyvelich

Thanks for this update, it's looks great @kimwnasptd.
I left few comments.

...new-ui/v1beta1/frontend/src/app/pages/experiment-creation/experiment-creation.component.html

pkg/new-ui/v1beta1/frontend/src/app/services/experiment-form.service.ts

kimwnasptd · 2021-03-12T12:14:31Z

Ι see the Charmed Katib / Test failing and this relevant, I believe, log

Error from server (BadRequest): container "katib-ui" in pod "katib-ui-7cff86f74f-9bvpp" is waiting to start: PodInitializing

I'll try to rebase on top of the latest master to see if this resolves the issue

google-cla · 2021-03-12T12:15:28Z

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 · 2021-03-12T12:55:11Z

Hmm the error seems to persist. Why does the katib-ui test get triggered when we don't modify any "relevant" code? Is this expected?

Lastly, @andreyvelich could you also write the I consent command to google bot so that it doesn't complain about the CLA? The bot seems to error once I rebase and force push

andreyvelich · 2021-03-12T14:26:03Z

@googlebot I consent.

andreyvelich · 2021-03-12T14:27:07Z

@kimwnasptd GitHub actions should be fixed in this PR: #1465.

kimwnasptd · 2021-03-12T14:50:53Z

@kimwnasptd GitHub actions should be fixed in this PR: #1465.

ACK!

I also pushed another small commit, because there was a small error with building the frontend. I've explained the reason behind it in the commit message as well, but the verdict is that Angular could not detect that

formGroup.get('trialParameters').controls

Is a FormArray and has a property controls. This happens when we use this in the html file.

In any case I believe this is another indication that the next thing to work on is the tests and having a CI process that at least checks that the UI can be build.

The form will be showing a dynamic list of trial parameters that the users will need to configure. This list is affected from the yaml content. The JS parses the yaml contents to find the trial parameters. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Create a common component for the algorithm settings. We will need this for Early Stopping, which also has algorithm settings. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add a distinct section for Early Stopping, when the Search Algorithm is for Hyper Parameter tuning. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* The algorithm settings got applied in the form only after the user selected a different algorithm. The preselected value would not have the list of settings assigned once the form loads. * Use null everywhere for the `random_state` parameter Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Using "formGroup.get('trialParameters').controls" in the html results in an error, during the build process. This is because Angular can't deduce that the control returned from the get() method is a form array. We will define a FormArray variable and use it directly instead of get()ing it from he form group. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

andreyvelich · 2021-03-12T21:17:57Z

@kimwnasptd As I can see the CI passed.
Are we ready to merge it ?

kimwnasptd · 2021-03-12T22:07:03Z

Yeap, we should be good to go!

andreyvelich

/lgtm
/approve

google-oss-robot · 2021-03-12T22:20:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, 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:

~~OWNERS~~ [andreyvelich]

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

google-oss-robot requested review from andreyvelich, gaocegege and johnugeorge March 10, 2021 15:59

google-oss-robot added the size/XL label Mar 10, 2021

kimwnasptd changed the title ~~Feature kimwnasptd missing form fields~~ Add missing form fields to new UI Mar 11, 2021

andreyvelich reviewed Mar 11, 2021

View reviewed changes

...new-ui/v1beta1/frontend/src/app/pages/experiment-creation/experiment-creation.component.html Outdated Show resolved Hide resolved

pkg/new-ui/v1beta1/frontend/src/app/services/experiment-form.service.ts Outdated Show resolved Hide resolved

kimwnasptd force-pushed the feature-kimwnasptd-missing-form-fields branch from 03b0efc to 43ff140 Compare March 12, 2021 12:15

kimwnasptd and others added 8 commits March 12, 2021 22:12

ui(form): Refactor the algorithm component

b1ae8b4

Create a common component for the algorithm settings. We will need this for Early Stopping, which also has algorithm settings. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

ui(form): Add support for early stopping

f02703d

Add a distinct section for Early Stopping, when the Search Algorithm is for Hyper Parameter tuning. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

ui(form): Add resume policy to form

15ab94f

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

review: Use correct form group

ba2d777

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>

review: Use LongRunning for resumepolicy default

12e2dee

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

kimwnasptd force-pushed the feature-kimwnasptd-missing-form-fields branch from 0527881 to 560a767 Compare March 12, 2021 20:13

andreyvelich approved these changes Mar 12, 2021

View reviewed changes

google-oss-robot assigned andreyvelich Mar 12, 2021

google-oss-robot added the lgtm label Mar 12, 2021

google-oss-robot added the approved label Mar 12, 2021

google-oss-robot merged commit 45c5727 into kubeflow:master Mar 12, 2021

kimwnasptd mentioned this pull request Jun 1, 2021

[UI bug] Missing configuration fields in form #1443

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing form fields to new UI #1463

Add missing form fields to new UI #1463

kimwnasptd commented Mar 10, 2021

andreyvelich left a comment

kimwnasptd commented Mar 12, 2021

google-cla bot commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich left a comment

google-oss-robot commented Mar 12, 2021

Add missing form fields to new UI #1463

Add missing form fields to new UI #1463

Conversation

kimwnasptd commented Mar 10, 2021

andreyvelich left a comment

Choose a reason for hiding this comment

kimwnasptd commented Mar 12, 2021

google-cla bot commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich commented Mar 12, 2021

kimwnasptd commented Mar 12, 2021

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-robot commented Mar 12, 2021