feat(protocol-engine): implement well & labware's well accessor methods

[sanni-t]

Overview

Adds well access methods to Labware class

Changelog

• Implemented wells(), wells_by_name(), rows(), columns(), rows_by_name(), columns_by_name()
• Labware now caches the labware definition as well as wells_by_name(), rows_by_name(), columns_by_name()
• Implemented a few Well property methods
• Added & updated tests

Review requests

• The caching of labware definition & well properties during labware initialization makes stubbing Labware a bit more involved but still feels right to me since some of the well access methods are a bit expensive (potentially more for labware with lots of wells, esp when accessed many times in a protocol). But open to other thoughts.
  Update: After much discussion, decided caching is the right approach but caching in __init__ or using lru_cache isn't because the former makes test fixtures unnecessarily involved while lru_cache creates problems in testing if a Labware's id is not consistent (as the method refers to self). So, decided to make all cached methods use lazy properties which solves all testing issues.

• Related to above, the protocol context load_labware tests now fail because we don't mock out Labware class. What's the best approach to refactoring these tests? Not an issue anymore because of lazy properties.

Risk assessment

Medium impact on PE. None on non-P.E code

[codecov]

Codecov Report

Merging #8151 (c5ac262) into edge (73bd30c) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #8151      +/-   ##
==========================================
+ Coverage   87.20%   87.24%   +0.04%     
==========================================
  Files         445      427      -18     
  Lines       22697    22501     -196     
==========================================
- Hits        19793    19632     -161     
+ Misses       2904     2869      -35     

Impacted Files	Coverage Δ
opentrons/protocol_engine/state/labware.py	100.00% <0.00%> (ø)
notify-server/notify_server/__init__.py
.../notify_server/models/hardware_event/door_state.py
notify-server/notify_server/models/payload_type.py
...er/notify_server/models/hardware_event/__init__.py
notify-server/notify_server/main.py
notify-server/notify_server/server/__init__.py
notify-server/notify_server/clients/serdes.py
notify-server/notify_server/network/connection.py
notify-server/notify_server/logging.py
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c47f712...c5ac262. Read the comment docs.

[sanni-t]

what kind of Exception do we want here? Make a custom LabwareDefinitionNotFound error?

[mcous]

labware.get_labware_definition (which calls labware.get_labware_data_by_id and labware.get_definition_by_uri) will already raise if the labware ID or labware definition isn't found. For example:

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

Lines 144 to 151 in d9c7ed2

	def get_definition_by_uri(self, uri: LabwareUri) -> LabwareDefinition:
	"""Get the labware definition matching loadName namespace and version."""
	try:
	return self._state.labware_definitions_by_uri[uri]
	except KeyError:
	raise errors.LabwareDefinitionDoesNotExistError(
	f"Labware definition for matching {uri} not found."
	)

[mcous]

Some early initial feedback

[mcous]

labware.get_labware_definition (which calls labware.get_labware_data_by_id and labware.get_definition_by_uri) will already raise if the labware ID or labware definition isn't found. For example:

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

Lines 144 to 151 in d9c7ed2

	def get_definition_by_uri(self, uri: LabwareUri) -> LabwareDefinition:
	"""Get the labware definition matching loadName namespace and version."""
	try:
	return self._state.labware_definitions_by_uri[uri]
	except KeyError:
	raise errors.LabwareDefinitionDoesNotExistError(
	f"Labware definition for matching {uri} not found."
	)

[sanni-t]

So, I couldn't find a way to avoid doing pattern matching for rows because we refer to rows with row letters. So, in a situation where wells.ordering deviates from our expected naming pattern of letter+number, e.g: [['ab', 'cd'], ['wx', 'yz']], the columns would be parsed easily with a {'1': ['ab', 'cd'], '2': ['wx', 'yz']}, but for rows, a {'A': ['ab', 'wx'], 'B': ['cd', 'yz']} wouldn't make sense, and, doing something like {1: ['ab', 'wx'], 2: ['cd', 'yz']} would totally alter the behavior of rows_by_name()

[sanni-t]

So, to me, it seems like we need to stick to the r'^([A-Z]+)([0-9]+)$' pattern and expect the well definition to have wells named according to that pattern.
In which case, it feels a bit weird to have different ways of parsing rows and columns but I can get used to it, esp since column parsing like this (using well.ordering instead of pattern matching) is much easier.

[mcous]

Could we maybe add a TODO to evaluate if there are any differences we can find between ordering and well names in our existing definition?

---

Edit: I re-read this implementation more fully, and I think this is a good compromise for now

[sanni-t]

Note from call:
Will be keeping this cached in __init__ as is for now but will probably need to be changed to a lazy property once we start adding more tests that access wells.

[mcous]

🪂

[mcous]

Could we maybe add a TODO to evaluate if there are any differences we can find between ordering and well names in our existing definition?

---

Edit: I re-read this implementation more fully, and I think this is a good compromise for now

[mcous]

Should this compile be at the module level?

[sanni-t]

Since the regex's used only in this function, I like it being contained in here.

[mcous]

Ok to leave around for now, but this assert makes me feel a little weird

[SyntaxColoring]

Reviewed in a call with @mcous and @sanni-t.