test(api): Configure ctx fixture as PAPIv2.14 when it's configured as an OT-3

[SyntaxColoring]

Overview

The api tests have this top-level ctx pytest fixture:

opentrons/api/tests/opentrons/conftest.py

Lines 258 to 260 in a89d965

	@pytest.fixture()
	def ctx(hardware: ThreadManagedHardware) -> ProtocolContext:
	return create_protocol_context(api_version=APIVersion(2, 13), hardware_api=hardware)

Right now, any tests that use this ctx fixture will try to run under two configurations:

	OT-2 hardware simulator and deck definition	OT-3 hardware simulator and deck definition
PAPI version="2.13"	✅	✅

With this PR, that changes to this:

	OT-2 hardware simulator and deck definition	OT-3 hardware simulator and deck definition
PAPI version="2.13"	✅	🚫
PAPI version="2.14"	🚫	✅

This work is one part of RLAB-250, and it will also help with RCORE-535.

Test plan

• Make sure the api tests keep passing, both locally and in CI.
• Make sure the api tests don't have any hangs.
• Make sure the api tests don't have any new flaky failures.

Why

PAPIv2.14 runs through Protocol Engine, and PAPIv2.13 runs through the legacy PAPI back-end.

The legacy PAPI back-end will not support the OT-3. So this fixture was "wrong," in the sense that it was causing tests to run in a configuration that we don't expect to work.

In practice, it's happened to work so far. At least, it's worked well enough. However, in my work on RLAB-250, it stopped working well enough. I wanted to change the OT-3's deck definition to have slot names like "D1" instead of "1", but PAPIv2.13's Deck class isn't built to support that, so I got a bunch of errors from the PAPIv2.13 Deck class:

ERROR tests/opentrons/protocol_api_old/test_labware_load.py::test_load_to_slot[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_labware_load.py::test_loaded[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_labware_load.py::test_load_label[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_labware_load.py::test_deprecated_load[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-tempdeck-TemperatureModuleContext-temperatureModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-temperature module-TemperatureModuleContext-temperatureModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-temperature module gen2-TemperatureModuleContext-temperatureModuleV2] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-magdeck-MagneticModuleContext-magneticModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-magnetic module-MagneticModuleContext-magneticModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-magnetic module gen2-MagneticModuleContext-magneticModuleV2] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-thermocycler-ThermocyclerContext-thermocyclerModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-thermocycler module-ThermocyclerContext-thermocyclerModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-thermocycler module gen2-ThermocyclerContext-thermocyclerModuleV2] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_load_simulating_module[OT-3 Standard-heaterShakerModuleV1-HeaterShakerContext-heaterShakerModuleV1] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_thermocycler_semi_plate_configuration[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_magdeck_gen1_labware_props[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocol_api_old/test_module_context.py::test_magdeck_gen2_labware_props[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'

ERROR tests/opentrons/protocols/api_support/test_instrument.py::test_validate_takes_liquid[OT-3 Standard-True] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/api_support/test_instrument.py::test_validate_takes_liquid[OT-3 Standard-False] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/api_support/test_instrument.py::test_validate_takes_liquid_module_location[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/api_support/test_util.py::test_build_edges_left_pipette[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/api_support/test_util.py::test_build_edges_right_pipette[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'

ERROR tests/opentrons/protocols/execution/test_execute_json_v3.py::test_load_labware_from_json_defs[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/execution/test_execute_python.py::test_execute_ok[OT-3 Standard-testosaur_v2.py] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/execution/test_execute_python.py::test_bad_protocol[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'
ERROR tests/opentrons/protocols/execution/test_execute_python.py::test_proto_with_exception[OT-3 Standard] - ValueError: invalid literal for int() with base 10: 'D1'

How

This turns out to be tricky because of concurrency stuff.

When ProtocolEngine is used to execute a Python protocol, the two must be in separate threads. This is because ProtocolEngine expects to be in an asyncio event loop, whereas the Python Protocol API is all blocking and non-async. If we tried to put them in the same thread, they would deadlock. PipetteContext.aspirate() would enqueue a Protocol Engine aspirate command and wait for it to complete, but it would never complete because the ProtocolEngine's background command execution task would never have a chance to run, because the thread is blocked on pipette.aspirate(...) and not yielding to the asyncio event loop!

In robot-server's production code, we solve this by keeping the ProtocolEngine in the "main thread"'s event loop and stuffing the Python protocol in a "worker thread." Here, in api's tests, we need to do the reverse. The "main thread" is already occupied by the pytest tests, so we need to stuff the ProtocolEngine into a worker thread.

This means the ctx fixture needs to:

1. Before proceeding to the test, create a background thread.
2. In that background thread, start running an asyncio event loop.
3. In that asyncio event loop, start running a ProtocolEngine.
4. Let the test run.
5. After the test completes, clean up the background thread and everything in it.

This is one of the problems that ThreadManager solves for the hardware API. We're basically inventing a new ThreadManager for ProtocolEngine.

We do get to avoid some of ThreadManager's pitfalls, though. ThreadManager does two things: keep an async object in a background thread, and facilitate safe access to that async object. In our case, facilitation of safe access is already done by SyncClient, so we only need to solve the problem of keeping the async object in the background thread.

Review requests

Reinventing parts of ThreadManager for ProtocolEngine isn't something that we should take lightly. We should question several premises of this PR.

• Configuring a ProtocolContext as an OT-3—that is, with an OT-3 hardware controller and an OT-3 deck definition—is fundamentally wrong, right?
• Am I correct in thinking that we need two threads?
• If both of the above are true, I think that this is the correct approach because even if we theoretically could have rewritten the failing tests to avoid this problem now, we would have needed machinery like this anyway to implement opentrons.execute.get_protocol_api(version="2.14") for RCORE-535. Do you agree?

Plus some finer points:

• Should we be trying to deduplicate things more with ThreadManager instead of writing this background thread machinery from scratch? On one hand, duplication is bad, but on the other, ThreadManager is unnecessarily complicated for this purpose.

• Do we also want to test the combination of OT-2 and PAPIv2.14? In other words, do we want this?

  	OT-2 hardware simulator and deck definition	OT-3 hardware simulator and deck definition
  PAPI version="2.13"	✅	🚫
  PAPI version="2.14"	✅	✅

Risk assessment

So far, changes are just to tests, so there's no risk to breaking production behavior.

There is some risk of introducing architectural debt. See the review requests above.

[codecov]

Codecov Report

Merging #12567 (be51846) into edge (e32ef94) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             edge   #12567   +/-   ##
=======================================
  Coverage   73.31%   73.31%           
=======================================
  Files        1506     1506           
  Lines       49349    49349           
  Branches     2997     2997           
=======================================
  Hits        36180    36180           
  Misses      12713    12713           
  Partials      456      456           

Flag	Coverage Δ
notify-server	89.13% <ø> (ø)

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

[sfoster1]

I do agree with all of this. I think making a protocol context with an ot3 does make sense in practice, since people have been using it since before the contents of 2.14 existed, but I think it shouldn't.

I do in fact think that it's a good idea to test OT-2 + 2.14, but I think we've got a neat path set out for that and do not have to do it in this PR; I think PRs that move things forward but perhaps not far enough are good to merge and be taken up later.

Couple inline things though.

[sfoster1]

Yeah this makes more sense, and I think you've got the right set of things to test. In terms of loop cleanup... this is entirely a test artifice, and I think you're exactly right that we hsouldn't worry about it too much.

[sfoster1]

Yeah this looks fine. I'd kinda prefer having the functionality in threadmanager but if this works it works.

[jbleon95]

LGTM, this is definitely needed to make our tests more accurate and I think this is the right step forward.

[sanni-t]

Do we also want to test the combination of OT-2 and PAPIv2.14? In other words, do we want this?

Totally. It's important, but can be achieved in a follow-up.