-
Notifications
You must be signed in to change notification settings - Fork 179
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
Conversation
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.
@dataclass | ||
class TipPhysicallyMissingErrorInternalData: | ||
"""Internal-to-ProtocolEngine data about a TipPhysicallyMissingError.""" | ||
|
||
pipette_id: str | ||
labware_id: str | ||
well_name: str |
There was a problem hiding this comment.
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.
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. | ||
""" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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."
An unrelated robot-server test is failing. Does this fix it?
Robot server test failures are unrelated, caused by AUTH-401. |
There was a problem hiding this 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.
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
tipPhysicallyMissing
error and make thepickUpTip
command return it, when appropriate.Test plan
tipPhysicallyMissing
error.tipPhysicallyMissing
error shows up in thepickUpTip
command's OpenAPI spec.error
value, separate from the defaultErrorOccurrence
. 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.