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

feat(robot-server, app): add a new endpoint for fast-fetching all run commands #15031

Merged
merged 14 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from 11 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
22 changes: 22 additions & 0 deletions api-client/src/runs/commands/getCommandsAsPreSerializedList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { GET, request } from '../../request'

import type { ResponsePromise } from '../../request'
import type { HostConfig } from '../../types'
import type {
CommandsAsPreSerializedListData,
GetCommandsParams,
} from './types'

export function getCommandsAsPreSerializedList(
config: HostConfig,
runId: string,
params: GetCommandsParams
): ResponsePromise<CommandsAsPreSerializedListData> {
return request<CommandsAsPreSerializedListData>(
GET,
`/runs/${runId}/commandsAsPreSerializedList`,
null,
config,
params
)
}
6 changes: 6 additions & 0 deletions api-client/src/runs/commands/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ export interface CommandsData {
links: CommandsLinks
}

export interface CommandsAsPreSerializedListData {
data: string[]
meta: GetCommandsParams & { totalLength: number }
links: CommandsLinks
}

export interface CreateCommandParams {
waitUntilComplete?: boolean
timeout?: number
Expand Down
1 change: 1 addition & 0 deletions api-client/src/runs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export { createCommand } from './commands/createCommand'
export { createLiveCommand } from './commands/createLiveCommand'
export { getCommand } from './commands/getCommand'
export { getCommands } from './commands/getCommands'
export { getCommandsAsPreSerializedList } from './commands/getCommandsAsPreSerializedList'
export { createRunAction } from './createRunAction'
export * from './createLabwareOffset'
export * from './createLabwareDefinition'
Expand Down
4 changes: 3 additions & 1 deletion app/src/organisms/CommandText/LoadCommandText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ export const LoadCommandText = ({
return null
}
} else {
const labware = command.result?.definition.metadata.displayName
const labware =
command.result?.definition.metadata.displayName ??
command.params.displayName
return command.params.location === 'offDeck'
? t('load_labware_info_protocol_setup_off_deck', { labware })
: t('load_labware_info_protocol_setup_no_module', {
Expand Down
28 changes: 20 additions & 8 deletions app/src/organisms/RunPreview/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { useTranslation } from 'react-i18next'
import { ViewportList, ViewportListRef } from 'react-viewport-list'

import { RUN_STATUSES_TERMINAL } from '@opentrons/api-client'
import { useAllCommandsQuery } from '@opentrons/react-api-client'
import {
ALIGN_CENTER,
BORDERS,
Expand All @@ -21,7 +20,10 @@ import {
} from '@opentrons/components'

import { useMostRecentCompletedAnalysis } from '../LabwarePositionCheck/useMostRecentCompletedAnalysis'
import { useNotifyLastRunCommandKey } from '../../resources/runs'
import {
useNotifyAllCommandsAsPreSerializedList,
useNotifyLastRunCommandKey,
} from '../../resources/runs'
import { CommandText } from '../CommandText'
import { Divider } from '../../atoms/structure'
import { NAV_BAR_WIDTH } from '../../App/constants'
Expand All @@ -33,6 +35,8 @@ import type { RobotType } from '@opentrons/shared-data'

const COLOR_FADE_MS = 500
const LIVE_RUN_COMMANDS_POLL_MS = 3000
// arbitrary large number of commands
const MAX_COMMANDS = 100000

interface RunPreviewProps {
runId: string
Expand All @@ -52,11 +56,17 @@ export const RunPreviewComponent = (
? (RUN_STATUSES_TERMINAL as RunStatus[]).includes(runStatus)
: false
// we only ever want one request done for terminal runs because this is a heavy request
const commandsFromQuery = useAllCommandsQuery(runId, null, {
staleTime: Infinity,
cacheTime: Infinity,
enabled: isRunTerminal,
}).data?.data
const commandsFromQuery = useNotifyAllCommandsAsPreSerializedList(
runId,
{ cursor: 0, pageLength: MAX_COMMANDS },
{
staleTime: Infinity,
cacheTime: Infinity,
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

i guess we don't need staleTime and cacheTime anymore now that we're using the notification service right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to leave those there in the event that we can't establish MQTT connection and need to fallback to HTTP with those options passed through?

Copy link
Member

Choose a reason for hiding this comment

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

oh yes you are totally right!

enabled: isRunTerminal,
}
).data?.data
const nullCheckedCommandsFromQuery =
commandsFromQuery == null ? robotSideAnalysis?.commands : commandsFromQuery
const viewPortRef = React.useRef<HTMLDivElement | null>(null)
const currentRunCommandKey = useNotifyLastRunCommandKey(runId, {
refetchInterval: LIVE_RUN_COMMANDS_POLL_MS,
Expand All @@ -67,7 +77,9 @@ export const RunPreviewComponent = (
] = React.useState<boolean>(true)
if (robotSideAnalysis == null) return null
const commands =
(isRunTerminal ? commandsFromQuery : robotSideAnalysis.commands) ?? []
(isRunTerminal
? nullCheckedCommandsFromQuery
: robotSideAnalysis.commands) ?? []
const currentRunCommandIndex = commands.findIndex(
c => c.key === currentRunCommandKey
)
Expand Down
1 change: 1 addition & 0 deletions app/src/redux/shell/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export type NotifyTopic =
| 'robot-server/runs'
| `robot-server/runs/${string}`
| 'robot-server/deck_configuration'
| `robot-server/runs/pre_serialized_commands/${string}`

export interface NotifySubscribeAction {
type: 'shell:NOTIFY_SUBSCRIBE'
Expand Down
1 change: 1 addition & 0 deletions app/src/resources/runs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export * from './utils'
export * from './useNotifyAllRunsQuery'
export * from './useNotifyRunQuery'
export * from './useNotifyLastRunCommandKey'
export * from './useNotifyAllCommandsAsPreSerializedList'
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import * as React from 'react'

import { useAllCommandsAsPreSerializedList } from '@opentrons/react-api-client'

import { useNotifyService } from '../useNotifyService'

import type { UseQueryResult } from 'react-query'
import type { AxiosError } from 'axios'
import type { CommandsData, GetCommandsParams } from '@opentrons/api-client'
import type {
QueryOptionsWithPolling,
HTTPRefetchFrequency,
} from '../useNotifyService'

export function useNotifyAllCommandsAsPreSerializedList(
runId: string | null,
params?: GetCommandsParams | null,
options: QueryOptionsWithPolling<CommandsData, AxiosError> = {}
): UseQueryResult<CommandsData, AxiosError> {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService<CommandsData, AxiosError>({
topic: `robot-server/runs/pre_serialized_commands/${runId}`,
setRefetch,
options,
})

const httpResponse = useAllCommandsAsPreSerializedList(runId, params, {
...options,
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
})

return httpResponse
}
1 change: 1 addition & 0 deletions react-api-client/src/runs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export { usePauseRunMutation } from './usePauseRunMutation'
export { useStopRunMutation } from './useStopRunMutation'
export { useRunActionMutations } from './useRunActionMutations'
export { useAllCommandsQuery } from './useAllCommandsQuery'
export { useAllCommandsAsPreSerializedList } from './useAllCommandsAsPreSerializedList'
export { useCommandQuery } from './useCommandQuery'
export * from './useCreateLabwareOffsetMutation'
export * from './useCreateLabwareDefinitionMutation'
Expand Down
52 changes: 52 additions & 0 deletions react-api-client/src/runs/useAllCommandsAsPreSerializedList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { UseQueryResult, useQuery } from 'react-query'
import { getCommandsAsPreSerializedList } from '@opentrons/api-client'
import { useHost } from '../api'
import type { UseQueryOptions } from 'react-query'
import type {
GetCommandsParams,
HostConfig,
CommandsData,
RunCommandSummary,
} from '@opentrons/api-client'

const DEFAULT_PAGE_LENGTH = 30
export const DEFAULT_PARAMS: GetCommandsParams = {
cursor: null,
pageLength: DEFAULT_PAGE_LENGTH,
}

export function useAllCommandsAsPreSerializedList<TError = Error>(
runId: string | null,
params?: GetCommandsParams | null,
options: UseQueryOptions<CommandsData, TError> = {}
): UseQueryResult<CommandsData, TError> {
const host = useHost()
const nullCheckedParams = params ?? DEFAULT_PARAMS

const allOptions: UseQueryOptions<CommandsData, TError> = {
...options,
enabled: host !== null && runId != null && options.enabled !== false,
}
const { cursor, pageLength } = nullCheckedParams
const query = useQuery<CommandsData, TError>(
[host, 'runs', runId, 'getCommandsAsPreSerializedList', cursor, pageLength],
() => {
return getCommandsAsPreSerializedList(
host as HostConfig,
runId as string,
nullCheckedParams
).then(response => {
const responseData = response.data
return {
...responseData,
data: responseData.data.map(
command => JSON.parse(command) as RunCommandSummary
),
}
})
},
allOptions
)

return query
}
3 changes: 3 additions & 0 deletions robot-server/robot_server/runs/router/actions_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
MaintenanceEngineStore,
)
from robot_server.maintenance_runs.dependencies import get_maintenance_engine_store
from robot_server.service.notifications import get_runs_publisher, RunsPublisher

log = logging.getLogger(__name__)
actions_router = APIRouter()
Expand All @@ -45,6 +46,7 @@ async def get_run_controller(
task_runner: TaskRunner = Depends(get_task_runner),
engine_store: EngineStore = Depends(get_engine_store),
run_store: RunStore = Depends(get_run_store),
runs_publisher: RunsPublisher = Depends(get_runs_publisher),
) -> RunController:
"""Get a RunController for the current run.

Expand All @@ -67,6 +69,7 @@ async def get_run_controller(
task_runner=task_runner,
engine_store=engine_store,
run_store=run_store,
runs_publisher=runs_publisher,
)


Expand Down
63 changes: 62 additions & 1 deletion robot-server/robot_server/runs/router/commands_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from anyio import move_on_after
from fastapi import APIRouter, Depends, Query, status

from pydantic import BaseModel, Field

from opentrons.protocol_engine import (
Expand All @@ -21,11 +22,12 @@
MultiBody,
MultiBodyMeta,
PydanticResponse,
SimpleMultiBody,
)
from robot_server.robot.control.dependencies import require_estop_in_good_state

from ..run_models import RunCommandSummary
from ..run_data_manager import RunDataManager
from ..run_data_manager import RunDataManager, PreSerializedCommandsNotAvailableError
from ..engine_store import EngineStore
from ..run_store import RunStore, CommandNotFoundError
from ..run_models import RunNotFoundError
Expand Down Expand Up @@ -70,6 +72,18 @@ class CommandNotAllowed(ErrorDetails):
title: str = "Command Not Allowed"


class PreSerializedCommandsNotAvailable(ErrorDetails):
"""An error if one tries to fetch pre-serialized commands before they are written to the database."""

id: Literal[
"PreSerializedCommandsNotAvailable"
] = "PreSerializedCommandsNotAvailable"
title: str = "Pre-Serialized commands not available."
detail: str = (
"Pre-serialized commands are only available once a run has finished running."
)


class CommandLinkMeta(BaseModel):
"""Metadata about a command resource referenced in `links`."""

Expand Down Expand Up @@ -351,6 +365,53 @@ async def get_run_commands(
)


@PydanticResponse.wrap_route(
commands_router.get,
path="/runs/{runId}/commandsAsPreSerializedList",
Copy link
Contributor

@SyntaxColoring SyntaxColoring May 1, 2024

Choose a reason for hiding this comment

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

Is there a way to do this that doesn't require:

  • A separate endpoint path?
  • New 503 semantics for runs that haven't completed?

i.e. could we make this something like GET /runs/{id}/commands?validate=false instead?

I know we're repeating how we did things with GET /protocols/analyses/{id}/asDocument, but I think it was a mistake for that endpoint to work like that, in hindsight.

Unifying them would have integration testing benefits. Imagine just parametrizing every Tavern test with ?validate=true/?validate=false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to do this that doesn't require:
A separate endpoint path?

I think the separate endpoint path is the right thing to do here because the shape/type of the response is different from the regular ../commands endpoint

New 503 semantics for runs that haven't completed?

Talked about this briefly on the call and posting here for posterity- Not having a 503 for runs can be two things-

  • returning an empty list in the response: this is misleading and can cause confusion as to whether this run just didn't have any commands or whether the commands just haven't been serialized yet. We agreed that this is not a good option
  • serializing commands from memory (since it's a current run) and returning them as response: this is a good option but this too takes a non-ideal amount of processing time to respond with. I will test for exactly how much time and respond here. If it's faster/ takes similar time as fetching pre-serialized commands, then I'll replace the 503 with it. If it takes more than twice the amount it would to fetch pre-serialized commands, then I think it would be more disruptive to the client flow (and user experience) if the client hits the endpoint at the wrong time and ends up having to wait a long time and blocking rest of the flow.

Copy link
Contributor

@SyntaxColoring SyntaxColoring May 1, 2024

Choose a reason for hiding this comment

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

I think the separate endpoint path is the right thing to do here because the shape/type of the response is different from the regular ../commands endpoint

Definitely, if the response shape is different. If possible, I'm saying the response shape should not be different. (Discussed in the other thread: #15031 (comment))

If it takes more than twice the amount it would to fetch pre-serialized commands, then I think it would be more disruptive to the client flow (and user experience) if the client hits the endpoint at the wrong time and ends up having to wait a long time and blocking rest of the flow.

I'm not totally following this. How does returning 503 avoid the client having to wait a long time and avoid blocking the rest of the flow? Either way, the client can only move on after the server has spent time serializing everything.

summary="Get all commands of a completed run as a list of pre-serialized commands",
description=(
"Get all commands of a completed run as a list of pre-serialized commands."
"**Warning:** This endpoint is experimental. We may change or remove it without warning."
"\n\n"
"The commands list will only be available after a run has completed"
" (whether successful, failed or stopped) and its data has been committed to the database."
" If a request is received before the run is completed, it will return a 503 Unavailable error."
" This is a faster alternative to fetching the full commands list using"
" `GET /runs/{runId}/commands`. For large protocols (10k+ commands), the above"
" endpoint can take minutes to respond, whereas this one should only take a few seconds."
),
responses={
status.HTTP_404_NOT_FOUND: {"model": ErrorBody[RunNotFound]},
status.HTTP_503_SERVICE_UNAVAILABLE: {
"model": ErrorBody[PreSerializedCommandsNotAvailable]
},
},
)
async def get_run_commands_as_pre_serialized_list(
runId: str,
run_data_manager: RunDataManager = Depends(get_run_data_manager),
) -> PydanticResponse[SimpleMultiBody[str]]:
Copy link
Member

Choose a reason for hiding this comment

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

Since the fundamental idea with this endpoint is to avoid server work by not doing json serialization, what if this endpoint's response was in the form of jsonlines, json object records (aka the json-formatted Command instances we get from the database) that are line-separated? I think this is a good idea for two reasons:

  1. It requires less server work. With a SimpleMultiBody like this, we still have to do the work of built in json module encoding all those commands into a json array - things like escaping all the internal quotes
  2. It generalizes better (IMO) to queries for running runs, since it's not totally contained and therefore doesn't really have "this is a complete view" semantics

If we're concerned about breaking similarity with the other elements of our api by not being json, one thing we can do is let the client specify Accept for either application/json (MultiBody response) or application/jsonlines

Copy link
Contributor

@SyntaxColoring SyntaxColoring May 1, 2024

Choose a reason for hiding this comment

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

We can get advantage 1 without giving up our usual response format, right? JSON lines would look basically this:

response_body = "\n".join(serialized_commands)

But this would be equally fast:

response_body = '{"data":[' + ','.join(serialized_commands) + ']}'

And I'm sure there's a way to write a custom JSON encoder to do it if concatenating JSON fragments like that feels too icky. [Edit: Nope, standard library JSON encoders can't do this.]

Could you clarify your point 2? I'm not sure what you mean by "contained" or how that affects generalization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting idea. I've added a todo comment to investigate this option further.

"""Get all commands of a completed run as a list of pre-serialized (string encoded) commands.

Arguments:
runId: Requested run ID, from the URL
run_data_manager: Run data retrieval interface.
"""
try:
commands = run_data_manager.get_all_commands_as_preserialized_list(runId)
except RunNotFoundError as e:
raise RunNotFound.from_exc(e).as_error(status.HTTP_404_NOT_FOUND) from e
except PreSerializedCommandsNotAvailableError as e:
raise PreSerializedCommandsNotAvailable.from_exc(e).as_error(
status.HTTP_503_SERVICE_UNAVAILABLE
) from e
return await PydanticResponse.create(
content=SimpleMultiBody.construct(
data=commands, meta=MultiBodyMeta(cursor=0, totalLength=len(commands))
)
)


@PydanticResponse.wrap_route(
commands_router.get,
path="/runs/{runId}/commands/{commandId}",
Expand Down
9 changes: 8 additions & 1 deletion robot-server/robot_server/runs/run_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

from opentrons.protocol_engine.types import DeckConfigurationType

from robot_server.service.notifications import RunsPublisher

log = logging.getLogger(__name__)


Expand All @@ -21,19 +23,21 @@ class RunActionNotAllowedError(RoboticsInteractionError):


class RunController:
"""An interface to manage the side-effects of requested run actions."""
"""An interface to manage the side effects of requested run actions."""

def __init__(
self,
run_id: str,
task_runner: TaskRunner,
engine_store: EngineStore,
run_store: RunStore,
runs_publisher: RunsPublisher,
) -> None:
self._run_id = run_id
self._task_runner = task_runner
self._engine_store = engine_store
self._run_store = run_store
self._runs_publisher = runs_publisher

def create_action(
self,
Expand Down Expand Up @@ -108,3 +112,6 @@ async def _run_protocol_and_insert_result(
commands=result.commands,
run_time_parameters=result.parameters,
)
await self._runs_publisher.publish_pre_serialized_commands_notification(
self._run_id
)
Loading
Loading