feat(api): store and use disposal locations in the location cache#18268
Conversation
sanni-t
left a comment
sanni-t
left a comment
There was a problem hiding this comment.
LGTM! Glad it wasn't too gnarly to add disposal locations to cache!
| Behavior of the ``volume`` parameter. | ||
|
|
||
| .. versionchanged:: 2.24 | ||
| Can dispense over trash bin or waste chute implicitly. |
There was a problem hiding this comment.
Should elaborate this to explain that dispense no longer needs an explicit trash location if pipette was already moved over a trash previously.
| :returns: This instance. | ||
|
|
||
| .. versionchanged:: 2.24 | ||
| Can blow out over trash bin or waste chute implicitly. |
| if last_location is None or isinstance( | ||
| last_location, (TrashBin, WasteChute) | ||
| ): | ||
| raise RuntimeError("No valid current location cache present") |
There was a problem hiding this comment.
| raise RuntimeError("No valid current location cache present") | |
| raise RuntimeError(f"Cached location of {last_location} is not valid for touch tip.") |
| isinstance(last_location, types.Location) | ||
| and not last_location.labware.is_well | ||
| ): | ||
| raise RuntimeError("No previous valid location cached to perform air gap") |
There was a problem hiding this comment.
Maybe update this error message with a similar one as touch tip suggestion? Helps to actually see the cached location in error message
|
|
||
| Raises: | ||
| NoLocationError: The is no input location and no cached loaction. | ||
| NoLocationError: The is no input location and no cached location. |
There was a problem hiding this comment.
| NoLocationError: The is no input location and no cached location. | |
| NoLocationError: There is no input location and no cached location. |
😝
| mock_protocol_core: ProtocolCore, | ||
| subject: InstrumentCore, | ||
| ) -> None: | ||
| """It should move the pipette to a trash.""" |
There was a problem hiding this comment.
| """It should move the pipette to a trash.""" | |
| """It should move the pipette to a trash and update cached location.""" ? |
…sposal_locations_location_cache_support
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18268 +/- ##
=======================================
Coverage 23.56% 23.56%
=======================================
Files 3059 3059
Lines 256332 256332
Branches 30109 30109
=======================================
Hits 60416 60416
Misses 195902 195902
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| if not loc or not loc.labware.is_well: | ||
| raise RuntimeError("No previous Well cached to perform air gap") | ||
| target = loc.labware.as_well().top(height) | ||
| last_location = self._protocol_core.get_last_location() |
There was a problem hiding this comment.
Did you mean to use _get_last_location_by_api_version() that you defined below?
There was a problem hiding this comment.
If we were to make this change we'd call get_last_location using a mount argument, which we have not been doing here. That may be more technically correct, but this is what has been in the code and changing it without a version check would potentially break or change the behavior of older protocols, and changing that is slightly out of scope for this PR.
| if self.api_version < APIVersion(2, 24) and isinstance( | ||
| last_location, (TrashBin, WasteChute) | ||
| ): | ||
| last_location = None |
There was a problem hiding this comment.
Did you mean to use _get_last_location_by_api_version() that you defined below?
There was a problem hiding this comment.
(Sorry, I don't know why Github sent out a duplicate comment here!)
| ) | ||
| return last_location if isinstance(last_location, types.Location) else None | ||
| else: | ||
| return self._protocol_core.get_last_location() |
There was a problem hiding this comment.
What does this return (before version 2.14)? Could this be a trashbin/wastechute?
There was a problem hiding this comment.
This calls get_last_location without a mount argument. We can get away without checking the type because that core will be the legacy core and that will not return a trash bin or waste chute.
| and isinstance(last_location, types.Location) | ||
| and isinstance(last_location.labware, labware.Well) | ||
| ): | ||
| self.move_to(last_location.labware.top()) |
There was a problem hiding this comment.
So the pipette should not move during configure_for_volume() if the pipette is in the trash/wastechute?
There was a problem hiding this comment.
The main concern for configure_for_volume is accidentally aspirating liquid if the configuration is done while the tip is submerged in liquid. This is not a valid concern if the tip is over a trash bin or waste chute.
| if self.api_version < APIVersion(2, 24) and isinstance( | ||
| last_location, (TrashBin, WasteChute) | ||
| ): | ||
| last_location = None |
There was a problem hiding this comment.
(Sorry, I don't know why Github sent out a duplicate comment here!)
| def location_cache(self) -> Optional[Union[Location, TrashBin, WasteChute]]: | ||
| """The cache used by the robot to determine where it last was. | ||
|
|
||
| .. versionchanged:: 2.24 |
There was a problem hiding this comment.
We've hidden this reference entry, but this is a good addition for internal documentation.
| If previously moved to, dispensed or blown out into a trash bin or waste chute, | ||
| you do not need to include location if blowing out into that disposal location. |
There was a problem hiding this comment.
These are a little wordy, but accurate and not as vague as the previous wording with "implicitly". If you feel like one more commit:
| If previously moved to, dispensed or blown out into a trash bin or waste chute, | |
| you do not need to include location if blowing out into that disposal location. | |
| ``location`` is no longer required if the pipette just moved to, dispensed, or blew out into a trash bin or waste chute. |
Otherwise 
…8268) Adds support to the location cache to store and use trash bin and waste chute objects
…8268) Adds support to the location cache to store and use trash bin and waste chute objects
…8268) Adds support to the location cache to store and use trash bin and waste chute objects
…8268) Adds support to the location cache to store and use trash bin and waste chute objects
…8268) Adds support to the location cache to store and use trash bin and waste chute objects
4 participants
Overview
Closes AUTH-1629.
This PR adds support to the location cache to store
TrashBinandWasteChuteobjects, as well as using those inInstrumentContext.dispense,InstrumentContext.blow_outandInstrumentContext.air_gapif you have last accessed a trash bin or waste chute and call those without arguments.air_gapdoes not take a location argument, but previously would raise an error if not over a well. This was left over from when trashes were labware, and there is no reason to not allow an air gap if over a trash.In addition to using this, we are now storing it after you
move_to,dispenseorblow_outinto a trash or waste chute. While storing is being done unconditionally, we will still returnNoneif below api version 2.24. This maintains bug and error compatibility with previous versions.Test Plan and Hands on Testing
Tested the following protocol works and passes analysis
Changelog
TrashBinandWasteChutewhen accessed bymove_to,dispenseandblow_outdispenseandblow_outto be called without location argument if already over a disposal locationair_gapto be used if moved over a disposal locationReview requests
Risk assessment
Low-medium, touches some public facing code but new behavior is gated by version checks.