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

Update layout of serve page to be deployments first #42079

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

alanwguo
Copy link
Contributor

Why are these changes needed?

Part 1 of 2 changes:

Part 1:

  • Updates layout of serve ray dashboard to be deployments first.
  • Creates a deployments detail page
  • Update recent serve card in Overview page to point to deployments instead of applications.
  • [Optimization]: Re-use the same SWR cache key for the getServeApplications call so we don't have to refetch the API as often and so the data is kept in sync between pages.

Part 2 (Future PR):

  • Add a multi log viewer to serve deployments list page and serve deployments detail page.

Screenshot 2023-12-21 at 6 38 15 PM
Screenshot 2023-12-21 at 6 38 20 PM
Screenshot 2023-12-21 at 6 53 11 PM

Related issue number

#42055

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 :(

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

LGTM.

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

tchordia commented Jan 2, 2024

what does the deployment details page look like?

@csivanich
Copy link
Contributor

How does this work with multiple applications, seeing that Ray 2.9 no longer has a single-application mode

@alanwguo
Copy link
Contributor Author

alanwguo commented Jan 3, 2024

@tchordia , please take a look in this PR: alanwguo#76 (comment)

@csivanich, the top-level page will show all deployments across all applications. Each deployment has a column for the parent application it belongs to. A user can click the application name to go into an application detail page if they only want to see info about a single application.

The idea here is we were trying to address the feedback that it takes too many clicks to get to valuable information and that majority of workflows, people care primarily about deployments and not applications. So this is moving things from being "applications first" to "deployments first"

@tchordia
Copy link
Contributor

tchordia commented Jan 8, 2024

i think it would also be very helpful to put actor logs on the replica page -- this is another level of unnecessary clicking to get to the actor logs

Copy link
Contributor

@tchordia tchordia left a comment

Choose a reason for hiding this comment

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

lgtm [didn't review code, just reviewed layout. lmk if you need code review as well]. Can we add an application filter?

@@ -250,8 +254,8 @@ const App = () => {
/>
<Route
element={
<SideTabPage tabId="applications">
<ServeApplicationsListPage />
<SideTabPage tabId="deployments">
Copy link
Member

Choose a reason for hiding this comment

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

what happened to applications? can they still be viewed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no application list page, but you can see the name of the application next to each deployment, and you can also use a filter to filter down to specific applications.

Someone can also still go to the application detail page by clicking into an application.

In general, people who use ray serve deal directly with deployments way more than applications.

Copy link
Member

Choose a reason for hiding this comment

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

ok, maybe this should be in a release note on next ray release


export const JobRunningIcon = ({
className,
small = false,
"data-testid": dataTestId,
Copy link
Member

Choose a reason for hiding this comment

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

nit: if possible, if there's an ...IconProps type in react-icons, i'd prefer that we use ...iconProps instead of "data-testid".

Then it can be applied to the <Ri... component as

  return (
    <RiLoader4Line
        ...iconProps

and the caller can still do

<JobRunningIcon data-testid="blah"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-01-11 at 10 07 03 PM

didn't seem to work, but I can try to make the test work with a11y attributes instead of data-testid.

// Use mock data by uncommenting the following line
// const applications = mockServeApplications.applications;
const { allServeApplications: applications } = useServeApplications();
const { allServeDeployments: deployments } = useServeDeployments();
Copy link
Member

Choose a reason for hiding this comment

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

can we type some of the API response calls, such as allServeDeployments? it makes developing on the code hard for external ppl (like the ["application.last_deployed_time_s", "name"] part on L27)

it may be worth looking into at some point if we can automatically export typescript models based on ray @PublicAPI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's typed right here: https://github.com/ray-project/ray/blob/master/dashboard/client/src/type/serve.ts#L112

I do want to do generated types from the API but we don't have the infra for that yet right now.

@architkulkarni architkulkarni merged commit b0d3e2b into ray-project:master Jan 12, 2024
9 checks passed
vickytsang pushed a commit to ROCm/ray that referenced this pull request Jan 12, 2024
…project#42079)

Part 1 of 2 changes:

Part 1:

Updates layout of serve ray dashboard to be deployments first.
Creates a deployments detail page
Update recent serve card in Overview page to point to deployments instead of applications.
[Optimization]: Re-use the same SWR cache key for the getServeApplications call so we don't have to refetch the API as often and so the data is kept in sync between pages.
Part 2 (Future PR):

Add a multi log viewer to serve deployments list page and serve deployments detail page.

---------

Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo alanwguo mentioned this pull request Jan 16, 2024
8 tasks
architkulkarni pushed a commit that referenced this pull request Jan 22, 2024
Part 1 of 2 changes:

Part 1: #42079

Part 2:

Add a multi log viewer to serve deployments list page and serve deployments detail page.
Make the log viewer remember the log the user is viewing in the query params so it is preserved when going back through browser history or refreshing the page.
Allow multi tab log viewer remember the last tab that was selected and start on that tab next time.

---------

Signed-off-by: Alan Guo <aguo@anyscale.com>
architkulkarni added a commit that referenced this pull request Feb 29, 2024
…rows (#43506)

Adds back "application" rows to the top-level deployments page, which were removed in #42079. We keep the deployment rows as a dropdown under the application row, similar to how worker rows are a dropdown under node rows in the "Cluster" page (and we mimic the implementation from that part as well).

Per discussion offline:

Delete the "filters" component because it's not necessary for most use cases, and the user can always cmd-F to find in page if necessary
Start all the rows expanded
Replace the "deployments status" chip with "application status". If there's time, we can try to include both by making the metadata section number of columns configurable and set it to 4 instead of 3.
I also took a couple liberties which hadn't been discussed yet, but happy to revert them if they don't make sense:

Un-bold the the deployment name, but keep the application name bold. This highlights the hierarchical nature and makes the "deployment" rows more visually distinct from the "application" rows.
Make the number of replicas link to the deployment page where it shows the replicas. Reason: when I first saw "2" under replicas my first thought was "where can I see more about these 2 replicas", and I naturally wanted to click it.
---------

Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
Co-authored-by: Huaiwei Sun <scottsun94@gmail.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.

6 participants