feat(app, api): map PAPIv2 liquid handling commands to JSON v6 commands

[jerader]

closes #11187 & RSS-61

Overview

Maps Aspirate, Dispense, and Blow_out to json v6 commands. In order to support pipette access to labware loaded onto modules, this also adds a mapping of labware to module ids.

Changelog

• add Aspirate, Dispense, and Blow_out to legacy_command_mapper and add coverage to the smoke test
• fix bug that was causing labware loaded onto modules to be inaccessible

Review requests

• Check that aspirate and dispense maps correctly and all test cases are included in the smoke test
  params tested:
• location (with specific labware, specific well, no specific well or labware
• with and without rate
• only with volume
• Check that blow_out maps correctly when there IS NO location included and that test is included in smoke test
• Check that blow_out still maps to custom when there IS location included and that test is included in smoke test( TODO comments are also included to make sure that we remember to add mapping logic at a future time)
• Upload a protocol with aspirate, dispense, and blow_out and check the result of this function, one way is to console.log the protocolData in stepText, you should see the aspirate, dispense and blow_out show up as their correct commands/commandType rather than custom

Risk assessment

low

[codecov]

Codecov Report

Merging #11296 (d72da65) into edge (7e3f1e6) will decrease coverage by 10.80%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             edge   #11296       +/-   ##
===========================================
- Coverage   74.40%   63.59%   -10.81%     
===========================================
  Files        2069     1357      -712     
  Lines       57258    23401    -33857     
  Branches     5529     5390      -139     
===========================================
- Hits        42601    14883    -27718     
+ Misses      13401     7274     -6127     
+ Partials     1256     1244       -12     

Flag	Coverage Δ
api	?
g-code-testing	?
hardware	?
hardware-testing	?
notify-server	89.17% <ø> (ø)
ot3-gravimetric-test	?
react-api-client	?
robot-server	?
shared-data	?
update-server	?

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

Impacted Files	Coverage Δ
...opentrons/protocol_runner/legacy_command_mapper.py
hardware/opentrons_hardware/firmware_update/run.py
...othie/reset_from_error_g_code_functionality_def.py
...erver/robot_server/service/notifications/router.py
...act-api-client/src/sessions/useAllSessionsQuery.ts
api/src/opentrons/protocols/api_support/util.py
...rver/robot/calibration/tip_length/state_machine.py
robot-server/robot_server/runs/router/__init__.py
...c/opentrons/protocols/geometry/labware_geometry.py
robot-server/robot_server/service/dependencies.py
... and 702 more

[TamarZanzouri]

overall code changes look good to me! nice job! just a few small changes.
I have asked robot services for another review bc I am not familiar with the legacy command mapper.

[sanni-t]

I would highly recommend splitting up the _build_initial_command method into multiple private methods. You can even group them by theme- tip_handling_commands and liquid_handling_commands; especially since the aspirate and dispense commands share so much of their code. That will make the code much more readable and reduce unnecessary code size. Moreover, such if-else ladders tend to hide bugs with multiple 'redefinitions' of the same variable (e.g., volume is defined in both aspirate & dispense. Python linting & error-checking is a little weird/lenient with variable scopes).

Tests won't have to change since they'll all be private methods. So as long as we have public method tests & smoke tests passing, those don't need to be updated.

[sanni-t]

I'd switch these to accessing directly with keys instead of the get method. We've already established that these keys exist in lines 325-327 and those params don't have an optional Nonetype in the aspirate/dispense command message types so we won't have to do the checks for None here at all.

[sanni-t]

Let's make sure we're passing the correct argument type here. labware.first_parent() can be a string or None while _module_id_by_slot expects its key to be a DeckSlotName type.

First, assert that first_parent is not None, then cast the string to DeckSlotName.

[jerader]

Having a bit of troubles. Can you elaborate a bit? I tried the following but getting a lint error:
assert location.labware.first_parent() is not None
firstParent : DeckSlotName = location.labware.first_parent()
parentModuleId = self._module_id_by_slot.get(firstParent)

[sanni-t]

Try-

first_parent = location.labware.first_parent()
assert first_parent is not None, "Labware needs to be associated with a slot
parent_module_id = self._module_id_by_slot.get(DeckSlotName(first_parent))

Also, just realized, you'll want to make the variable names lowercase and underscored (it's the convention we follow for all python code)
(Updated them in this comment just now)

[jerader]

That works, thank you! I will fix the other areas to be the same as well as fixing the variable names.

[sanni-t]

We'll also need to add legacy_command_mapper unit tests for aspirate & dispense.

[sanni-t]

We'll also need to add legacy_command_mapper unit tests for aspirate & dispense.

Paired with @jerader to address the above comments and looks like writing a unit test for mapping pipetting commands is not that straight forward. Smoke tests are basically covering the mapping functionality verification so unit tests don't seem to be strictly required. Maybe they'll be easier after the broker refactor.

[mcous]

In addition to these comments below about never doing raise Exception(...) in our Python codebase, we're missing tests for the various permutations of the liquid handling commands at different location types and with various parameters specified and unspecified.

We'll need to be pretty confident in our test coverage of this one, because unexpected exceptions during command mapping will not fail gracefully.

I'd recommend writing a Python protocol per command type that executes the PAPIv2 method under test in all possible manner to ensure proper mapping

[mcous]

Still missing test cases, did a more thorough code review and looks like there's some stuff to clear up in the command mapper itself, too. Concentrate on getting the test cases in place before continuing to change the logic code

[mcous]

Just some cleanup comments, but nothing that affects functionality

[mcous]

Can we change this to an assert? The Protocol API ensures that only a well Location will ever make it here, so the if is a little misleading, because the else would be a violation of our internal guarantees

Suggested change

	if location.labware.is_well:
	well = location.labware.as_well()
	assert location.labware.is_well
	well = location.labware.as_well()

You can tell this if is bad because it's the only place where the well variable is assigned a value, but it's used later in the function outside of the if

[mcous]

Same comment here; the PAPI is supposed to guarantee this, so we should assert rather than branch

Suggested change

	if isinstance(location, LegacyWell):
	well = location
	assert isinstance(location, LegacyWell)
	well = location

[mcous]

For readability, it might be worth moving this block to the top. I had a lot of trouble tracing this back to its original if

[mcous]

Should this just be one if?

Suggested change

	if isinstance(location, Location) and location.labware.is_empty is False:
	if location.labware.is_well:
	if isinstance(location, Location) and location.labware.is_well:

Got an approval from Mike