refactor(protocol-engine): rework command models for HTTP API schema #7993
Conversation
mcous
added
api
Codecov Report
@@ Coverage Diff @@
## edge #7993 +/- ##
==========================================
+ Coverage 85.88% 85.95% +0.07%
==========================================
Files 348 351 +3
Lines 21254 21384 +130
==========================================
+ Hits 18253 18380 +127
- Misses 3001 3004 +3
Continue to review full report at Codecov.
|
mcous
added
the
robot-svcs
| # return to top if labware is fixed trash | ||
| if labware_id == FIXED_TRASH_ID: | ||
| # return to top if labware is fixed trash= | ||
| is_fixed_trash = self._labware.get_labware_has_quirk( |
There was a problem hiding this comment.
Oh oops, this is the fix I said I wasn't going to include. Accidentally committed; I think I wanna leave it in because I'm feeling lazy and it's a very small fix
api/src/opentrons/protocol_engine/commands/add_labware_definition.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Sanniti Pimpley <[email protected]>
There was a problem hiding this comment.
Partial review. What I've read so far looks good! Just some small questions and comments. I'm up to api/src/opentrons/protocol_engine/execution/command_executor.py.
I only skimmed the drop_tip.py, load_labware.py, etc. files because their changes seemed rote.
| _ImplementationCls: Type[ | ||
| AddLabwareDefinitionImplementation | ||
| ] = AddLabwareDefinitionImplementation |
There was a problem hiding this comment.
If we wanted to avoid a magically-named _ImplementationCls attribute, we could do something like this:
@command_mapper.map_to_implementation(AddLabwareDefinitionImplementation)
class AddLabwareDefinition(...)
# ...
# _ImplementationCls automatically added by class decorator.This would make the link to the command_mapper module explicit.
I'm totally fine with it staying as-is for the sake of simplicity, though.
There was a problem hiding this comment.
I think that's a good idea to keep in the back pocket. For now, something like this would introduce a circular dependency between protocol_engine.commands and protocol_engine.execution that would need untangling.
What I like about this, though, is that I think the AbstractCommandImplementation base class should be coming in from protocol_engine.execution, rather than its current home of protocol_engine.commands. So I think it's worthwhile to untangle this and figure out a more explicit connection
Co-authored-by: Max Marrone <[email protected]>
api/tests/opentrons/protocol_engine/commands/test_command_mapper.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/commands/test_command_mapper.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/commands/test_load_pipette.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/commands/test_move_to_well.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/commands/test_pick_up_tip.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Marrone <[email protected]> Co-authored-by: Sanniti Pimpley <[email protected]>
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_state_store() -> MagicMock: |
There was a problem hiding this comment.
What was the reason for moving some of these frequently used fixtures out of conftest and adding them to individual test files?
There was a problem hiding this comment.
This particular (MagicMock) fixture was only used in one test, so I moved it there.
In general, though, I'm finding conftest fixtures kind of hard to follow a lot of the time, and some tests become a lot more readable even if the fixtures are duplicated
There was a problem hiding this comment.
Yeah, conftest feels too implicit a lot of the time, for me. Especially when there are multiple levels of conftests.
I often think about turning fixtures into just regular functions that we import from some test_utils module. That would solve the implicitness problem.
Overview
This PR is precursor work to #7870 and #7871.
Since the Protocol Engine is the primary processor and state-tracker of protocol commands, we want the models that it uses to be broadly compatible with the HTTP API. This PR reworks all the Protocol Engine's commands to (mostly) match the Command model from existing sessions.
This is a massive PR because it turns out, there was a lot of not-single-responsibility stuff about commands in the Protocol Engine. This PR, by necessity, attempts to sequester some of those responsibilities to prevent this sort of hellish churn in the future.
Changelog
Review requests
This is awful and I'm sorry. When in doubt, fire-drill me in Slack for an in-person call to resolve stuff.
Important smoke tests
I've run the following sessions to completion on my robot on commit b011d29:
liveProtocolare working with theenableProtocolEnginefeature flag disabledcalibrationChecktipLengthCalibrationdeckCalibrationpipetteOffsetCalibrationprotocolenableProtocolEnginefeature flag enabledUnrelated issues found during testing
Not going to address any of these in this PR, but noting them to make sure we don't lose them:
Protocol engine does not support drop tip in fixed trash from JSON protocolfixedTrashquirk rather than looking for the hardcodedFIXED_TRASH_IDRisk assessment
Medium. This PR gets close to existing sessions endpoints by necessity. It (intentionally) breaks the non-production
liveProtocolsessions in favor of the upcoming feature-flaggedbasicsession. Also note thatliveProtocolsessions are not the same thing as theprotocolsessions documented in the http-api-beta. These protocol-file based sessions should continue to function.Mitigation steps: