-
Notifications
You must be signed in to change notification settings - Fork 441
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
[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
Thanks @elenzio9 for this /assign @kimwnasptd |
/assign @orfeas-k |
pkg/new-ui/v1beta1/backend.go
Outdated
@@ -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. |
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.
This one seems to be verbose for a data scientist. May be just log message instead of showing in UI?
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.
@johnugeorge Do you mean to remove the example completely?
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 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.```
Thank you very much for this amazing feature, many users wait for this so long! |
@elenzio9 Can you rebase and resolve conflicts? |
@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 |
@kimwnasptd I think, it should be fine to merge this PR after the RC0. cc @johnugeorge |
I'm ok with not including this in RC0. |
1f8185b
to
cbc8644
Compare
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.
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.
this.backendService.getTrialLogs(this.trialName, this.namespace).subscribe( | ||
logs => { | ||
this.trialLogs = logs; | ||
this.logsRequestError = ''; |
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.
Nit but I think it would make more sense semantically pass null
to logsRequestError
since there is no error.
return; | ||
} | ||
|
||
let arrayOfLogs = trialLogs.split('\n'); |
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.
This could be a const
instead of let
.
|
||
return this.http.get(url).pipe( | ||
catchError(error => this.handleError(error, false)), | ||
map((resp: any) => { |
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 .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 ?
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, I agree! @orfeas-k Thanks for your comments! I'll make the changes and push the branch again.
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.
Done!
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 'm good then, regarding the frontend: /lgtm
ed25489
to
8c65da2
Compare
@orfeas-k @kimwnasptd @elenzio9 Are we ready to merge this PR ? |
@kimwnasptd Please review this PR today if you can. |
8c65da2
to
7ae152c
Compare
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.
Did a pass, had some small comments but the code LGTM! Also deployed it in my cluster and everything works as expected
pkg/new-ui/v1beta1/backend.go
Outdated
@@ -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. |
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 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.```
|
||
if (error instanceof HttpErrorResponse) { | ||
if (this.getBackendErrorLog(error) !== undefined) { | ||
return `[${error.status}] ${this.getBackendErrorLog(error)}`; |
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.
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
.
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.
const arrayOfLogs = trialLogs.split('\n'); | ||
this.logs = arrayOfLogs; |
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.
nit: can we combine these 2 in one line? I think there's no need to create the arrayOfLogs
var
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, sure!
<!--if no logs are present at all then show a warning message--> | ||
<ng-container *ngIf="logsRequestError"> | ||
<lib-panel icon="error" color="warn"> | ||
Error: {{ logsRequestError }} |
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.
Let's just keep this as {{ logsRequestError }}
. The icon in this panel and the text should make it clear that something unexpected has happened
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.
@kimwnasptd Thanks for the above comments! I force pushed the branch with the changes!
7ae152c
to
1449b9b
Compare
It seems that Charmed actions are failing, please can you take a look @DomFleischmann @knkski @ca-scribner? |
@tenzen-y beat me to it, but that's what I would have done as well. Seems like |
@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>
1449b9b
to
340add3
Compare
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.
@elenzio9 Greate contribution! Many users are looking forward to this feature!
/lgtm
/approve
@kubeflow/wg-automl-leads Can you restart the flaky tests?
[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:
Approvers can indicate their approval by writing |
…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>
…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>
In this PR:
LOGS
tab, which displays the trial's logs in the Trial details page.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