refactor(api): slightly faster simulation #9821

amitlissack · 2022-03-29T20:33:10Z

Overview

TLDR using caching to reduce simulation duration. These changes apply both to fast simulation and slow simulation.

While digging into #9503 I considered that maybe we're better off just improving the speed of slow simulation and doing away with fast simulation all together. Why? Because it seemed agonizing to have the PipetteDict produced by fast simulation match what is produced by the HC.

From profiling a Protocol that has thousands of aspirate/dispenses I learned that most of the SynchAdapter price was paid in the move_to function. It's called repeatedly. So I attempted to modify our InstrumentContextSimulation to have it inherit from InstrumentContextImplementation and ONLY override move_to. So basically, use the backward strategy from how fast sim had worked.

I spent a good deal of time profiling simulation and found sneaky little functions that are called zillions of times for no good reason. Comparing results on the PI I got:

  Slow Sim - 0:01:42.165820 
  New Fast sim - 0:00:56.065534
  Original Fast Sim - 0:00:33.754647

Pretty good, but not good enough.

So I fixed #9503 and am creating this PR to add the profiler learnings to improve simulation times.

Changelog

lru cache mathy functions. These are slow and called repeatedly.
Add thermocycler_present property to Deck. should_dodge_thermocycler is called zillions of times and does a linear search for thermocycler in deck map. We can cache this value.
BAD_PAIRS can be a set to avoid linear search

Review requests

Am I caching stuff that I shouldn't? Are the lru_cache limits reasonable?

I will add real life performance improvement metrics when I can.

Risk assessment

The thermocycler_present attribute is somewhat risky, though it is well covered by unit tests.
Otherewise. Low.

codecov · 2022-03-29T20:41:20Z

Codecov Report

Merging #9821 (b70ae8e) into edge (6ac3fc1) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             edge    #9821   +/-   ##
=======================================
  Coverage   75.00%   75.00%           
=======================================
  Files        2007     2007           
  Lines       53286    53286           
  Branches     5148     5148           
=======================================
  Hits        39967    39967           
  Misses      12300    12300           
  Partials     1019     1019

Flag	Coverage Δ
app	`71.52% <ø> (ø)`
components	`52.49% <ø> (ø)`
notify-server	`89.17% <ø> (ø)`
protocol-designer	`44.27% <ø> (ø)`
step-generation	`86.07% <ø> (ø)`

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

Impacted Files	Coverage Δ
api/src/opentrons/protocols/geometry/planning.py	`90.52% <ø> (ø)`
api/src/opentrons/hardware_control/pipette.py	`88.08% <100.00%> (ø)`
.../protocols/context/simulator/instrument_context.py	`91.20% <100.00%> (ø)`
api/src/opentrons/protocols/geometry/deck.py	`93.06% <100.00%> (ø)`

amitlissack · 2022-03-31T12:48:40Z

api/src/opentrons/hardware_control/instrument_handler.py

    def plunger_position(
        self, instr: Pipette, ul: float, action: "UlPerMmAction"
    ) -> float:
        mm = ul / instr.ul_per_mm(ul, action)
        position = mm + instr.config.bottom
        return round(position, 6)

+    @functools.lru_cache(PLUNGER_CACHE_LIMIT)
    def plunger_speed(
        self, instr: Pipette, ul_per_s: float, action: "UlPerMmAction"
    ) -> float:
        mm_per_s = ul_per_s / instr.ul_per_mm(instr.config.max_volume, action)


ul_per_mm calls piecewise_volume_conversion which does a decent bit of math.

These plunger_X methods can be called thousands of times with very small variance in inputs. The config values are immutable so caching seems like a save move.

amitlissack · 2022-03-31T12:50:43Z

api/src/opentrons/protocols/geometry/planning.py

@@ -61,7 +60,7 @@ def should_dodge_thermocycler(

    Returns True if we need to dodge, False otherwise
    """
-    if any([isinstance(item, ThermocyclerGeometry) for item in deck.values()]):


This method is called in each plan_moves which is called in each move_to. A needless search for a value that can be cached.

sfoster1

InstrumentHandler has the same lifetime as the hardware control api object and takes the attached instruments as inputs. That means that we need to clear the caches on the plunger position/speed/flowrate when the attached instruments change.

What might be a better idea is adding an LRU cache on hardware_control.pipettes.Pipette.ul_per_mm. Since that has the lifetime of an instrument, and it gets replaced when you change instruments, it won't need cache clearing, and it's where all the work happens.

I also think we could change the cache location for the deck slot key checking, but reasonable people can disagree.

api/src/opentrons/protocols/geometry/deck.py

amitlissack · 2022-03-31T14:07:21Z

InstrumentHandler has the same lifetime as the hardware control api object and takes the attached instruments as inputs. That means that we need to clear the caches on the plunger position/speed/flowrate when the attached instruments change.

What might be a better idea is adding an LRU cache on hardware_control.pipettes.Pipette.ul_per_mm. Since that has the lifetime of an instrument, and it gets replaced when you change instruments, it won't need cache clearing, and it's where all the work happens.

I also think we could change the cache location for the deck slot key checking, but reasonable people can disagree.

I'm not sure I understand you regarding InstrumentHandler. If the attached instruments change won't the instr arg to the plunger methods be different hence creating a new cache entry?

I was hoping to do exactly what you suggested, but lru cache requires hashable arguments. ul_per_mm and _check_names fail that test. I will continue working on this.

amitlissack · 2022-04-05T13:47:15Z

InstrumentHandler has the same lifetime as the hardware control api object and takes the attached instruments as inputs. That means that we need to clear the caches on the plunger position/speed/flowrate when the attached instruments change.
What might be a better idea is adding an LRU cache on hardware_control.pipettes.Pipette.ul_per_mm. Since that has the lifetime of an instrument, and it gets replaced when you change instruments, it won't need cache clearing, and it's where all the work happens.
I also think we could change the cache location for the deck slot key checking, but reasonable people can disagree.

I'm not sure I understand you regarding InstrumentHandler. If the attached instruments change won't the instr arg to the plunger methods be different hence creating a new cache entry?

I was hoping to do exactly what you suggested, but lru cache requires hashable arguments. ul_per_mm and _check_names fail that test. I will continue working on this.

I was wrong. It's piecewise_volume_conversion whose args cannot be hashed. Moving cache to ul_per_mm

mcous

Caching on a Deck method while the state of Deck can mutate underneath it makes me a little anxious. Is it necessary given the addition of the thermocycler_preset property?

mcous · 2022-04-05T13:44:26Z

api/src/opentrons/protocols/geometry/deck.py

@@ -75,6 +77,7 @@ def _assure_int(key: object) -> int:
        else:
            raise TypeError(type(key))

+    @functools.lru_cache(20)


Is this safe? It looks like self.data will mutate without busting the cache

mcous · 2022-04-05T13:46:57Z

api/src/opentrons/protocols/geometry/deck.py

+    def __hash__(self) -> int:
+        """A hash function as UserDict is unhashable."""
+        return id(self)


This feels iffy for the same reason the cache above feels iffy

amitlissack · 2022-04-05T14:03:13Z

Caching on a Deck method while the state of Deck can mutate underneath it makes me a little anxious. Is it necessary given the addition of the thermocycler_preset property?

I had initially put the lru_cache on _assure_int which is static, potentially called thousands of times, and harmless to cache. I moved the cache to _check_names without thinking it through.

sfoster1

Looks great to me!

* cache thermocycler presense to avoid linear search * cache check names * comments. more caching. * thermocycler_present attribute test and fix. * constants. * move lru_cache to ul_per_mm

amitlissack requested review from a team and X-sam and removed request for a team March 29, 2022 20:33

amitlissack requested a review from a team as a code owner March 29, 2022 20:33

amitlissack force-pushed the api-sim-faster branch 2 times, most recently from 8625a1c to b834c5a Compare March 30, 2022 20:13

amitlissack marked this pull request as draft March 30, 2022 20:52

amitlissack changed the title ~~WIP(api): faster simulation~~ refactor(api): faster simulation Mar 31, 2022

amitlissack requested review from a team and LaconicPneumonic and removed request for X-sam and LaconicPneumonic March 31, 2022 12:40

amitlissack force-pushed the api-sim-faster branch from ae64ade to 5f24c80 Compare March 31, 2022 12:42

amitlissack marked this pull request as ready for review March 31, 2022 12:45

amitlissack changed the title ~~refactor(api): faster simulation~~ refactor(api): slightly faster simulation Mar 31, 2022

amitlissack commented Mar 31, 2022

View reviewed changes

sfoster1 requested changes Mar 31, 2022

View reviewed changes

api/src/opentrons/protocols/geometry/deck.py Show resolved Hide resolved

mcous reviewed Apr 5, 2022

View reviewed changes

amit lissack added 6 commits April 5, 2022 09:56

cache thermocycler presense to avoid linear search

707f08b

cache check names

6a2cb65

comments. more caching.

8c2604f

thermocycler_present attribute test and fix.

e33ea74

constants.

e3fdba8

move lru_cache to ul_per_mm

b70ae8e

amitlissack force-pushed the api-sim-faster branch from 58a65cd to b70ae8e Compare April 5, 2022 14:00

amitlissack requested review from sfoster1 and mcous April 5, 2022 14:11

mcous approved these changes Apr 6, 2022

View reviewed changes

sfoster1 approved these changes Apr 6, 2022

View reviewed changes

amitlissack merged commit 3bffb41 into edge Apr 6, 2022

amitlissack deleted the api-sim-faster branch April 6, 2022 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(api): slightly faster simulation #9821

refactor(api): slightly faster simulation #9821

amitlissack commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

amitlissack Mar 31, 2022

amitlissack Mar 31, 2022

sfoster1 left a comment

amitlissack commented Mar 31, 2022 •

edited

Loading

amitlissack commented Apr 5, 2022

mcous left a comment

mcous Apr 5, 2022

mcous Apr 5, 2022

amitlissack commented Apr 5, 2022

sfoster1 left a comment

refactor(api): slightly faster simulation #9821

refactor(api): slightly faster simulation #9821

Conversation

amitlissack commented Mar 29, 2022 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

amitlissack Mar 31, 2022

Choose a reason for hiding this comment

amitlissack Mar 31, 2022

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

amitlissack commented Mar 31, 2022 • edited Loading

amitlissack commented Apr 5, 2022

mcous left a comment

Choose a reason for hiding this comment

mcous Apr 5, 2022

Choose a reason for hiding this comment

mcous Apr 5, 2022

Choose a reason for hiding this comment

amitlissack commented Apr 5, 2022

sfoster1 left a comment

Choose a reason for hiding this comment

amitlissack commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

amitlissack commented Mar 31, 2022 •

edited

Loading