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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions dashboard/client/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ import {
ServeApplicationDetailLayout,
ServeApplicationDetailPage,
} from "./pages/serve/ServeApplicationDetailPage";
import { ServeApplicationsListPage } from "./pages/serve/ServeApplicationsListPage";
import {
ServeDeploymentDetailLayout,
ServeDeploymentDetailPage,
} from "./pages/serve/ServeDeploymentDetailPage";
import { ServeDeploymentsListPage } from "./pages/serve/ServeDeploymentsListPage";
import { ServeLayout, ServeSideTabLayout } from "./pages/serve/ServeLayout";
import { ServeReplicaDetailLayout } from "./pages/serve/ServeReplicaDetailLayout";
import { ServeReplicaDetailPage } from "./pages/serve/ServeReplicaDetailPage";
Expand Down Expand Up @@ -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

<ServeDeploymentsListPage />
</SideTabPage>
}
path=""
Expand All @@ -273,11 +277,17 @@ const App = () => {
>
<Route element={<ServeApplicationDetailPage />} path="" />
<Route
element={<ServeReplicaDetailLayout />}
path=":deploymentName/:replicaId"
element={<ServeDeploymentDetailLayout />}
path=":deploymentName"
>
<Route element={<ServeReplicaDetailPage />} path="" />
<Route path="tasks/:taskId" element={<TaskPage />} />
<Route element={<ServeDeploymentDetailPage />} path="" />
<Route
element={<ServeReplicaDetailLayout />}
path=":replicaId"
>
<Route element={<ServeReplicaDetailPage />} path="" />
<Route path="tasks/:taskId" element={<TaskPage />} />
</Route>
</Route>
</Route>
</Route>
Expand Down
5 changes: 4 additions & 1 deletion dashboard/client/src/common/JobStatus.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Box, createStyles, makeStyles } from "@material-ui/core";
import classNames from "classnames";
import React from "react";
import { IconBaseProps } from "react-icons";
import {
RiCheckboxCircleFill,
RiCloseCircleFill,
Expand Down Expand Up @@ -39,11 +40,12 @@ const useJobRunningIconStyles = makeStyles((theme) =>
}),
);

type JobRunningIconProps = { small?: boolean } & ClassNameProps;
type JobRunningIconProps = { small?: boolean } & ClassNameProps & IconBaseProps;

export const JobRunningIcon = ({
className,
small = false,
...props
}: JobRunningIconProps) => {
const classes = useJobRunningIconStyles();
return (
Expand All @@ -56,6 +58,7 @@ export const JobRunningIcon = ({
},
className,
)}
{...props}
/>
);
};
Expand Down
50 changes: 16 additions & 34 deletions dashboard/client/src/common/ServeStatus.component.test.tsx
Original file line number Diff line number Diff line change
@@ -1,60 +1,42 @@
import { render, screen } from "@testing-library/react";
import React from "react";
import { ServeApplication, ServeApplicationStatus } from "../type/serve";
import { ServeDeployment, ServeDeploymentStatus } from "../type/serve";
import { ServeStatusIcon } from "./ServeStatus";

const APP: ServeApplication = {
name: "MyServeApp",
route_prefix: "/my-serve-app",
docs_path: null,
status: ServeApplicationStatus.RUNNING,
message: "",
last_deployed_time_s: 1682029771.0748637,
deployed_app_config: null,
deployments: {},
const DEPLOYMENT: ServeDeployment = {
name: "MyServeDeployment",
deployment_config: {} as any,
message: "Running",
replicas: [],
status: ServeDeploymentStatus.HEALTHY,
};

describe("ServeStatusIcon", () => {
it("renders RUNNING status", async () => {
render(<ServeStatusIcon app={APP} small={false} />);
it("renders HEALTHY status", async () => {
render(<ServeStatusIcon deployment={DEPLOYMENT} small={false} />);

await screen.findByTestId("serve-status-icon");

const icon = screen.getByTestId("serve-status-icon");
const classList = icon.getAttribute("class");
expect(classList).toContain("colorSuccess");
await screen.findByTitle("Healthy");
});

it("renders NOT_STARTED status", async () => {
it("renders UNHEALTHY status", async () => {
render(
<ServeStatusIcon
app={{ ...APP, status: ServeApplicationStatus.NOT_STARTED }}
deployment={{ ...DEPLOYMENT, status: ServeDeploymentStatus.UNHEALTHY }}
small={false}
/>,
);

await screen.findByTestId("serve-status-icon");

expect(screen.queryByTestId("serve-status-icon")).not.toHaveClass(
"colorSuccess",
);
expect(screen.queryByTestId("serve-status-icon")).not.toHaveClass(
"colorError",
);
await screen.findByTitle("Unhealthy");
});

it("renders DEPLOY_FAILED status", async () => {
it("renders UPDATING status", async () => {
render(
<ServeStatusIcon
app={{ ...APP, status: ServeApplicationStatus.DEPLOY_FAILED }}
deployment={{ ...DEPLOYMENT, status: ServeDeploymentStatus.UPDATING }}
small={false}
/>,
);

await screen.findByTestId("serve-status-icon");

const icon = screen.getByTestId("serve-status-icon");
const classList = icon.getAttribute("class");
expect(classList).toContain("colorError");
await screen.findByTitle("Updating");
});
});
37 changes: 11 additions & 26 deletions dashboard/client/src/common/ServeStatus.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
import { createStyles, makeStyles } from "@material-ui/core";
import classNames from "classnames";
import React from "react";
import {
RiCloseCircleFill,
RiRecordCircleFill,
RiStopCircleFill,
} from "react-icons/ri";
import { ServeApplication } from "../type/serve";
import { RiCloseCircleFill, RiRecordCircleFill } from "react-icons/ri";
import { ServeDeployment } from "../type/serve";
import { JobRunningIcon } from "./JobStatus";
import { ClassNameProps } from "./props";

type ServeStatusIconProps = {
app: ServeApplication;
deployment: ServeDeployment;
small: boolean;
} & ClassNameProps;

Expand All @@ -36,42 +32,31 @@ const useServeStatusIconStyles = makeStyles((theme) =>
);

export const ServeStatusIcon = ({
app,
deployment,
small,
className,
}: ServeStatusIconProps) => {
const classes = useServeStatusIconStyles();

switch (app.status) {
case "RUNNING":
switch (deployment.status) {
case "HEALTHY":
return (
<RiRecordCircleFill
data-testid="serve-status-icon"
className={classNames(classes.icon, classes.colorSuccess)}
title="Healthy"
/>
);
case "NOT_STARTED":
return (
<RiStopCircleFill
data-testid="serve-status-icon"
className={classes.icon}
/>
);
case "DEPLOY_FAILED":
case "UNHEALTHY":
return (
<RiCloseCircleFill
data-testid="serve-status-icon"
className={classNames(classes.icon, classes.colorError)}
title="Unhealthy"
/>
);
default:
// DEPLOYING || DELETEING
// UPDATING
return (
<JobRunningIcon
data-testid="serve-status-icon"
className={className}
small={small}
/>
<JobRunningIcon className={className} small={small} title="Updating" />
);
}
};
4 changes: 4 additions & 0 deletions dashboard/client/src/common/props.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export type ClassNameProps = {
className?: string;
};

export type DataTestIdProps = {
"data-testid"?: string;
};
2 changes: 1 addition & 1 deletion dashboard/client/src/pages/layout/MainNavLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ const NAV_ITEMS = [
id: "jobs",
},
{
title: "Serve",
title: "Serve Deployments",
path: "/serve",
id: "serve",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ describe("RecentServeCard", () => {
import_path: "home:graph",
},
last_deployed_time_s: new Date().getTime() / 1000,
deployments: {
FirstDeployment: {
name: "FirstDeployment",
},
},
},
"second-app": {
name: "second-app",
Expand All @@ -36,48 +41,52 @@ describe("RecentServeCard", () => {
status: ServeApplicationStatus.DEPLOYING,
deployed_app_config: null,
last_deployed_time_s: new Date().getTime() / 1000,
deployments: {},
deployments: {
SecondDeployment: {
name: "SecondDeployment",
},
},
},
},
},
} as any);
});

it("should display serve applications with deployed_app_config", async () => {
it("should display serve deployments with deployed_app_config", async () => {
render(<RecentServeCard />, {
wrapper: TEST_APP_WRAPPER,
});

await screen.findByText("View all applications");
await screen.findByText("View all deployments");

expect.assertions(3);
expect(screen.getByText("home")).toBeInTheDocument();
expect(screen.getByText("FirstDeployment")).toBeInTheDocument();
expect(screen.getByText("home:graph")).toBeInTheDocument();
expect(screen.getByText("Serve Applications")).toBeInTheDocument();
expect(screen.getByText("Serve Deployments")).toBeInTheDocument();
});

it("should display serve applications without deployed_app_config", async () => {
it("should display serve deployments without deployed_app_config", async () => {
render(<RecentServeCard />, {
wrapper: TEST_APP_WRAPPER,
});

await screen.findByText("View all applications");
await screen.findByText("View all deployments");

expect.assertions(3);
expect(screen.getByText("second-app")).toBeInTheDocument();
expect(screen.getByText("-")).toBeInTheDocument(); // default value for no deployed_app_config
expect(screen.getByText("Serve Applications")).toBeInTheDocument();
expect(screen.getByText("SecondDeployment")).toBeInTheDocument();
expect(screen.getByText("second-app")).toBeInTheDocument(); // default value for no deployed_app_config
expect(screen.getByText("Serve Deployments")).toBeInTheDocument();
});

it("should navigate to the applications page when the 'View all applications' link is clicked", async () => {
it("should navigate to the applications page when the 'View all deployments' link is clicked", async () => {
render(<RecentServeCard />, {
wrapper: TEST_APP_WRAPPER,
});

await screen.findByText("View all applications");
await screen.findByText("View all deployments");
const link = screen.getByRole("link", {
name: /view all applications/i,
name: /view all deployments/i,
});
expect(link).toHaveAttribute("href");
expect(link).toHaveAttribute("href", "/serve");
});
});
46 changes: 29 additions & 17 deletions dashboard/client/src/pages/overview/cards/RecentServeCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import _ from "lodash";
import React from "react";
import { ServeStatusIcon } from "../../../common/ServeStatus";
import { ListItemCard } from "../../../components/ListItemCard";
import { useServeApplications } from "../../serve/hook/useServeApplications";
import { useServeDeployments } from "../../serve/hook/useServeApplications";

const useStyles = makeStyles((theme) =>
createStyles({
Expand All @@ -20,33 +20,45 @@ type RecentServeCardProps = {
export const RecentServeCard = ({ className }: RecentServeCardProps) => {
const classes = useStyles();

// 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.


const sortedApplications = _.orderBy(
applications,
["last_deployed_time_s"],
["desc"],
const sortedDeployments = _.orderBy(
deployments,
["application.last_deployed_time_s", "name"],
["desc", "asc"],
).slice(0, 6);

const sortedApplicationsToRender = sortedApplications.map((app) => {
const sortedDeploymentsToRender = sortedDeployments.map((deployment) => {
return {
title: app.name,
subtitle: app?.deployed_app_config?.import_path || "-",
link: app.name ? `/serve/applications/${app.name}` : undefined,
title: deployment.name,
subtitle:
deployment.application.deployed_app_config?.import_path ||
deployment.application.name ||
deployment.application.route_prefix,
link:
deployment.application.name && deployment.name
? `/serve/applications/${encodeURIComponent(
deployment.application.name,
)}/${encodeURIComponent(deployment.name)}`
: undefined,
className: className,
icon: <ServeStatusIcon className={classes.icon} app={app} small />,
icon: (
<ServeStatusIcon
className={classes.icon}
deployment={deployment}
small
/>
),
};
});

return (
<ListItemCard
headerTitle="Serve Applications"
headerTitle="Serve Deployments"
className={className}
items={sortedApplicationsToRender}
emptyListText="No Applications yet..."
footerText="View all applications"
items={sortedDeploymentsToRender}
emptyListText="No Deployments yet..."
footerText="View all deployments"
footerLink="/serve"
/>
);
Expand Down
Loading
Loading