feat(api): restore Thermocycler semi-configuration for API >= 2.18#20865
feat(api): restore Thermocycler semi-configuration for API >= 2.18#20865kirushavasilev wants to merge 1 commit intoOpentrons:edgefrom
Conversation
Re-enable the configuration='semi' parameter for load_module() when loading a Thermocycler module. This was deprecated in API v2.14 (PR Opentrons#12156) because the legacy geometry system couldn't handle it reliably. Now that the engine-backed API is mature, we can restore it properly. Changes: - protocol_context.py: Narrow the UnsupportedAPIError to only block configuration='semi' for API 2.14-2.17 (not all >= 2.14) - engine/protocol.py: Remove assert, route configuration through to deck conflict via register_semi_tc_module() - engine/deck_conflict.py: Add _semi_tc_module_ids registry so _map_module correctly sets is_semi_configuration based on actual module config - test_deck_conflict.py: Add test_maps_semi_configured_thermocycler The semi-configuration causes the Thermocycler to only occupy slots 7/10 (instead of 7/8/10/11), freeing slots 8 and 11 for labware. Users must physically reposition their Thermocycler and use set_offset() to adjust the labware position accordingly. Closes #XXXX
sfoster1
left a comment
sfoster1
left a comment
There was a problem hiding this comment.
Hi, @kirushavasilev ! Thank you for the PR!
There are a couple things I think we need to change about it, though. I've put them in inline comments on the PR.
More generally, though, I think the changes need to go a bit deeper and get integrated into the protocol engine - otherwise, we risk bringing in the same layer of offsets poked into interesting places that we mistakenly added in the first place and became unmaintainable enough we removed the configuration.
I definitely think we can get there, though! Thank you again for the PR!
| # with proper engine-backed support and automatic offset. | ||
| if ( | ||
| self._api_version >= ENGINE_CORE_API_VERSION | ||
| and self._api_version < APIVersion(2, 18) |
There was a problem hiding this comment.
The idea with the protocol API versions is that we never change them retroactively (we do an okay, though not perfect, job of living up to this). That may mean that old API versions have bugs that are preserved - and that's on purpose, so that somebody who's worked around that bug in their protocol doesn't have to alter their protocol to remove the workaround until they raise the target API version. So, we'd want this change to target the next unreleased API version, which in edge would be 2.29. You'll have to modify opentrons.protocols.api_support.definitions.MAX_SUPPORTED_VERSION to raise it from 2.28 to 2.29, and change the version checks here to support it.
|
|
||
| _log = logging.getLogger(__name__) | ||
|
|
||
| # Module-level registry of Thermocycler module IDs loaded in semi-configuration. |
There was a problem hiding this comment.
While this seems like it will work, I'm not a big fan of these kinds of very non-local state transformations. I think this would be better off as saving the information in some kind of data inside an object instance and passing it into _map_module or saving it as part of the engine state.
It's the engine state approach that I think is the right one, in fact. This is probably better off being a part of the dedicated thermocycler module substate such that it can be queried by some engine state query method when the protocol api core is calling the deck conflict check - for instance, a new bool property on opentrons.protocol_engine.state.module_substates.thermocycler_module_substate.ThermocyclerModuleSubstate, and a new function on that same object to query it (we prefer function calls to object access to preserve space for moving this into an IPC in the future).
| # Python Protocol API >=v2.14 never allows loading a Thermocycler in | ||
| # its semi configuration. | ||
| is_semi_configuration=False, | ||
| is_semi_configuration=(module_id in _semi_tc_module_ids), |
There was a problem hiding this comment.
going with the above comment about moving the semi status into the engine, this can be replaced with an engine check
| # Register semi-configured Thermocyclers so the deck conflict | ||
| # checker knows to only block slots 7 and 10 (not 8 and 11). | ||
| if is_semi: | ||
| deck_conflict.register_semi_tc_module(result.moduleId) |
There was a problem hiding this comment.
again going with the comment about moving it to the engine, this would probably have to become a parameter to the LoadModule command
|
|
||
| configuration: Configure a Thermocycler to be in the `semi` position. | ||
| This parameter does not work. Do not use it. | ||
| configuration: Configure a Thermocycler to be in the ``semi`` position. |
There was a problem hiding this comment.
Probably also worth noting (and adding a check to ensure) that this could only happen on the OT-2.
2 participants
Re-enable the configuration='semi' parameter for load_module() when loading a Thermocycler module. This was deprecated in API v2.14 (PR #12156) because the legacy geometry system couldn't handle it reliably. Now that the engine-backed API is mature, we can restore it properly.
This is a really useful functionality that our lab uses in automation.
Changes:
The semi-configuration causes the Thermocycler to only occupy slots 7/10 (instead of 7/8/10/11), freeing slots 8 and 11 for labware. Users must physically reposition their Thermocycler and use set_offset() to adjust the labware position accordingly.
Overview
Describe your PR at a high level. State acceptance criteria and how this PR fits into other work. Link issues, PRs, and other relevant resources.
Test Plan and Hands on Testing
Describe your testing of the PR. Emphasize testing not reflected in the code. Attach protocols, logs, screenshots and any other assets that support your testing.
Changelog
List changes introduced by this PR considering future developers and the end user. Give careful thought and clear documentation to breaking changes.
Review requests
Risk assessment
Created with support of Opus 4.6