fix(api): add PAPI loaded trashes to engine on load statement

[jbleon95]

Overview

This PR fixes an issue that was introduced in 7.1, where in when the trash bin or waste chutes were loaded in PAPI with load_trash_bin or load_waste_chute, they were not added to the engine store (and therefore undergoing conflict checks and being tracked by Protocol Engine), until they were first referenced, usually via a drop tip operation.

This was a known issue and generally was not a problem as the conflicts that could occur were positional/configuration based, and so it didn't matter if they bubbled up on a later line, even though it could create a misleading line number for the error. However, with new partial tip pickup checks being introduced in 7.2 that require knowing about adjacent trash bins or waste chutes, now it is entirely possible and not unlikely to do a dangerous move before dropping tips. And if this were to be the only dangerous move, the protocol would pass analysis when it should not.

This fixes this issue by adding a new protocol engine action and hooking that all the way up to the sync client, allowing us to add the trash bin or waste chute object to engine when the user declares they are loading one. This also requires us to do a check beforehand to raise any potential conflict, since those checks must happen in the view and not the store.

Test Plan

Tested this protocol to ensure that it raised now on the expected line.

metadata = {
    'protocolName': 'Trash Bin Load Expected Failure',
}

requirements = {
    "robotType": "Flex",
    "apiLevel": "2.16"
}


def run(context):
    tiprack1 = context.load_labware('opentrons_flex_96_tiprack_50ul', 'A1')
    tiprack2 = context.load_labware('opentrons_flex_96_tiprack_50ul', 'A4')

    trash_b3 = context.load_trash_bin('B3')

    # This should fail here, on line 18, because of the staging slot
    trash_a3 = context.load_trash_bin('A3')
    pipette = context.load_instrument('flex_1channel_1000', 'left', tip_racks=[tiprack1])

    pipette.pick_up_tip()

    # Not here, on line 24
    pipette.drop_tip()

Changelog

• add waste chute and trash bin addressable areas to addressable area store immediately upon a load_trash_bin or load_waste_chute

Review requests

It was brought up that it might make sense to gate these changes behind an API level bump. I've not done so yet since most of these errors are now just being raised earlier in the protocol, and the newly introduced ones are coming in this version. Depending on how we are introducing the partial tip pickup checks/changes, it might make sense to add it either here or in one of those PRs.

Risk assessment

Lowish to medium, this is introducing a new side effect to two previously introduced functions, but it is also now correctly adding them to the store immediatly and reducing confusing bugs/error line numbers.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fae29a7) 68.04% compared to head (66feff8) 68.04%.
Report is 19 commits behind head on edge.

❗ Current head 66feff8 differs from pull request most recent head cf51345. Consider uploading reports for the commit cf51345 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #14358   +/-   ##
=======================================
  Coverage   68.04%   68.04%           
=======================================
  Files        2508     2508           
  Lines       71585    71585           
  Branches     9106     9106           
=======================================
  Hits        48712    48712           
  Misses      20757    20757           
  Partials     2116     2116           

Flag	Coverage Δ
g-code-testing	96.48% <ø> (ø)
notify-server	89.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
.../src/opentrons/protocol_engine/actions/__init__.py	100.00% <ø> (ø)
...i/src/opentrons/protocol_engine/actions/actions.py	100.00% <ø> (ø)
...c/opentrons/protocol_engine/clients/sync_client.py	100.00% <ø> (ø)
...i/src/opentrons/protocol_engine/protocol_engine.py	100.00% <ø> (ø)

[SyntaxColoring]

Minor comments, but this looks like it does what it says on the tin, thanks.

I agree that we need to be careful about PAPI versioning here. If we have any reason to believe that any protocol that used to work will not work anymore, this should be behind an apiLevel bump—even if we think that protocol is "weird."

---

Recapping a conversation that the team had on a call:

It's becoming eminently clear that our protocol analysis system is inadequate to implement deck configuration. We have rough ideas for how to solve this ("multi-pass analysis" aka "concrete analysis"), and we think those ideas will also help with runtime parameters. But we have not yet fleshed those out or charted a plan to implement them.

I think our efforts are better spent towards fleshing out those ideas and charting a plan to implement them, instead of trying to ad-hoc patch around the problems in the current system, which we know is a dead-end.

However, the Tribe has Spoken, and this seems unobtrusive enough, so I'm happy for it to be merged if you all find the additional complexity tolerable.

[SyntaxColoring]

Can we have some documentation explaining the intent of this new method and how it compares with the existing deck_configuration param in play()?

[SyntaxColoring]

Can we have a comment explaining why 1ChannelWasteChute, specifically, is an appropriate value?

My understanding is that it's something like this:

We want to tell Protocol Engine that there's "something" in the waste chute location, so analysis can prevent the user from doing anything that would collide with it. (Mostly 96-channel partial tip pickup.)

At the same time, we do not want that "something" to conflict with information that we learn later in analysis, or with whatever will be present in the actual run.

1ChannelWasteChute is appropriate because it implies that a waste chute fixture exists, and it happens to not restrict what kind of waste chute fixture exists, since every waste chute fixture provides the 1ChannelWasteChute addressable area.

Is this all correct?