refactor(robot-server): add PE commands to /sessions responses #8006
Conversation
mcous
added
robot-svcs
Codecov Report
@@ Coverage Diff @@
## edge #8006 +/- ##
==========================================
- Coverage 85.93% 85.89% -0.05%
==========================================
Files 351 352 +1
Lines 21351 21418 +67
==========================================
+ Hits 18347 18396 +49
- Misses 3004 3022 +18
Continue to review full report at Codecov.
|
| @@ -56,7 +56,7 @@ def add_command(self, request: CommandRequest) -> Command: | |||
|
|
|||
| async def execute_command_by_id(self, command_id: str) -> Command: | |||
| """Execute a protocol engine command by its identifier.""" | |||
| queued_command = self.state_view.commands.get_command_by_id(command_id) | |||
| queued_command = self.state_view.commands.get(command_id) | |||
There was a problem hiding this comment.
In usage, state_view.commands.get_command_by_id felt quite excessive. In line with similar feedback in the past, I renamed the methods of the CommandView to remove command, given that they're all accessed via state_view.commands:
| before | after |
|---|---|
commands.get_command_by_id |
commands.get |
commands.get_all_commands |
commands.get_all |
commands.get_next_command |
commands.get_next_queued |
There was a problem hiding this comment.
This feels much better to me.
If we wanted to go even further, how would we feel about replacing commands.get(id) with commands[id]?
There was a problem hiding this comment.
I'm anti-bracket-notation for this interface. Supporting bracket notation would bring in magic (methods) that just aren't needed and don't (IMO) meaningfully improve the DX of this class.
- It would make testing harder (or at least inconsistent, where we can no longer do a simple function mock)
- It makes it less obvious that
commands.get(id)may be a computed value- If, for example, different parts of a given
Commandwere stored in different places, andgetpulled them all together - This isn't outside the realm of possibility
- If, for example, different parts of a given
- You can already do
command_state.commands_by_id[id]
| @@ -22,7 +22,7 @@ class ResourceModel(ResponseDataModel): | |||
| id: str = Field(..., description="Unique identifier of the resource.") | |||
|
|
|||
|
|
|||
| ResponseDataT = TypeVar("ResponseDataT", bound=ResponseDataModel) | |||
| ResponseDataT = TypeVar("ResponseDataT", bound=BaseModel) | |||
There was a problem hiding this comment.
Went down a bit of a rabbit hole on this one!
- This TypeVar is used by the generic
ResponseModelto say that itsdatafield must be aResponseDataModel ResponseDataModelis a class in therobot_serverpackage that is aBaseModelwith anidfield defined- We want to put
protocol_engine.Commandmodels in HTTPResponseModelsprotocol_engine.Commandis aBaseModelwith an ID field defined- However, it cannot inherit from
robot_server.ResponseDataModelbecause it's in theopentronspackage (and that would introduce a circular dependency)
So, I saw two options available:
- Move the responsibility of
ResponseDataModelinto theopentronspackage as some basic resource base class - Downgrade this to a
BaseModeland rely on server conventions to ensure all responses have IDs
I went with (2) because it was just way easier. I also explored a third option:
- Create a resource
Protocoland set thisTypeVarto say thatdatamust be bothResourceLike(an object with anidproperty) and aBaseModel
Option 3 doesn't work because of fundamental limitations in the Python typing system:
- There's no
Intersectiontype to sayTypeA & TypeB - You can't create a
Protocolfrom an actual class
Longer term (assuming Option 3 never pans out), I can see us doing a version of option 1, especially if discussions continue on some sort of OpentronsBaseModel
| }, | ||
| ) | ||
| async def get_session_commands( | ||
| session: ResponseModel[Session] = Depends(get_session), |
There was a problem hiding this comment.
I can't decide if this is great or awful, but turns out, you can throw an entire route handler into Depends to compose routes. Here (and in the route below) it allows us to avoid repeating ourselves with SessionNotFound responses.
If this bothers you, I'm happy to revert and make this more verbose / repeat ourselves a little
There was a problem hiding this comment.
Off the top of my head, I do not understand this, but I'll look at it again tomorrow morning when I'm smarter.
There was a problem hiding this comment.
Aha okay I get it now.
Yeah, uh, I also can't tell if this is great or awful, lol. I don't hate it!
Would it feel better if, instead of this:
get_session_commands()
|
V
get_session()
It were this?
get_session_commands() get_session()
\ /
V V
_internal_get_session()
I don't mind it staying as-is for this PR, if you want to merge today.
| ResponseModel[ProtocolSession], | ||
| ] | ||
|
|
||
| SessionCommandResponse = Union[ |
There was a problem hiding this comment.
This is tedious, but it's also really important if we want our OpenAPI Spec to be reasonable. The difference between...
ResponseModel[Union[CommandA, CommandB, ...]]Union[ResponseModel[CommandA], ResponseModel[CommandB], ...]]
...is huge in terms of the generated spec, even though they're mathematically identical. The OAS from option (1) is unusable, wheras (2) generates a nice set of reasonable types in the schema.
If we had known about this when we were evaluating response shapes, I think this would've been a huge 👎 against going with JSON:API-style responses (i.e. { data: ... }. Unfortunately, that ship's kind of sailed, so I think we're stuck with this for the time being
There was a problem hiding this comment.
Yeah. I don't know if this is the same problem you're talking about, but this is basically how I imagined we'd fix our possible links values not being specified in the OpenAPI spec.
| client: TestClient, | ||
| ) -> None: | ||
| """It should return a list of all commands in a session.""" | ||
| session = SessionResource( |
There was a problem hiding this comment.
Could probably DRY these command route tests up a little. These stubs are common between all or most of the 200 tests
Edit: on second thought, explicit (with duplication) is probably fine for now
mcous
force-pushed
the
robot-server_session-commands
branch
from
a186775 to
ad7d37d
Compare
| return self._engine.state_view.commands.get_next_command() | ||
| return self._engine.state_view.commands.get_next_queued() |
| @@ -56,7 +56,7 @@ def add_command(self, request: CommandRequest) -> Command: | |||
|
|
|||
| async def execute_command_by_id(self, command_id: str) -> Command: | |||
| """Execute a protocol engine command by its identifier.""" | |||
| queued_command = self.state_view.commands.get_command_by_id(command_id) | |||
| queued_command = self.state_view.commands.get(command_id) | |||
There was a problem hiding this comment.
This feels much better to me.
If we wanted to go even further, how would we feel about replacing commands.get(id) with commands[id]?
| # TODO(mc, 2021-06-23): mypy >= 0.780 broke Unions as `response_model` | ||
| # see https://github.com/tiangolo/fastapi/issues/2279 | ||
| response_model=SessionResponse, # type: ignore[arg-type] |
There was a problem hiding this comment.
Not for this PR, but maybe we should in general just not use response_model, and instead do:
responses={
status.HTTP_200_SUCCESS: {"model": Union[Success1, Success2]}, # I'm assuming this works.
status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[ProtocolNotFound]},
status.HTTP_409_CONFLICT: {"model": ErrorResponse[SessionAlreadyActive]},
},response_model always weirded me out anyway: why is there one special success response? Especially given our error handling strategy of not raiseing HTTP errors from anything but the top level.
response_model's use seems to be automatically converting the function's return value to the given class. But we don't do that—we always return the exact, explicit model class that we want. e.g. we return Foo(bar="baz") instead of {"bar": "baz"}.
There was a problem hiding this comment.
I've played around with this, and I walked away having experienced a bad time. It messes with response validation and possibly also the OpenAPI Spec. Better to work on pulling in InferringRouter instead I think
| }, | ||
| ) | ||
| async def get_session_commands( | ||
| session: ResponseModel[Session] = Depends(get_session), |
There was a problem hiding this comment.
Off the top of my head, I do not understand this, but I'll look at it again tomorrow morning when I'm smarter.
| ResponseModel[ProtocolSession], | ||
| ] | ||
|
|
||
| SessionCommandResponse = Union[ |
There was a problem hiding this comment.
Yeah. I don't know if this is the same problem you're talking about, but this is basically how I imagined we'd fix our possible links values not being specified in the OpenAPI spec.
| command_summaries = [ | ||
| SessionCommandSummary(id=c.id, commandType=c.commandType, status=c.status) | ||
| for c in commands | ||
| ] |
There was a problem hiding this comment.
If we basically stick with this summarization scheme, we might want something like [c.to_summary() for c in commands].
| client: TestClient, | ||
| ) -> None: | ||
| """It should return an empty collection response with no sessions exist.""" | ||
| """It should a collection response when a session exists.""" | ||
| # TODO(mc, 2021-06-23): add actual multi-session support |
There was a problem hiding this comment.
This TODO seems bigger than this particular test function. Should we make it a ticket instead?
There was a problem hiding this comment.
We don't have any user stories for this yet, so I think a TODO is appropriate
| @@ -106,7 +118,8 @@ async def create_session( | |||
| raise SessionAlreadyActive(detail=str(e)).as_error(status.HTTP_409_CONFLICT) | |||
|
|
|||
| session_store.upsert(session=session) | |||
| data = session_view.as_response(session=session) | |||
| commands = engine_store.engine.state_view.commands.get_all() | |||
There was a problem hiding this comment.
That's interesting.. How about summarization + pagination? The summarization makes it readable while pagination would help with the 'unbounded commands' problem.
Co-authored-by: Max Marrone <[email protected]>
[Edit: Tracking this problem separately in #8024.] There's some weirdness happening to our enums. Here's a snippet from our generated OpenAPI spec: "SessionCommandSummary": {
"title": "SessionCommandSummary",
"required": [
"id",
"commandType",
"status"
],
"type": "object",
"properties": {
"id": {
"title": "Id",
"type": "string",
"description": "Unique command identifier."
},
"commandType": {
"title": "Commandtype",
"anyOf": [
{
"const": "addLabwareDefinition",
"type": "string"
},
{
"const": "aspirate",
"type": "string"
},
...The
I don't know if this is a |
|
Postman testing worked.
|
Co-authored-by: Max Marrone <[email protected]>
Oof. Fingers crossed this is a redoc thing. Maybe time to experiment with a FastAPI + Pydantic upgrade. Merging for now, but lets keep an close eye on this. |
Overview
This PR builds on the models added to the ProtocolEngine by #7993 and adds ProtocolEngine commands to the (currently feature-flagged)
/sessionsendpoints. It also adds two new endpoints:GET /sessions/:session_id/commandsGET /sessions/:session_id/commands/:command_idCloses #7871.
Changelog
SessionCommandSummarymodels toGET /sessionsandGET /sessions/:session_idGET /sessions/:session_id/commandsendpoint that returnsSessionCommandSummarymodelsGET /sessions/:session_id/commands/:command_idendpoint that returns fullprotocol_engine.CommandmodelsIn real world testing, serializing all the commands of even a simple JSON protocol in full took a long time (1.5s response time on an OT-2). So, after disucssions in Slack, I decided to create a minimal
SessionCommandSummarymodel for the collection endpoints. We should expect to revisit this decision as development progresses, but for now I think it's a reasonable compromise.Review requests
Smoke test plan
I've updated the Postman sessions testing collection with:
Simple Test Protocol.json)command_idvariable to the first command in thePOST /sessionsresponse, if presentUsing this collection, this is the test smoke test procedure I've been running on the dev server and a real robot:
Simple Test Protocol.jsonusing aPOST /protocolsrequestPOST /sessions { sessionType: protocol }requestdata.commandsGET /sessions/:session_id/commandsrequestdataGET /sessions/:session_id/commands/:command_idrequestdataRisk assessment
N/A; work is entirely feature flagged