[kwa-trials-logs] Create the LOGS tab of Trial's details page in KWA #2101

elenzio9 · 2023-01-25T09:21:07Z

In this PR:

Create a distinct LOGS tab, which displays the trial's logs in the Trial details page.
Don't show the backend's error popup for logs, but show the message error in the admonition.
Update the message the backend sends to not just expose that logs are not there because retain might not be set, but also because the cluster was scaled down.

Screenshots

Show the trial's logs

If the user is in the logs tab and the logs request is not 200, then show the message error in the admonition

Related issue: #1764
Related PR: #2039

johnugeorge · 2023-01-25T09:53:55Z

Thanks @elenzio9 for this

/assign @kimwnasptd

tenzen-y · 2023-01-25T09:58:07Z

/assign @orfeas-k

johnugeorge · 2023-01-25T10:25:22Z

pkg/new-ui/v1beta1/backend.go

@@ -705,7 +705,8 @@ func fetchMasterPodName(clientset *kubernetes.Clientset, trial *trialsv1beta1.Tr
 	if len(podList.Items) == 0 {
 		return "", errors.New(`Logs for the trial could not be found.
 Was 'retain: true' specified in the Experiment definition?
-An example can be found here: https://github.com/kubeflow/katib/blob/7bf39225f7235ee4ba6cf285ecc2c455c6471234/examples/v1beta1/argo/argo-workflow.yaml#L33`)
+An example can be found here: https://github.com/kubeflow/katib/blob/7bf39225f7235ee4ba6cf285ecc2c455c6471234/examples/v1beta1/argo/argo-workflow.yaml#L33.


This one seems to be verbose for a data scientist. May be just log message instead of showing in UI?

@johnugeorge Do you mean to remove the example completely?

I agree that this might be too verbose for end users. I'd suggest to change this text to something like:

Failed to find logs for this Trial. Make sure you've set "spec.trialTemplate.retain" field to "true" in the Experiment definition. If this error persists then the Pod's logs are not currently persisted in the cluster.```

andreyvelich · 2023-01-25T10:59:15Z

Thank you very much for this amazing feature, many users wait for this so long!
cc @d-gol

tenzen-y · 2023-01-26T13:15:57Z

@elenzio9 Can you rebase and resolve conflicts?

kimwnasptd · 2023-01-26T13:54:48Z

@andreyvelich @johnugeorge @tenzen-y I won't be able to review this today, to make it to the RC0. But we can markthis as one of the PRs to merge through the feature freeze. cc @DomFleischmann

This is a bigger one and I wouldn't want to rush it today

tenzen-y · 2023-01-26T13:59:38Z

@andreyvelich @johnugeorge @tenzen-y I won't be able to review this today, to make it to the RC0. But we can markthis as one of the PRs to merge through the feature freeze. cc @DomFleischmann

This is a bigger one and I wouldn't want to rush it today

@kimwnasptd
Thanks for your effort. Does that mean you would like to include this after the first RC release (e.g., RC1)?

andreyvelich · 2023-01-26T14:28:04Z

@kimwnasptd I think, it should be fine to merge this PR after the RC0. cc @johnugeorge

tenzen-y · 2023-01-26T14:31:37Z

I'm ok with not including this in RC0.

elenzio9 · 2023-01-26T14:40:22Z

@elenzio9 Can you rebase and resolve conflicts?

@tenzen-y Just rebased and pushed again!

orfeas-k

Thank you @elenzio9 for the PR! It looks great, here are some minor comments.

Regarding the error from the backend, I 'm not really sure what would be better to show to the user so I 'll also @kimwnasptd comment on this.

orfeas-k · 2023-01-31T13:33:43Z

...ntend/src/app/pages/experiment-details/trials-table/trial-details/trial-details.component.ts

+    this.backendService.getTrialLogs(this.trialName, this.namespace).subscribe(
+      logs => {
+        this.trialLogs = logs;
+        this.logsRequestError = '';


Nit but I think it would make more sense semantically pass null to logsRequestError since there is no error.

orfeas-k · 2023-01-31T14:03:53Z

...c/app/pages/experiment-details/trials-table/trial-details/trial-logs/trial-logs.component.ts

+      return;
+    }
+
+    let arrayOfLogs = trialLogs.split('\n');


This could be a const instead of let.

orfeas-k · 2023-01-31T14:26:10Z

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

+
+    return this.http.get(url).pipe(
+      catchError(error => this.handleError(error, false)),
+      map((resp: any) => {


I think .map doesn't serve a purpose here. We could change this to

return this.http .get(url) .pipe(catchError(error => this.handleError(error, false)));

WDYT @elenzio9 ?

Yes, I agree! @orfeas-k Thanks for your comments! I'll make the changes and push the branch again.

I 'm good then, regarding the frontend: /lgtm

andreyvelich · 2023-02-08T11:12:50Z

@orfeas-k @kimwnasptd @elenzio9 Are we ready to merge this PR ?

andreyvelich · 2023-02-14T10:28:12Z

@kimwnasptd Please review this PR today if you can.

kimwnasptd

Did a pass, had some small comments but the code LGTM! Also deployed it in my cluster and everything works as expected

kimwnasptd · 2023-02-14T11:32:09Z

pkg/new-ui/v1beta1/backend.go

@@ -705,7 +705,8 @@ func fetchMasterPodName(clientset *kubernetes.Clientset, trial *trialsv1beta1.Tr
 	if len(podList.Items) == 0 {
 		return "", errors.New(`Logs for the trial could not be found.
 Was 'retain: true' specified in the Experiment definition?
-An example can be found here: https://github.com/kubeflow/katib/blob/7bf39225f7235ee4ba6cf285ecc2c455c6471234/examples/v1beta1/argo/argo-workflow.yaml#L33`)
+An example can be found here: https://github.com/kubeflow/katib/blob/7bf39225f7235ee4ba6cf285ecc2c455c6471234/examples/v1beta1/argo/argo-workflow.yaml#L33.


I agree that this might be too verbose for end users. I'd suggest to change this text to something like:

Failed to find logs for this Trial. Make sure you've set "spec.trialTemplate.retain" field to "true" in the Experiment definition. If this error persists then the Pod's logs are not currently persisted in the cluster.```

kimwnasptd · 2023-02-14T11:37:07Z

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

+
+    if (error instanceof HttpErrorResponse) {
+      if (this.getBackendErrorLog(error) !== undefined) {
+        return `[${error.status}] ${this.getBackendErrorLog(error)}`;


IIUC this line is the only one that differs from the inherited function. I'd propose to not show the error in this case and not override this function, and just keep the changes from getBackendError.

I removed the error status and now the message looks like:

kimwnasptd · 2023-02-14T11:37:55Z

...c/app/pages/experiment-details/trials-table/trial-details/trial-logs/trial-logs.component.ts

+    const arrayOfLogs = trialLogs.split('\n');
+    this.logs = arrayOfLogs;


nit: can we combine these 2 in one line? I think there's no need to create the arrayOfLogs var

kimwnasptd · 2023-02-14T11:39:02Z

...app/pages/experiment-details/trials-table/trial-details/trial-logs/trial-logs.component.html

+<!--if no logs are present at all then show a warning message-->
+<ng-container *ngIf="logsRequestError">
+  <lib-panel icon="error" color="warn">
+    Error: {{ logsRequestError }}


Let's just keep this as {{ logsRequestError }}. The icon in this panel and the text should make it clear that something unexpected has happened

@kimwnasptd Thanks for the above comments! I force pushed the branch with the changes!

andreyvelich · 2023-02-14T12:25:08Z

It seems that Charmed actions are failing, please can you take a look @DomFleischmann @knkski @ca-scribner?

kimwnasptd · 2023-02-14T13:11:51Z

All the changes look good from my side, thanks for the work on this feature @elenzio9 @d-gol!

/lgtm
/approve

tenzen-y · 2023-02-14T15:17:02Z

@elenzio9 Can you rebase? Since I fixed CI in #2114.

ca-scribner · 2023-02-14T16:47:37Z

@tenzen-y beat me to it, but that's what I would have done as well. Seems like black's defaults changed between v22.x and v23.x.

tenzen-y · 2023-02-14T16:52:44Z

@tenzen-y beat me to it, but that's what I would have done as well. Seems like black's defaults changed between v22.x and v23.x.

@ca-scribner Thanks for letting me know. We may want to pin the black version.

* Update the message the backend sends to not just expose that logs are not there because 'retain' might not be set, but also because the cluster was scaled down. Signed-off-by: Elena Zioga <elena@arrikto.com>

In this commit: * Create a distinct LOGS tab, which displays the trial's logs in the Trial details page. * Don't show the backend's error popup for logs, but show the message error in the admonition. Signed-off-by: Elena Zioga <elena@arrikto.com>

elenzio9 · 2023-02-14T18:06:26Z

@elenzio9 Can you rebase? Since I fixed CI in #2114.

@tenzen-y Just rebased!

tenzen-y

@elenzio9 Greate contribution! Many users are looking forward to this feature!

/lgtm
/approve

@kubeflow/wg-automl-leads Can you restart the flaky tests?

google-oss-prow · 2023-02-14T18:39:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elenzio9, kimwnasptd, tenzen-y

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:

~~pkg/new-ui/v1beta1/OWNERS~~ [kimwnasptd,tenzen-y]

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

tenzen-y · 2023-02-14T19:42:40Z

@elenzio9 IIRC, we plan to include this feature in RC.1 release. So can you create a PR for cherry-picking of this PR like #2115?

…s details page in KWA Signed-off-by: Elena Zioga <elena@arrikto.com> --------- Signed-off-by: Elena Zioga <elena@arrikto.com>

…s details page in KWA Signed-off-by: Elena Zioga <elena@arrikto.com>

elenzio9 · 2023-02-15T08:03:21Z

@elenzio9 IIRC, we plan to include this feature in RC.1 release. So can you create a PR for cherry-picking of this PR like #2115?

@tenzen-y Just sent the PR: #2117.

…ubeflow#2101) * backend: Update error message when no logs could be found * Update the message the backend sends to not just expose that logs are not there because 'retain' might not be set, but also because the cluster was scaled down. Signed-off-by: Elena Zioga <elena@arrikto.com> * frontend: Add LOGS tab in Trial details page In this commit: * Create a distinct LOGS tab, which displays the trial's logs in the Trial details page. * Don't show the backend's error popup for logs, but show the message error in the admonition. Signed-off-by: Elena Zioga <elena@arrikto.com> --------- Signed-off-by: Elena Zioga <elena@arrikto.com>

…s page in KWA (#2117) Signed-off-by: Elena Zioga <elena@arrikto.com>

google-oss-prow bot added the size/L label Jan 25, 2023

google-oss-prow bot requested review from anencore94, gaocegege and kimwnasptd January 25, 2023 09:21

google-oss-prow bot assigned kimwnasptd Jan 25, 2023

google-oss-prow bot assigned orfeas-k Jan 25, 2023

johnugeorge reviewed Jan 25, 2023

View reviewed changes

elenzio9 force-pushed the feature-elena-kwa-trials-logs-tab branch from 1f8185b to cbc8644 Compare January 26, 2023 14:39

orfeas-k reviewed Feb 1, 2023

View reviewed changes

elenzio9 force-pushed the feature-elena-kwa-trials-logs-tab branch 2 times, most recently from ed25489 to 8c65da2 Compare February 1, 2023 10:39

elenzio9 force-pushed the feature-elena-kwa-trials-logs-tab branch from 8c65da2 to 7ae152c Compare February 14, 2023 11:18

kimwnasptd reviewed Feb 14, 2023

View reviewed changes

elenzio9 force-pushed the feature-elena-kwa-trials-logs-tab branch from 7ae152c to 1449b9b Compare February 14, 2023 12:16

google-oss-prow bot added lgtm approved labels Feb 14, 2023

tenzen-y mentioned this pull request Feb 14, 2023

Reformat katib-operators #2114

Merged

1 task

elenzio9 added 2 commits February 14, 2023 20:05

backend: Update error message when no logs could be found

9b10a39

* Update the message the backend sends to not just expose that logs are not there because 'retain' might not be set, but also because the cluster was scaled down. Signed-off-by: Elena Zioga <elena@arrikto.com>

elenzio9 force-pushed the feature-elena-kwa-trials-logs-tab branch from 1449b9b to 340add3 Compare February 14, 2023 18:05

google-oss-prow bot removed the lgtm label Feb 14, 2023

tenzen-y reviewed Feb 14, 2023

View reviewed changes

google-oss-prow bot assigned tenzen-y Feb 14, 2023

google-oss-prow bot added the lgtm label Feb 14, 2023

google-oss-prow bot merged commit 1429d61 into kubeflow:master Feb 14, 2023

elenzio9 mentioned this pull request Feb 15, 2023

Automated cherry pick of #2101: Create the LOGS tab of Trial's details page in KWA #2117

Merged

elenzio9 added a commit to arrikto/katib that referenced this pull request Feb 15, 2023

Automated cherry pick of kubeflow#2101: Create the LOGS tab of Trial'…

6ba9cba

…s details page in KWA Signed-off-by: Elena Zioga <elena@arrikto.com>

johnugeorge mentioned this pull request Feb 15, 2023

Cherry pick: #2101 #2120

Closed

google-oss-prow bot pushed a commit that referenced this pull request Feb 15, 2023

Automated cherry pick of #2101: Create the LOGS tab of Trial's detail…

ff1d4d0

…s page in KWA (#2117) Signed-off-by: Elena Zioga <elena@arrikto.com>

		const arrayOfLogs = trialLogs.split('\n');
		this.logs = arrayOfLogs;

[kwa-trials-logs] Create the LOGS tab of Trial's details page in KWA #2101

[kwa-trials-logs] Create the LOGS tab of Trial's details page in KWA #2101

Conversation

elenzio9 commented Jan 25, 2023

Screenshots

Show the trial's logs

If the user is in the logs tab and the logs request is not 200, then show the message error in the admonition

johnugeorge commented Jan 25, 2023

tenzen-y commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Jan 25, 2023

tenzen-y commented Jan 26, 2023

kimwnasptd commented Jan 26, 2023

tenzen-y commented Jan 26, 2023 • edited

andreyvelich commented Jan 26, 2023

tenzen-y commented Jan 26, 2023

elenzio9 commented Jan 26, 2023

orfeas-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 8, 2023

andreyvelich commented Feb 14, 2023

kimwnasptd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 14, 2023

kimwnasptd commented Feb 14, 2023

tenzen-y commented Feb 14, 2023

ca-scribner commented Feb 14, 2023

tenzen-y commented Feb 14, 2023

elenzio9 commented Feb 14, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Feb 14, 2023

tenzen-y commented Feb 14, 2023

elenzio9 commented Feb 15, 2023

tenzen-y commented Jan 26, 2023 •

edited