-
Notifications
You must be signed in to change notification settings - Fork 180
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
Changes from 11 commits
d11cd3c
1f85bc1
8248e9a
c943dd1
ab89d5b
763cd91
b3138d8
f56ac48
d02bf45
eb5239f
8e7f69d
8dc0dd8
4a0aa9b
d00e1bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
) | ||
} |
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 | ||
} |
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
|
@@ -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 | ||
|
@@ -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`.""" | ||
|
||
|
@@ -351,6 +365,53 @@ async def get_run_commands( | |
) | ||
|
||
|
||
@PydanticResponse.wrap_route( | ||
commands_router.get, | ||
path="/runs/{runId}/commandsAsPreSerializedList", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to do this that doesn't require:
i.e. could we make this something like I know we're repeating how we did things with Unifying them would have integration testing benefits. Imagine just parametrizing every Tavern test with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Talked about this briefly on the call and posting here for posterity- Not having a 503 for runs can be two things-
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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))
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]]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) + ']}'
Could you clarify your point 2? I'm not sure what you mean by "contained" or how that affects generalization. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}", | ||
|
There was a problem hiding this comment.
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
andcacheTime
anymore now that we're using the notification service right?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!