Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): Return a defined tipPhysicallyMissing error from pickUpTip commands #15176

Merged
merged 13 commits into from
May 16, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented May 13, 2024

Overview

This implements our first real-world instance of a Protocol Engine command returning a defined error, instead of raising an undefined error. Closes EXEC-427.

Changelog

  • Define a tipPhysicallyMissing error and make the pickUpTip command return it, when appropriate.
  • Some small remaining Protocol Engine updates to handle errors that are returned in addition to errors that are raised.

Test plan

  • Run a protocol with some missing tips. When it fails, the command should fail with the new tipPhysicallyMissing error.
    • Confirmed by looking at the raw HTTP responses.
    • The Flex ODD, however, is still showing the underlying (wrapped) error. I suspect it's considering it higher-priority because its code is numerically higher. I'm going to look at this in a separate PR. EXEC-454
  • Make sure the new tipPhysicallyMissing error shows up in the pickUpTip command's OpenAPI spec.
    • This sort of works. It does show up as a new kind of error value, separate from the default ErrorOccurrence. But it's not showing the class docstring, which is where I've tried to document the error's semantics. So we'll need to figure that out. I'd like to upgrade to Pydantic v2 before worrying about this any further.

Review requests

See my inline GitHub comments.

Risk assessment

Low.

This was unused. Also, in general, sharing TypeVars from a centralized module can be a confusing thing to do, because the TypeVar isn't evaluated in the context of that module.
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner May 13, 2024 22:18
@SyntaxColoring SyntaxColoring marked this pull request as draft May 13, 2024 22:18
Comment on lines +76 to +82
@dataclass
class TipPhysicallyMissingErrorInternalData:
"""Internal-to-ProtocolEngine data about a TipPhysicallyMissingError."""

pipette_id: str
labware_id: str
well_name: str
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tips.py will need this info to logically remove the tip from the tip rack. It's currently getting this info from the command's params, which was quick hack that probably won't scale to other error types.

For the sake of being incremental, I'll undo that hack in a separate PR, not this one. So, as of this PR, this data is just in anticipation of that, and is not currently consumed by anything.

Comment on lines +64 to +73
class TipPhysicallyMissingError(ErrorOccurrence):
"""Returned when sensors determine that no tip was physically picked up.

That space in the tip rack is marked internally as not having any tip,
as if the tip were consumed by a pickup.

The pipette will act as if no tip was picked up. So, you won't be able to aspirate
anything, and movement commands will assume there is no tip hanging off the bottom
of the pipette.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

movement commands will assume there is no tip hanging off the bottom of the pipette.

Is this a good idea? Theoretically, we could assume that there is a tip hanging off the bottom of the pipette.

That might be a bit harder to implement and maintain, since there "is a tip" for movement purposes but "there is not a tip" for liquid handling purposes. But it would be safer for the client. Imagine this error triggers falsely because of a flaky sensor, so physically, there actually is a tip attached. Then, if a client commands a moveToWell as part of its recovery flow, we probably want the robot to go high enough to clear all the labware on the deck, right?

Copy link
Contributor Author

@SyntaxColoring SyntaxColoring May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, if a client commands a moveToWell to be followed by a pickUpTipInPlace, we want the robot to put the pipette nozzle in the proper tip pickup position, which means acting as if there is no tip attached...

The `wrappedErrors` thing looks correct.

The `errorType` thing will be part of EXEC-453.

The `detail` thing, I think yes it can have punctuation, because it can be up to "a couple of sentences."
@SyntaxColoring SyntaxColoring marked this pull request as ready for review May 14, 2024 20:10
@SyntaxColoring
Copy link
Contributor Author

Robot server test failures are unrelated, caused by AUTH-401.

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!!! just a couple of questions but overall I like this change

Merging to fix robot-server test failures.
@SyntaxColoring SyntaxColoring merged commit d6cdb70 into edge May 16, 2024
43 checks passed
@SyntaxColoring SyntaxColoring deleted the return_error_from_pick_up_tip branch May 16, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants