feat(api): build out well state for liquid height tracking #15681

aaron-kulkarni · 2024-07-16T19:09:32Z

Overview

This PR adds a new WellState class. This class currently has only one attribute, measured_liquid_heights, which is used in liquid height tracking. We created this new class(rather than just adding this attribute to labware state or something related), to allow for more detailed well state tracking in the future.

Details

measured_liquid_heights contains a Dict[labware_id, Dict[well_name, LiquidHeightInfo]]. This dictionary structure is meant to give time-efficient access to all wells of a particular labware, since the dictionary is primarily indexed by labware_id.

The presence/non-presence of a well in the WellState class indicates the following:

If a well is not present: that well has never been probed.
If a well is present but has a height value of 0: the most recent probe into that well found no liquid.
If a well is present and has a height value of > 0: the most recent probe into that well found liquid.

A quick word on the implementation of the new LiquidHeightInfo type. It has two attributes, height and last_measured. The last_measured attribute currently stores as a datetime, but it could be useful in the future to consider other alternatives. Maybe protocol steps?(or something similar). The reason we have a time-related attribute at all is because right now we don't track things like aspirates and dispenses, or even user inputted liquid loading(if that's the correct term). All liquid height state is determined only by doing a liquid_probe, so it's useful to know what the last time was we did a liquid_probe on this well.

For the purposes of the StateSummary, a new type was defined called LiquidHeightSummary. It has identifying information about the labware/well and the contents of the LiquidHeightInfo type.

Test Plan

Unit tests written and passed for well_view and well_store.

Changelog

Created WellState class, along with complementary WellView and WellStore
Created LiquidHeightInfo type, which is used to store info about recent liquid height measurements.
Created LiquidHeightSummary type, which is used in StateSummary.

Review requests

Risk assessment

Low.

sfoster1

I think the structure of this is nice and we'll definitely want a WellStore, but I think we should put some thought in - and write that thought down - about the structure of the information within. I think we can write that down in the description, but honestly the description should be the longest part of this PR in that case. We should cover

How do we track the relationship of these Well objects to the labware that contains them?
What does the presence or absence of a Well in the wells data structure mean? What's it based on?
How do we indicate that a well has never been probed for the presence of liquid? Probed, and no liquid found? Probed, and liquid found? Probed, and liquid found, but known to have been refilled?
In general, what information should a human looking at a serialization of the Wells data structure be able to come to?

sfoster1 · 2024-07-17T17:54:30Z

api/src/opentrons/protocol_engine/state/wells.py

+        if isinstance(action, FailCommandAction):
+            self._handle_failed_command(action)
+
+    def _handle_succeded_command(self, command: Command) -> None:


Suggested change

def _handle_succeded_command(self, command: Command) -> None:

def _handle_succeeded_command(self, command: Command) -> None:

sfoster1 · 2024-07-17T17:55:48Z

api/src/opentrons/protocol_engine/state/wells.py

+            well = WellIdentifier(
+                labware_id=command.params.labwareId, well_name=command.params.wellName
+            )
+            if command.result is None:


does that make sense? Under what conditions would the command result be None?

Agreed it doesn't make sense. I initially did that to pass lint checks but I found a cleaner way around it.

sfoster1

Looking good, but let's remove that commented out code and make sure the well identification information is in the summary too. Also maybe a format pass on the description?

sfoster1 · 2024-07-19T18:03:54Z

api/src/opentrons/protocol_engine/state/wells.py

+class WellState:
+    """State of all wells."""
+
+    #                       Dict[Labware: Dict[Wellname: [Height,TimeRecorded]]]


Remove commented out code

sfoster1 · 2024-07-19T18:04:41Z

api/src/opentrons/protocol_engine/state/state_summary.py

@@ -28,3 +29,4 @@ class StateSummary(BaseModel):
    startedAt: Optional[datetime]
    completedAt: Optional[datetime]
    liquids: List[Liquid] = Field(default_factory=list)
+    wells: List[LiquidHeightInfo] = Field(default_factory=list)


This needs to have more of the data from the state - from just the summary, you wouldn't be able to tell what well is being talked about.

github-actions · 2024-07-22T13:49:35Z

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

codecov · 2024-07-22T13:59:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (0902ff4) to head (3b8fd27).
Report is 56 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #15681   +/-   ##
=======================================
  Coverage   92.43%   92.43%           
=======================================
  Files          77       77           
  Lines        1283     1283           
=======================================
  Hits         1186     1186           
  Misses         97       97

Flag	Coverage Δ
g-code-testing	`92.43% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

sfoster1

Some more small style things.

sfoster1 · 2024-07-22T13:54:54Z

api/src/opentrons/protocol_engine/state/wells.py

+    def get_all(self) -> List[LiquidHeightSummary]:
+        """Get all well liquid heights."""
+        allHeights = []  # type: List[LiquidHeightSummary]
+        # for key,  in self._state.measured_liquid_heights.items():


remove commented out code

sfoster1 · 2024-07-22T13:59:20Z

api/src/opentrons/protocol_engine/state/wells.py

+        """Set the liquid height of the well."""
+        lhi = LiquidHeightInfo(height=height, last_measured=time)
+        try:
+            self._state.measured_liquid_heights[labware_id]


This is not really a great pattern. In Python it's usually better to catch exceptions than try up front (some people disagree, but let's accept it for the sake of argument), but that means "write the code you were going to write and then catch possible exceptions", not "do a special thing to try and cause an exception". There's two options to try:

Make self._state.measured_liquid_heights actually be a defaultdict that builds dictionaries as its defaults; however, this might not work great with serialization

Just have this code look more like if labware_id not in self._state.measured_liquid_heights: ...

sfoster1 · 2024-07-22T14:00:30Z

api/src/opentrons/protocol_engine/state/wells.py

+        #     lhs = LiquidHeightSummary(labware_id=)
+        #     allHeights.extend(a for a in val.values())
+        # return allHeights
+        for labware in self._state.measured_liquid_heights.keys():


nit: could be for labware, wells in self._state.measured_liquid_heights.items() and then for well, lhi in wells.items(). I htink it would be neater because it would be only accessing temporaries.

sfoster1 · 2024-07-22T14:01:02Z

api/src/opentrons/protocol_engine/state/wells.py

+
+    def get_all_in_labware(self, labware_id: str) -> List[LiquidHeightSummary]:
+        """Get all well liquid heights for a particular labware."""
+        allHeights = []  # type: List[LiquidHeightSummary]


Prefer type annotations (i.e. all_heights: List[LiquidHeightSummary]) to type comments (i.e. what you have here)

sfoster1 · 2024-07-22T14:01:20Z

api/src/opentrons/protocol_engine/state/wells.py

+
+    def get_all(self) -> List[LiquidHeightSummary]:
+        """Get all well liquid heights."""
+        allHeights = []  # type: List[LiquidHeightSummary]


prefer snake_case for variables

sfoster1 · 2024-07-22T14:01:29Z

api/src/opentrons/protocol_engine/state/wells.py

+
+    def get_all(self) -> List[LiquidHeightSummary]:
+        """Get all well liquid heights."""
+        allHeights = []  # type: List[LiquidHeightSummary]


prefer type annotations to type comments

sfoster1 · 2024-07-22T14:03:43Z

api/src/opentrons/protocol_engine/state/wells.py

+    def has_measured_liquid_height(self, labware_id: str, well_name: str) -> bool:
+        """Returns True if the well has been liquid level probed previously."""
+        try:
+            self._state.measured_liquid_heights[labware_id][well_name].height


here is a good example of handling exceptions as something that happens from normal code. This will work, but a better way to do it would be something like

try: return bool(self._state.measured_liquid_heights[labware_id][well_name]) except KeyError: return False

As with the inserting defaults example above, you really want the exception to be thrown while doing something you intend, not just doing something to see if it throws.

github-actions · 2024-07-23T18:32:24Z

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

aaron-kulkarni · 2024-07-23T18:43:19Z

Nevermind I did need the snapshot changes. The snapshot stuff on this branch is unrelated to the issues we were having earlier.

fixed

sfoster1

I think this is a really solid first implementation, and I also think we shouldn't merge it to edge until we branch it to 8.0. So this request-changes is to mark that we shouldn't merge it until then, but I do think it's great!

Add a well state class that will be used for now just for liquid height tracking but in the future could be built out more. re EXEC-603

… snapshot failure capture (#15730)" This reverts commit 3b8fd27.

sfoster1

Great! Thanks for finishing up the merge.

aaron-kulkarni mentioned this pull request Jul 16, 2024

feat(api, shared-data): build out functionality for tracking liquid height #15666

Closed

aaron-kulkarni marked this pull request as ready for review July 16, 2024 20:15

aaron-kulkarni requested a review from a team as a code owner July 16, 2024 20:15

aaron-kulkarni requested a review from ryanthecoder July 16, 2024 20:50

sfoster1 requested changes Jul 17, 2024

View reviewed changes

aaron-kulkarni requested a review from sfoster1 July 17, 2024 19:57

sfoster1 requested changes Jul 19, 2024

View reviewed changes

aaron-kulkarni requested a review from a team as a code owner July 22, 2024 13:54

sfoster1 previously requested changes Jul 22, 2024

View reviewed changes

aaron-kulkarni requested a review from sfoster1 July 22, 2024 15:04

sfoster1 requested changes Jul 26, 2024

View reviewed changes

aaron-kulkarni requested a review from a team as a code owner July 26, 2024 18:52

aaron-kulkarni requested review from smb2268 and removed request for a team July 26, 2024 18:52

aaron-kulkarni force-pushed the exec-603-add-last-liquid-height branch from 5718edb to bb66627 Compare July 29, 2024 20:40

aaron-kulkarni added 10 commits July 29, 2024 16:41

feat(api): build out well state for liquid height tracking

2ab0ddd

Add a well state class that will be used for now just for liquid height tracking but in the future could be built out more. re EXEC-603

lint

8edc9f2

added tests

55d1954

docstring fix

f74cfa3

lint fix?

036094e

test cov

537007e

oops formatting

7f2e7c4

last robot server cov fix

b74d83e

formatting again

6ab3690

added time to wellstate

b2ee8f3

aaron-kulkarni added 6 commits July 29, 2024 16:41

typo fix

cd598c1

addressing comments. removing WellIdentifier

1862f94

lint

6e2e2fb

refactoring

803eda8

addressing comments

56b81fc

Revert "fix(analyses-snapshot-testing): exec-603-add-last-liquid-height…

f6eef7a

… snapshot failure capture (#15730)" This reverts commit 3b8fd27.

aaron-kulkarni force-pushed the exec-603-add-last-liquid-height branch from bb66627 to f6eef7a Compare July 29, 2024 20:42

pmoegenburg added 2 commits September 6, 2024 15:31

Merge branch 'edge' into exec-603-add-last-liquid-height

943e3e0

lint fix

ddfa84a

sfoster1 approved these changes Sep 6, 2024

View reviewed changes

sfoster1 merged commit 9869356 into edge Sep 6, 2024
21 checks passed

sfoster1 deleted the exec-603-add-last-liquid-height branch September 6, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): build out well state for liquid height tracking #15681

feat(api): build out well state for liquid height tracking #15681

aaron-kulkarni commented Jul 16, 2024 •

edited

Loading

sfoster1 left a comment

sfoster1 Jul 17, 2024

sfoster1 Jul 17, 2024

aaron-kulkarni Jul 17, 2024

sfoster1 left a comment

sfoster1 Jul 19, 2024

sfoster1 Jul 19, 2024

github-actions bot commented Jul 22, 2024

codecov bot commented Jul 22, 2024

sfoster1 left a comment

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

sfoster1 Jul 22, 2024

github-actions bot commented Jul 23, 2024

aaron-kulkarni commented Jul 23, 2024

sfoster1 left a comment

sfoster1 left a comment

	def _handle_succeded_command(self, command: Command) -> None:
	def _handle_succeeded_command(self, command: Command) -> None:

feat(api): build out well state for liquid height tracking #15681

feat(api): build out well state for liquid height tracking #15681

Conversation

aaron-kulkarni commented Jul 16, 2024 • edited Loading

Overview

Details

Test Plan

Changelog

Review requests

Risk assessment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2024

codecov bot commented Jul 22, 2024

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2024

aaron-kulkarni commented Jul 23, 2024

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

aaron-kulkarni commented Jul 16, 2024 •

edited

Loading