[Dashboard] [Serve] Add application row with dropdown for deployment rows #43506

architkulkarni · 2024-02-28T18:33:27Z

Why are these changes needed?

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.

TODO before merge:

Fix pagination calculation
Fix lint
Fix dashboard tests
(If time) configure num_columns for metadata section

Related issue number

Closes #43350

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: Archit Kulkarni <[email protected]>

architkulkarni · 2024-02-28T18:43:45Z

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

- application={deployment.application}
+ .map((application) => (
+ <ServeApplicationRows
+ key={`${application.name}__${application.deployments.length}`}


Not confident about this key. @alanwguo what would you recommend? Does it make sense to use deployments.length here to cause some kind of refresh if the number of deployments changes?

You should only use application name here.

key is not used to determine if something needs to refresh or not. React determines it if any of the properties that are passed in changes. key is only used to see if components can be re-used across renders. React can easily determine whether to re-use components if the number of components is static, but when it's dynamic like in a list, react struggles and key is used as a hint to tell react which components can be re-used.

If a component is not re-used, it is unmounted and a new component is mounted with the updated props.

If a component is re-used, the component does not get unmounted, but the new props do get passed into the component.

Thanks, fixed be06ee8

architkulkarni · 2024-02-28T18:51:57Z

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

+ }}
+ />
+ <Pagination
+ count={Math.ceil(serveDeployments.length / page.pageSize)}


The pagination logic needs to be fixed before we merge. Right now it's calculating the count based on the number of deployments, but below we're using it to slice the Applications array.

One option is to get rid of the page component entirely. Another option is to somehow incorporate the total number of deployments and application rows into the page calculation. The thing to be careful about is we can't split a single application and its dropdown deployments across multiple pages.

Apart from that, this is ready for review @alanwguo @scottsun94. Feel free to suggest other reviewers as well

let's just show this as the count of applications instead of deployments. We'll paginate based on application.

The hope is that most applications won't have a huge amount of deployments.

+1 to paginate based on application for now

Sounds good. Fixed 4a7d868

…e-app-row-dashboard Signed-off-by: Archit Kulkarni <[email protected]>

alanwguo · 2024-02-28T20:43:58Z

dashboard/client/src/pages/serve/ServeApplicationRow.tsx

+ applicationName: name,
+ application: application,


do we still need these properties. I think since we removed the application column, we might be able to get rid of this.

Good call, we don't need it all because this part of the code is scoped to a single application. Fixed fdf617b

alanwguo · 2024-02-28T20:45:24Z

dashboard/client/src/pages/serve/ServeApplicationRow.tsx

+ {/* placeholder for num_replicas, which does not apply to an application */}
+ -


let's instead calculate the total number of replicas from all the deployments in this application.

@scottsun94 @edoakes wdyt? On one hand it makes sense logically to do this, but one thing I wonder if people might get confused and think the application concept can have replicas

We can start without showing it? If people ask for it, we can add

+1 ^ I think it'll probably be more confusing than anything

I'll leave it out for now.

alanwguo · 2024-02-28T20:45:58Z

dashboard/client/src/pages/serve/ServeDeploymentRow.tsx

@@ -24,12 +24,10 @@ import { useViewServeDeploymentMetricsButtonUrl } from "./ServeDeploymentMetrics
 const useStyles = makeStyles((theme) =>
 createStyles({
 deploymentName: {
- fontWeight: 500,
+ fontWeight: 400,


alanwguo · 2024-02-28T20:46:47Z

dashboard/client/src/pages/serve/ServeDeploymentRow.tsx

 deployment: ServeDeployment;
 application: ServeApplication;
+ // Optional prop to control the visibility of the first column.
+ // This is used to display an expand/collapse button on the applications page, but not the deployment page.
+ showFirstColumn?: boolean;


Suggested change

showFirstColumn?: boolean;

showExpandColumn?: boolean;

nit

Fixed adaf486

alanwguo · 2024-02-28T20:47:46Z

dashboard/client/src/pages/serve/ServeDeploymentRow.tsx

- application: { last_deployed_time_s, name: applicationName, route_prefix },
-}: ServeDeployentRowProps) => {
+ application: { last_deployed_time_s, name: applicationName },
+ showFirstColumn = true


let's make default be false for booleans. This is the more expected behavior for component optional props.

If we want showing first column to be the default behavior, we should rename the property hideFirstColumn

Thanks, good to know. Made the default false 6f8c0e3

alanwguo · 2024-02-28T20:48:40Z

dashboard/client/src/pages/serve/ServeDeploymentRow.tsx

- >
- {applicationName}
- </Link>
+ {/* placeholder for route_prefix, which does not apply to a deployment */}


let's add - to represent "no value"

Fixed 5fe1366

alanwguo · 2024-02-28T20:50:06Z

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

+ }}
+ />
+ <Pagination
+ count={Math.ceil(serveDeployments.length / page.pageSize)}


let's just show this as the count of applications instead of deployments. We'll paginate based on application.

The hope is that most applications won't have a huge amount of deployments.

alanwguo · 2024-02-28T20:52:58Z

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

- application={deployment.application}
+ .map((application) => (
+ <ServeApplicationRows
+ key={`${application.name}__${application.deployments.length}`}


You should only use application name here.

key is not used to determine if something needs to refresh or not. React determines it if any of the properties that are passed in changes. key is only used to see if components can be re-used across renders. React can easily determine whether to re-use components if the number of components is static, but when it's dynamic like in a list, react struggles and key is used as a hint to tell react which components can be re-used.

If a component is not re-used, it is unmounted and a new component is mounted with the updated props.

If a component is re-used, the component does not get unmounted, but the new props do get passed into the component.

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

scottsun94 · 2024-02-28T21:02:12Z

LGTM. Left a minor comment.

Signed-off-by: Archit Kulkarni <[email protected]>

Co-authored-by: Huaiwei Sun <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

Signed-off-by: Archit Kulkarni <[email protected]>

…lkarni/ray into serve-app-row-dashboard Signed-off-by: Archit Kulkarni <[email protected]>

Signed-off-by: Archit Kulkarni <[email protected]>

alanwguo

awesome work!

alanwguo · 2024-02-28T22:44:12Z

dashboard/client/src/pages/serve/ServeApplicationRow.tsx

+ key={`${application.name}-${deployment.name}`}
+ deployment={deployment}
+ application={application}
+ showExpandColumn={true}


nit: with react components, instead of sending in ={true} you can just do <Component showExpandColumn />.

This is also the reason why dropping the showExpandColumn should default the value to false.

Thanks, good to know! Fixed bd1b086

alanwguo · 2024-02-28T22:44:53Z

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

+ <ServeApplicationRows
+ key={`${application.name}`}
+ application={application}
+ startExpanded={true}


Fixed bd1b086

Signed-off-by: Archit Kulkarni <[email protected]>

…tests Signed-off-by: Archit Kulkarni <[email protected]>

Signed-off-by: Archit Kulkarni <[email protected]>

…rows (ray-project#43506) Adds back "application" rows to the top-level deployments page, which were removed in ray-project#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 <[email protected]> Co-authored-by: Huaiwei Sun <[email protected]>

Add application row with dropdown for deployment rows

8fca290

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni assigned alanwguo and scottsun94 Feb 28, 2024

architkulkarni added the ray 2.10 label Feb 28, 2024

architkulkarni added 3 commits February 28, 2024 10:36

Remove unused styles

45cf9e9

Signed-off-by: Archit Kulkarni <[email protected]>

remove unused tablecontainer

4c79ea1

Signed-off-by: Archit Kulkarni <[email protected]>

improve comment for first column

41a36a1

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni commented Feb 28, 2024

View reviewed changes

Merge branch 'master' of https://github.com/ray-project/ray into serv…

c9c41a4

…e-app-row-dashboard Signed-off-by: Archit Kulkarni <[email protected]>

alanwguo reviewed Feb 28, 2024

View reviewed changes

scottsun94 reviewed Feb 28, 2024

View reviewed changes

dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx Outdated Show resolved Hide resolved

architkulkarni assigned edoakes Feb 28, 2024

architkulkarni and others added 10 commits February 28, 2024 13:33

Remove deployments length from application row key

be06ee8

Signed-off-by: Archit Kulkarni <[email protected]>

Paginate based on applications

4a7d868

Signed-off-by: Archit Kulkarni <[email protected]>

Rename showFirstColumn to showExpandColumn

adaf486

Signed-off-by: Archit Kulkarni <[email protected]>

Make default for showExpandColumn false

6f8c0e3

Signed-off-by: Archit Kulkarni <[email protected]>

Add placeholder for route_prefix in ServeDeploymentRow component

5fe1366

Signed-off-by: Archit Kulkarni <[email protected]>

Update dashboard/client/src/pages/serve/ServeDeploymentsListPage.tsx

bbbdc32

Co-authored-by: Huaiwei Sun <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

Remove application and applicationName from deploymentsList map

fdf617b

Signed-off-by: Archit Kulkarni <[email protected]>

Merge branch 'serve-app-row-dashboard' of https://github.com/architku…

decd1ad

…lkarni/ray into serve-app-row-dashboard Signed-off-by: Archit Kulkarni <[email protected]>

Remove unnecessary column in ServeApplicationDetailPage

574368a

Signed-off-by: Archit Kulkarni <[email protected]>

Lint

2c54af9

Signed-off-by: Archit Kulkarni <[email protected]>

alanwguo approved these changes Feb 28, 2024

View reviewed changes

architkulkarni added 4 commits February 28, 2024 14:55

Fix import statement in useServeApplications hook

715030e

Signed-off-by: Archit Kulkarni <[email protected]>

Update assertions in ServeDeploymentsListPage and ServeSystemDetails …

75281a8

…tests Signed-off-by: Archit Kulkarni <[email protected]>

Use shorthand for boolean props in React components

bd1b086

Signed-off-by: Archit Kulkarni <[email protected]>

Merge branch 'master' into serve-app-row-dashboard

ceb66d6

architkulkarni merged commit 9e419f4 into ray-project:master Feb 29, 2024
9 checks passed

architkulkarni mentioned this pull request Mar 7, 2024

[Dashboard] [Serve] Rename "Serve Deployments" tab to "Serve" #43799

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] [Serve] Add application row with dropdown for deployment rows #43506

[Dashboard] [Serve] Add application row with dropdown for deployment rows #43506

architkulkarni commented Feb 28, 2024 •

edited

Loading

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

scottsun94 Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

scottsun94 Feb 28, 2024

edoakes Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024

alanwguo Feb 28, 2024

alanwguo Feb 28, 2024

scottsun94 commented Feb 28, 2024

alanwguo left a comment

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024 •

edited

Loading

alanwguo Feb 28, 2024

architkulkarni Feb 28, 2024 •

edited

Loading

		{/* placeholder for num_replicas, which does not apply to an application */}
		-

[Dashboard] [Serve] Add application row with dropdown for deployment rows #43506

[Dashboard] [Serve] Add application row with dropdown for deployment rows #43506

Conversation

architkulkarni commented Feb 28, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

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

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

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

scottsun94 commented Feb 28, 2024

alanwguo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

architkulkarni commented Feb 28, 2024 •

edited

Loading

architkulkarni Feb 28, 2024 •

edited

Loading

architkulkarni Feb 28, 2024 •

edited

Loading