fix(api): Filter out air_gap() calls as higher-order commands
#14985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sanni-t
approved these changes
Apr 23, 2024
| assert isinstance(load_well_plate, commands.LoadLabware) | ||
| assert isinstance(load_pipette, commands.LoadPipette) | ||
| assert isinstance(pick_up_tip, commands.PickUpTip) | ||
| # TODO(mm, 2024-04-23): This commands.Custom looks wrong. This should be a commands.MoveToWell. |
There was a problem hiding this comment.
👀
Might be worth ticketing out if it looks like another bug.
y3rsh
added a commit
that referenced
this pull request
Apr 23, 2024
* edge: (194 commits) fix(app): clone run with RTPs from HistoricalProtocolRun (#14959) fix(api): Filter out `air_gap()` calls as higher-order commands (#14985) fix(app): fix infinitely re-rendering/never rendering firmware success toasts (#14981) feat(api): add option to ignore different tip presence states (#14980) feat(opentrons-ai-client) add input textbox to container (#14968) fix(app): add robotSerialNumber to proceedToRun event (#14976) fix(api): remove homing patch fix for right mount when a 96-channel is attached (#14975) feat(api-client,app,react-api-client): upload splash logo from desktop app (#14941) fix(robot-server): notify /runs when a non-current run is deleted (#14974) feature(api, robot-server): Allow fixit commands to recover from an error (#14908) feat(hardware-testing): enable multi sensor processing in liquid probe (#14883) fix(app): prevent "run again" banner from rendering after navigating away from the current run (#14973) refactor(components): refactor roundtab stories (#14956) refactor(protocol-designer): assign module slot in createFileWizard instead of modal (#14951) fix(app, api-client): fix choose protocol slideout issue (#14949) refactor(protocol-designer): tip position modal max values round down (#14972) feat(app): add tiprack selection step to quick transfer flow (#14950) ci(shared-data): install dependencies in workflow (#14958) fix(components): fix icon stories (#14969) feat(opentrons-ai-client): introduce react-markdown to chat display component (#14965) ...
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This fixes RQA-2621.
Test Plan
This PR adds unit tests and a "mid-level" integration test. @y3rsh mentioned he's working on adding an analysis snapshot test.
Details
Before ~v5.0, Python protocols reported their activity as a nested tree, roughly like this:
But the Protocol-Engine–based HTTP API introduced in v5.0 could only represent a flat list of the innermost commands. So our mapping logic in
LegacyCommandMapperhad to filter out the "higher-order" things like transfers and mixes, leaving only the "atomic" things like aspirates and dispenses.We apparently forgot to filter out
air_gap(), which looks like this:The effect of forgetting to filter it out was that the engine would have two commands running at the same time: the outer air gap and the inner aspirate. This was a dormant bug; Protocol Engine is certainly not meant for that to work.
In #14726, we started being stricter about things like not having more than one command running at a time, so this started to raise an
AssertionError(correctly, but confusingly).The fix is just to filter out air gaps, like we filter out transfers, mixes, etc.
Review requests
We should be on the lookout for any other higher-order commands that aren't being filtered out properly.
We should think about ways to make this inherently safer. Like, instead of mapping every command by default and filtering out the ones that we know to be higher-order, we might want to omit commands by default and only map the ones that we know to be at the deepest level of nesting. Or, we might want to defensively code
LegacyCommandMapperto automatically close the last command before starting a new one.Risk assessment
Low risk to the fix, but there's definitely some risk that we're missing similar bugs.