-
Notifications
You must be signed in to change notification settings - Fork 178
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): add a reload-labware command #14963
Conversation
The ReloadLabware command reloads the non-position details of a labware that has already been loaded. Its most common use case is to update the labware offset applied to the labware by rerunning the offset binding logic with presumably-updated stored offsets.
labwareId: str = Field( | ||
..., description="The already-loaded labware instance to update." | ||
) | ||
loadName: str = Field( | ||
..., | ||
description="Name used to reference a labware definition.", | ||
) | ||
namespace: str = Field( | ||
..., | ||
description="The namespace the labware definition belongs to.", | ||
) | ||
version: int = Field( | ||
..., | ||
description="The labware definition version.", | ||
) | ||
displayName: Optional[str] = Field( | ||
None, | ||
description="An optional user-specified display name " | ||
"or label for this labware.", | ||
) |
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.
I would have expected this to only take an id
. What do loadName
/namespace
/version
do if they're different from the original? Replace the labware with a different one, but keep the ID?
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.
Correct! Honestly I don't know why someone would want to do this, but it does work, and it leads to better symmetry with the load command
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.
Gotcha. Changing the loadName
/namespace
/version
makes me nervous. No concrete reason, but it feels pretty good to me that a labware ID immutably points to a single thing. How do you feel about leaving this out until we have a use case for it?
I think loadLabware
does let you do this, but it's always kind of been by accident.
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.
I agree with @SyntaxColoring. maybe we should leave this out for now?
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.
Hmm. Okay, though I think it's going to do so little at that point that it'll look weird. I'm not so nervous about the changing-the-definition thing, I think this part of the code is really well tested and generally capable.
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 like there are some trivial linting errors, but otherwise this LGTM. Thank you!
class ReloadLabwareParams(BaseModel): | ||
"""Payload required to load a labware into a slot.""" | ||
|
||
labwareId: str = Field( |
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.
Ty!
|
||
|
||
class ReloadLabware(BaseCommand[ReloadLabwareParams, ReloadLabwareResult]): | ||
"""Load labware command resource model.""" |
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.
Reload?
|
||
|
||
class ReloadLabwareCreate(BaseCommandCreate[ReloadLabwareParams]): | ||
"""Load labware command creation request.""" |
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.
Reload?
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.
Lgtm! Some minor things but looks good!
Adds a new command ReloadLabware, which allows dispatchers to change all the details of a loaded labware except for the location. This is primarily intended to allow getting a new labware offset that was not added to the engine by the time this labware was loaded (though it can technically do more, for symmetry). This doesn't really change a whole lot of behavior and is well-supported with testing. It's a prerequisite for #14940 Closes RSQ-29
Adds a new command ReloadLabware, which allows dispatchers to change all the details of a loaded labware except for the location. This is primarily intended to allow getting a new labware offset that was not added to the engine by the time this labware was loaded (though it can technically do more, for symmetry).
Testing
This doesn't really change a whole lot of behavior and is well-supported with testing. It's a prerequisite for #14940
Closes RSQ-29