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.

Sign up for GitHub

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

refactor(api): Make sure command implementations return something compatible with the command's result type #15051

Merged
merged 3 commits into from
Apr 30, 2024
Merged

refactor(api): Make sure command implementations return something compatible with the command's result type #15051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62c3c57
Enforce that a command's implementation returns a type compatible wit…
SyntaxColoring Apr 30, 2024
5f810b8
Fix up tests.
SyntaxColoring Apr 30, 2024
c965b37
Shorten TypeVar names and mark them as private.
SyntaxColoring Apr 30, 2024
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
37 changes: 22 additions & 15 deletions api/src/opentrons/protocol_engine/commands/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
TypeVar,
Tuple,
List,
Type,
Union,
)

from pydantic import BaseModel, Field
Expand All @@ -29,11 +31,11 @@
from ..state import StateView


CommandParamsT = TypeVar("CommandParamsT", bound=BaseModel)

CommandResultT = TypeVar("CommandResultT", bound=BaseModel)

CommandPrivateResultT = TypeVar("CommandPrivateResultT")
_ParamsT = TypeVar("_ParamsT", bound=BaseModel)
_ParamsT_contra = TypeVar("_ParamsT_contra", bound=BaseModel, contravariant=True)
_ResultT = TypeVar("_ResultT", bound=BaseModel)
_ResultT_co = TypeVar("_ResultT_co", bound=BaseModel, covariant=True)
_PrivateResultT_co = TypeVar("_PrivateResultT_co", covariant=True)


class CommandStatus(str, Enum):
Expand All @@ -58,7 +60,7 @@ class CommandIntent(str, Enum):
FIXIT = "fixit"


class BaseCommandCreate(GenericModel, Generic[CommandParamsT]):
class BaseCommandCreate(GenericModel, Generic[_ParamsT]):
"""Base class for command creation requests.

You shouldn't use this class directly; instead, use or define
Expand All @@ -72,7 +74,7 @@ class BaseCommandCreate(GenericModel, Generic[CommandParamsT]):
"execution behavior"
),
)
params: CommandParamsT = Field(..., description="Command execution data payload")
params: _ParamsT = Field(..., description="Command execution data payload")
intent: Optional[CommandIntent] = Field(
None,
description=(
Expand All @@ -97,7 +99,7 @@ class BaseCommandCreate(GenericModel, Generic[CommandParamsT]):
)


class BaseCommand(GenericModel, Generic[CommandParamsT, CommandResultT]):
class BaseCommand(GenericModel, Generic[_ParamsT, _ResultT]):
"""Base command model.

You shouldn't use this class directly; instead, use or define
Expand Down Expand Up @@ -127,8 +129,8 @@ class BaseCommand(GenericModel, Generic[CommandParamsT, CommandResultT]):
),
)
status: CommandStatus = Field(..., description="Command execution status")
params: CommandParamsT = Field(..., description="Command execution data payload")
result: Optional[CommandResultT] = Field(
params: _ParamsT = Field(..., description="Command execution data payload")
result: Optional[_ResultT] = Field(
None,
description="Command execution result data, if succeeded",
)
Expand Down Expand Up @@ -167,10 +169,15 @@ class BaseCommand(GenericModel, Generic[CommandParamsT, CommandResultT]):
),
)

_ImplementationCls: Union[
Type[AbstractCommandImpl[_ParamsT, _ResultT]],
Type[AbstractCommandWithPrivateResultImpl[_ParamsT, _ResultT, object]],
]

Comment on lines +172 to +176
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By convention, each command links itself to its implementation via an _ImplementationCls class attribute.

Here, I'm declaring that attribute on the base class. This enforces that each command's ._ImplementationCls matches its .params and .result. In the near future, this will also make sure it matches its .error.

Comment on lines +172 to +176
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're using VS Code or another editor with Pyright integration, you'll get a red squiggly on every command's override of _ImplementationCls. (Mypy does not complain.)

I think it has to do with the fact that _ImplementationCls looks writable, so it's unsound for a subclass to have a more restrictive type than the base class. It looks like commandType has this problem, too. I don't have a solution right now. Maybe it'll get easier with Pydantic v2.


class AbstractCommandImpl(
ABC,
Generic[CommandParamsT, CommandResultT],
Generic[_ParamsT_contra, _ResultT_co],
Comment on lines -173 to +180
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In English:

  • An implementation that accepts a broader parameter type can be placed in a variable where it's annotated to accept a narrower parameter type. It's contravariant in its parameters.
  • An implementation that returns a narrower result type can be placed in a variable where it's annotated to return a broader parameter type. It's covariant in its result.

This is the usual behavior for callables, which is basically what a command implementation is. I think we need to spell it out manually because we're going through a wrapper class; the type-checker would understand this automatically if, e.g., the AspirateImplementation.execute() method were just a free-standing execute() function.

):
"""Abstract command creation and execution implementation.

Expand Down Expand Up @@ -204,14 +211,14 @@ def __init__(
pass

@abstractmethod
async def execute(self, params: CommandParamsT) -> CommandResultT:
async def execute(self, params: _ParamsT_contra) -> _ResultT_co:
"""Execute the command, mapping data from execution into a response model."""
...


class AbstractCommandWithPrivateResultImpl(
ABC,
Generic[CommandParamsT, CommandResultT, CommandPrivateResultT],
Generic[_ParamsT_contra, _ResultT_co, _PrivateResultT_co],
):
"""Abstract command creation and execution implementation if the command has private results.

Expand Down Expand Up @@ -247,7 +254,7 @@ def __init__(

@abstractmethod
async def execute(
self, params: CommandParamsT
) -> Tuple[CommandResultT, CommandPrivateResultT]:
self, params: _ParamsT_contra
) -> Tuple[_ResultT_co, _PrivateResultT_co]:
"""Execute the command, mapping data from execution into a response model."""
...
8 changes: 2 additions & 6 deletions api/tests/opentrons/protocol_engine/execution/test_command_executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,7 @@ class _TestCommand(BaseCommand[_TestCommandParams, _TestCommandResult]):
params: _TestCommandParams
result: Optional[_TestCommandResult]

@property
def _ImplementationCls(self) -> Type[_TestCommandImpl]:
return TestCommandImplCls
_ImplementationCls: Type[_TestCommandImpl] = TestCommandImplCls

command_params = _TestCommandParams()
command_result = _TestCommandResult()
Expand Down Expand Up @@ -407,9 +405,7 @@ class _TestCommand(BaseCommand[_TestCommandParams, _TestCommandResult]):
params: _TestCommandParams
result: Optional[_TestCommandResult]

@property
def _ImplementationCls(self) -> Type[_TestCommandImpl]:
return TestCommandImplCls
_ImplementationCls: Type[_TestCommandImpl] = TestCommandImplCls

command_params = _TestCommandParams()

Expand Down
Loading