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

[Dashboard] Update Serve System UI #36787

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

alanwguo
Copy link
Contributor

@alanwguo alanwguo commented Jun 24, 2023

Why are these changes needed?

  • Add logs to the main serve page
  • Put serve system details on a separate tab, but show a preview on the main Serve page
Screenshot 2023-06-23 at 7 41 47 PM Screenshot 2023-06-23 at 7 46 06 PM Screenshot 2023-06-23 at 7 47 21 PM

Related issue number

fixes #36630

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

- Add logs to the main serve page
- Put serve system details on a separate tab, but show a preview on the main Serve page

Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo
Copy link
Contributor Author

Still need to write tests, but ready for UX review @scottsun94

@scottsun94
Copy link
Contributor

Do we still show all statuses and their counts (instead of showing only 3 as we discussed)? Other changes look good.

@alanwguo
Copy link
Contributor Author

Do we still show all statuses and their counts (instead of showing only 3 as we discussed)? Other changes look good.

Decided to just show all states. Since you mentioned you were okay with the contents wrapping to the next line, it's easier to just show all states.
I'll put up a screenshot of how that looks later today.

@alanwguo
Copy link
Contributor Author

alanwguo commented Jun 26, 2023

@scottsun94
Screenshot 2023-06-26 at 4 55 28 PM

@scottsun94
Copy link
Contributor

scottsun94 commented Jun 27, 2023

@alanwguo Can we add a 4px padding for each chip/pill so that it looks less crowded?
Screenshot 2023-06-26 at 5 15 14 PM

@alanwguo
Copy link
Contributor Author

Screenshot 2023-06-26 at 5 35 20 PM

Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo alanwguo marked this pull request as ready for review June 27, 2023 00:44
@scottsun94
Copy link
Contributor

LGTM.

{serveDetails.http_options === undefined ? (
<Alert className={classes.serveInstanceWarning} severity="warning">
Serve not started. Please deploy a serve application first.
</Alert>
) : (
<React.Fragment>
<ServeSystemDetails
serveDetails={serveDetails}
<ServeSystemPreview
Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2023-06-23 at 7 46 06 PM

Shouldn't ServeSystemPreview be shown in ServeSystemDetailPage instead? Along with the logs panel (title="Logs")

We can also remove having Serve applications be a collapsible section, and make the title of the page Applications instead (similar to System tab)

Copy link
Contributor

@scottsun94 scottsun94 Jun 27, 2023

Choose a reason for hiding this comment

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

We can also remove having Serve applications be a collapsible section, and make the title of the page Applications instead (similar to System tab)

Serve applications -> Applications makes sense.

Let's keep the collapsable behavior for now since it doesn't hurt? I don't know if people might want to collapse it when the table is long and they just want to focus on logs and metrics.

@galenhwang
Copy link
Member

galenhwang commented Jun 27, 2023

Screenshot 2023-06-23 at 7 46 06 PM

the Serve / Applications breadcrumb seems to not show for the tab ignore, i see this is consistent with other pages

alanwguo added 2 commits June 27, 2023 19:19
Signed-off-by: Alan Guo <aguo@anyscale.com>
Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo
Copy link
Contributor Author

@galenhwang , please re-review

@alanwguo
Copy link
Contributor Author

@rkooo567 ready for merge!

Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo
Copy link
Contributor Author

Added an additional change to expand deployments by default in some cases. Will ping once tests pass

@rkooo567 rkooo567 merged commit fcef310 into ray-project:master Jun 28, 2023
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
Add logs to the main serve page
Put serve system details on a separate tab, but show a preview on the main Serve page

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Serve| Observability] Show detailed error for the Serve Application in Dashboard
5 participants