refactor(protocol-engine): split labware state into state/store/view classes #7873

mcous · 2021-06-04T00:12:30Z

Overview

This PR is part of some engine simplification work in advance of #7871. In order to make using, testing, and building on the ProtocolEngine state all around better, I'd like to shift things to the following structure moving forward:

State dataclass
- Canonical state
- Could be public, so Protocol Engine users could consume the state (maybe!)
- Frozen, to try to prevent stuff from shifting under us
Store class
- Holds the state as a property
- Handles commands and updates the state in reaction to commands
View class
- Is given the state to read
- Contains selector methods to return computed values from the state

The big changes from the previous system are:

The State and View class used to be the same thing
The shape of the State wasn't public

This PR implements this change only for the LabwareStore, while leaving everything else alone.

Why?

The GeometryState and MotionState are currently pulling double duty: they track state and they compute state from other stores
- Moving forward, I think that if something computes state from other stores, it should only exist as a view
- So, there will be a GeometryView but no GeometryState nor GeometryStore; ditto for motion
- You can see this code smell in the tests for the geometry and motion state right now
  - The tests heavily mix mocking as well as testing of actual logic, like geometry math
  - Because of this mix, many of the tests are simply mocks + restatement of the logic of the subject
Tests for stores that simply track their own state are hard to deal with
- Moving forward, testing can be split up into:
  - Test that the store spits out the right state dataclass given certain commands
  - Test that the view spits out the right output given the correct state input
- Highly modeled after Redux reducers and selectors, which generally don't suck for testing
As we hook the Protocol Engine into more and more things, I see two things on the horizon that I'd like to start preparing for while it's a little easier
- Dropping the state out of a ProtocolEngine and loading it into a new one (e.g. think session switching)
- It would be nice to not have to add selectors to ProtocolEngine state for every single client use-case imaginable; having a public (immutable) state dataclass could be a really useful escape hatch

Changes

Split LabwareState into LabwareState dataclass and LabwareView state selector interface
Move deck_definition state from GeometryStore to LabwareStore
- Move a few associated selectors from GeometryState to LabwareView (could move more, but decided to punt)
Build in a temporary facade or two to keep things running smoothly until the state/store/view pattern can propagate upwards

Review requests

Most of this PR is tests! But I'd recommend start with the tests to get a feel for the improvement (I think) this PR makes
Once you're done with the tests, probably check out the source code, too

Risk assessment

Very low. Might break some protocol engine stuff, but it's not being used yet so whatever

…ore, and view

codecov · 2021-06-04T00:17:29Z

Codecov Report

Merging #7873 (c2951bc) into edge (de05ed1) will increase coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #7873      +/-   ##
==========================================
+ Coverage   83.61%   83.95%   +0.33%     
==========================================
  Files         351      355       +4     
  Lines       21669    21954     +285     
==========================================
+ Hits        18119    18431     +312     
+ Misses       3550     3523      -27

Impacted Files	Coverage Δ
robot-server/robot_server/sessions/dependencies.py	`55.55% <0.00%> (-8.74%)`	⬇️
opentrons/protocol_engine/state/substore.py	`91.66% <0.00%> (-8.34%)`	⬇️
opentrons/protocol_engine/state/state_store.py	`95.91% <0.00%> (-2.05%)`	⬇️
opentrons/file_runner/abstract_file_runner.py	`71.42% <0.00%> (-1.30%)`	⬇️
robot-server/robot_server/service/dependencies.py	`91.57% <0.00%> (-1.15%)`	⬇️
opentrons/file_runner/__init__.py	`100.00% <0.00%> (ø)`
opentrons/file_runner/protocol_file.py	`100.00% <0.00%> (ø)`
opentrons/file_runner/json_file_runner.py	`100.00% <0.00%> (ø)`
opentrons/protocol_engine/state/labware.py	`100.00% <0.00%> (ø)`
opentrons/file_runner/python_file_runner.py	`100.00% <0.00%> (ø)`
... and 16 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 de05ed1...c2951bc. Read the comment docs.

mcous · 2021-06-14T16:35:27Z

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

-    """Basic labware data state and getter methods."""
+    """State of all loaded labware resources."""
+
+    labware_by_id: Dict[str, LabwareData]


I'm going to type these as Mapping instead, which is read-only at a type level. This won't give us runtime immutability of the dict, but I think the type safety is more than enough protection for now. (I'll do the same for any lists I find and set them as Sequence's)

Edit: made this change in #7882

mcous · 2021-06-14T19:49:03Z

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

+                load_name=command.result.definition.parameters.loadName,
+                version=command.result.definition.version,
+            )
+            self._state.labware_definitions_by_uri[uri] = command.result.definition


This should be treating self._state as immutable, but it is not. Fixed in #7882

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

SyntaxColoring

Mostly clarifying questions.

The rationale described in the PR description sounds great to me. Making the state dataclass public makes me a little nervous, but I don't think that matters right now.

Haven't made my way through all the tests, yet.

SyntaxColoring · 2021-06-15T15:57:47Z

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

+# TODO(mc, 2021-06-03): continue evaluation of which selectors should go here
+# vs which selectors should be in LabwareView


TBH, I'd be down to stuff both labware and non-labware geometry things into GeometryView.

Or even, maybe, absolutely everything under one ProtocolEngineView class. For example, this would ~~disallow~~ discourage mocking out geometry view stuff without also mocking out command view stuff, which is maybe a good thing?

I agree with both these points.

Moving more stuff out of GeometryView can be accomplished in another PR (it might even make GeometryView unnecessary)

refactor(protocol-engine): split top-level engine state into state/store/view classes #7882 adds a top-level StateView for external users. Mocking out subviews is useful (and necessary) for testing other subviews, but outside of ProtocolEngine everything will interact with the StateView classs

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

SyntaxColoring · 2021-06-15T16:24:45Z

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

+        Arguments:
+            state: Labware state dataclass used for all calculations.
+        """
+        self._state = state


If I'm reading this right:

LabwareState is frozen=True. This means LabwareStore will have to represent modifications to it as a rebinding of its self._state attribute, rather than as an in-place modification of that object. (Your comment above says it doesn't do this now, but it will in refactor(protocol-engine): split top-level engine state into state/store/view classes #7882).

A LabwareView is initialized with a LabwareState and never updated. So, a LabwareView will only provide a view of the state snapshotted at the LabwareView's __init__ time. The view won't automatically update.

e.g. if I wrote code like this, it would be wrong:

view = LabwareView(store.state) with pytest.raises(LabwareDoesNotExistError): view.get_labware_data_by_id("some-labware-id") # Nothing loaded yet. store.handle_completed_command(some_load_labware_command) assert view.get_labware_data_by_id("some-labware-id") == some_labware_data # Now it's loaded.

It would instead have to be:

with pytest.raises(LabwareDoesNotExistError): LabwareView(store.state).get_labware_data_by_id("some-labware-id") # Nothing loaded yet. store.handle_completed_command(some_load_labware_command) assert LabwareView(store.state).get_labware_data_by_id("some-labware-id") == some_labware_data # Now it's loaded.

Is this intentional?

Forgetting to update LabwareStore to rebind self._state in this PR was not intentional, and pushing it to #7882 was out of laziness and not wanting to do a whole rebase chain.

The LabwareState > LabwareView thing is intentional, though! In #7882, where all substores are converted, the top-level StateStore has the responsibility of updating the view whenever the underlying state dataclass needs to change:

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

Lines 74 to 81 in 272fb9d

def _update_state(self) -> None:

"""Set state data and view interface to latest underlying values."""

self._state = State(

commands=self._command_store.state,

labware=self._labware_store.state,

pipettes=self._pipette_store.state,

)

self._state_view = StateView(state=self._state)

api/tests/opentrons/protocol_engine/state/command_fixtures.py

api/tests/opentrons/protocol_engine/state/test_geometry_state.py

SyntaxColoring

Scrolling through the tests, it looks like they're all ports from the old class layout to the new one—i.e. we're not doing anything like adding new tests in this PR. If this is wrong, I might need a pointer to which tests I should focus on.

Otherwise LGTM. 👍

api/tests/opentrons/protocol_engine/state/test_labware_view.py

sanni-t

Reviewed in person
👍

mcous added 3 commits June 3, 2021 19:47

refactor(api): remork engine labware state into a state dataclass, st…

8e344cf

…ore, and view

fixup: revert formatting only file changes

a0fded1

fixup: remove Geometry store from command reactor list

952b4a4

mcous added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Jun 4, 2021

mcous requested review from a team and celsasser and removed request for a team June 4, 2021 00:12

mcous requested a review from a team as a code owner June 4, 2021 00:12

mcous changed the title ~~refactor(api): remork engine labware state into a state, store, and view~~ refactor(api): remork engine labware state into state/store/view Jun 4, 2021

SyntaxColoring changed the title ~~refactor(api): remork engine labware state into state/store/view~~ refactor(api): rework engine labware state into state/store/view Jun 4, 2021

This was referenced Jun 4, 2021

refactor(protocol-engine): split pipette state into state/store/view classes #7875

Merged

refactor(protocol-engine): split top-level engine state into state/store/view classes #7882

Merged

mcous added the protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs label Jun 11, 2021

mcous commented Jun 14, 2021

View reviewed changes

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

fixup: docstring fix

1674bca

SyntaxColoring reviewed Jun 15, 2021

View reviewed changes

SyntaxColoring approved these changes Jun 15, 2021

View reviewed changes

SyntaxColoring changed the title ~~refactor(api): rework engine labware state into state/store/view~~ refactor(protocol-engine): rework labware state into state/store/view classes Jun 15, 2021

SyntaxColoring changed the title ~~refactor(protocol-engine): rework labware state into state/store/view classes~~ refactor(protocol-engine): split labware state into state/store/view classes Jun 15, 2021

mcous commented Jun 15, 2021

View reviewed changes

fixup: fix test docstrings

c2951bc

sanni-t approved these changes Jun 15, 2021

View reviewed changes

mcous merged commit 26f61f2 into edge Jun 16, 2021

mcous deleted the api_engine-state-refactor branch June 16, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(protocol-engine): split labware state into state/store/view classes #7873

refactor(protocol-engine): split labware state into state/store/view classes #7873

mcous commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 4, 2021 •

edited

Loading

mcous Jun 14, 2021 •

edited

Loading

mcous Jun 14, 2021

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring Jun 15, 2021

SyntaxColoring Jun 15, 2021 •

edited

Loading

mcous Jun 15, 2021

SyntaxColoring Jun 15, 2021

mcous Jun 15, 2021

SyntaxColoring left a comment •

edited

Loading

sanni-t left a comment

		# TODO(mc, 2021-06-03): continue evaluation of which selectors should go here
		# vs which selectors should be in LabwareView

	def _update_state(self) -> None:
	"""Set state data and view interface to latest underlying values."""
	self._state = State(
	commands=self._command_store.state,
	labware=self._labware_store.state,
	pipettes=self._pipette_store.state,
	)
	self._state_view = StateView(state=self._state)

refactor(protocol-engine): split labware state into state/store/view classes #7873

refactor(protocol-engine): split labware state into state/store/view classes #7873

Conversation

mcous commented Jun 4, 2021 • edited Loading

Overview

Why?

Changes

Review requests

Risk assessment

codecov bot commented Jun 4, 2021 • edited Loading

Codecov Report

mcous Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

mcous Jun 14, 2021

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

SyntaxColoring Jun 15, 2021

Choose a reason for hiding this comment

SyntaxColoring Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

mcous Jun 15, 2021

Choose a reason for hiding this comment

SyntaxColoring Jun 15, 2021

Choose a reason for hiding this comment

mcous Jun 15, 2021

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

mcous commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 4, 2021 •

edited

Loading

mcous Jun 14, 2021 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring Jun 15, 2021 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading