fix(api): Do not hang if there is an error in pickUpTip planning
#18207
Conversation
This file contains hidden or 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
There is one behavioral change here, from the perspective of a protocol engine command implementation. Formerly, the initial value of get_active_channels(), after the pipette was loaded and the first PipetteConfigUpdate was applied to state, would be based on the pipette's *total* channels. This looked like a bug. It is now based on the active channels declared in PipetteConfigUpdate.config.nozzle_map. Because loadPipette commands always start pipettes off in their full nozzle configuration, those two values have always agreed in practice. Therefore, this internal fix does not change anything from the perspective of a protocol engine command issuer. It's just more theoretically correct.
sfoster1
approved these changes
Apr 30, 2025
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 16, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where.
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 17, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 19, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 19, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 19, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 20, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 20, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 22, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 23, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 24, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
ddcc4
pushed a commit
that referenced
this pull request
May 24, 2025
…18207) ## Overview This closes RQA-4058 (protocol analysis hanging if `pickUpTip` is given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)). ## Details When a `pickUpTip` command completes, `TipStore` marks the picked-up tips as "used." Before this PR, that worked by `PickUpTipImplementation` passing along just the single "target" tip, like A1; `TipStore` then internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup. The choice to do the expansion inside `TipStore` caused two problems: 1. Error handling. If it happens inside `TipStore`, and something goes wrong and raises an exception, then we've raised an exception from inside an `opentrons.protocol_engine` *state update.* We are not in a good position to handle that: it was an early architectural of `opentrons.protocol_engine` that all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful. (We can and should do something about that premise, but that's beyond the scope of this PR.) This is what caused RQA-4058. 2. Duplication. For `TipStore` to automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part of `PipetteStore`'s job;`TipStore` was duplicating it. This PR tries to solve both of those by having `PickUpTipImplementation` give `TipStore` a full precomputed list of tips. `PickUpTipImplementation` produces that precomputed list with help from new functions in `PipetteView` and `TipView`. Pipette concerns like nozzle maps are expunged from `TipStore`. Overall, there's no new logic here—just shifting things around and changing what's running where. (cherry picked from commit 0855bd1)
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 closes RQA-4058 (protocol analysis hanging if
pickUpTipis given a bad labware definition), and resolves some duplication we noticed a little while ago (#16469 (comment)).Details
When a
pickUpTipcommand completes,TipStoremarks the picked-up tips as "used." Before this PR, that worked byPickUpTipImplementationpassing along just the single "target" tip, like A1;TipStorethen internally expanded that into the full list of actually-picked-up tips, like A1+B1+⋯+H1, for an 8-channel pickup.The choice to do the expansion inside
TipStorecaused two problems:Error handling. If it happens inside
TipStore, and something goes wrong and raises an exception, then we've raised an exception from inside anopentrons.protocol_enginestate update. We are not in a good position to handle that: it was an early architectural ofopentrons.protocol_enginethat all the real work ought to happen in command implementations, and that state updates ought to be dead-simple and always successful.(We can and should do something about that premise, but that's beyond the scope of this PR.)
This is what caused RQA-4058.
Duplication. For
TipStoreto automatically expand the target well into the full list of actually-picked-up tips, it needed to know details about the pipette's current configuration. That was already part ofPipetteStore's job;TipStorewas duplicating it.This PR tries to solve both of those by having
PickUpTipImplementationgiveTipStorea full precomputed list of tips.PickUpTipImplementationproduces that precomputed list with help from new functions inPipetteViewandTipView. Pipette concerns like nozzle maps are expunged fromTipStore. Overall, there's no new logic here—just shifting things around and changing what's running where.Test plan
I'm leaning on existing unit tests (which I had to largely rewrite, but I tried to retain their original coverage) and analysis snapshot tests.
Review requests
It looks like we're inconsistent in terminology between "nozzle map" and "nozzle configuration." Do we want to standardize in either of those directions?
This is merging into
edgeinstead ofchore_release-8.4.0because it feels too intrusive to add this late in the release, and RQA-4058 doesn't strike me as severe enough to override that. Agreed?Risk analysis
Medium.
The risk is, like, maybe
PipetteStoreandTipStorewere doing their bookkeeping of pipette nozzle layout in slightly different ways, so switching between them will cause a difference in behavior. I've stared closely at them both and I'm pretty sure we're safe.