Opentrons / opentrons

Software for writing protocols and running them on the Opentrons Flex and Opentrons OT-2
https://opentrons.com
Apache License 2.0
415 stars 177 forks source link

PD: Webworker shouldn't get parallel requests #6265

Open IanLondon opened 4 years ago

IanLondon commented 4 years ago

Some multi-dispatch actions may trigger a COMPUTE_ROBOT_STATE_TIMELINE_REQUEST before any COMPUTE_ROBOT_STATE_TIMELINE_SUCCESS has happened

image

This has a potential for race condition bugs. We should find a way to guard against this, perhaps by making the timeline middleware interruptable??

IanLondon commented 3 years ago

Found a bug caused by this!

  1. Make a protocol, delete all the starting tipracks, add a Transfer. You'll get "insufficient tips" error
  2. Add a tiprack to the initial deck setup. The error doesn't appear to go away! (Invisibly, the error goes away for a millisecond and then comes back!)
  3. If you re-save the transfer step, or do something else to cause the timeline to recompute, the "insufficient tips" error will go away

Cause: When you add a tiprack, a thunk is dispatched which in turn dispatches 2 actions: CREATE_CONTAINER and RENAME_LABWARE.

The 2 actions are dispatched very rapidly, faster then timeline middleware's webworker can compute. So we get 2 calls to worker.postMessage

The 2 computations will finish out of order, as logged below. So the correct timeline is computed, but then erased by a computation based on stale data (via RENAME_LABWARE without seeing CREATE_CONTAINER)

This comes in first: the new timeline with the tiprack added and the error removed

worker result {"context":{"name":"","onmessage":null,"onmessageerror":null},"result":{"standardTimeline":{"timeline":[{"commands":[{"command":"pickUpTip","params":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","labware":"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1","well":"A1"}},{"command":"aspirate","params":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","volume":5,"labware":"trashId","well":"A1","offsetFromBottomMm":1,"flowRate":3.78}},{"command":"dispense","params":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","volume":5,"labware":"trashId","well":"A1","offsetFromBottomMm":0.5,"flowRate":3.78}},{"command":"dropTip","params":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","labware":"trashId","well":"A1"}}],"robotState":{"labware":{"trashId":{"slot":"12"},"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1":{"slot":"10"}},"modules":{},"pipettes":{"a5e801d0-35aa-11eb-a44f-31900ef05827":{"mount":"left"}},"liquidState":{"pipettes":{"a5e801d0-35aa-11eb-a44f-31900ef05827":{"0":{}}},"labware":{"trashId":{"A1":{}},"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1":{"H1":{},"G1":{},"F1":{},"E1":{},"D1":{},"C1":{},"B1":{},"A1":{},"H2":{},"G2":{},"F2":{},"E2":{},"D2":{},"C2":{},"B2":{},"A2":{},"H3":{},"G3":{},"F3":{},"E3":{},"D3":{},"C3":{},"B3":{},"A3":{},"H4":{},"G4":{},"F4":{},"E4":{},"D4":{},"C4":{},"B4":{},"A4":{},"H5":{},"G5":{},"F5":{},"E5":{},"D5":{},"C5":{},"B5":{},"A5":{},"H6":{},"G6":{},"F6":{},"E6":{},"D6":{},"C6":{},"B6":{},"A6":{},"H7":{},"G7":{},"F7":{},"E7":{},"D7":{},"C7":{},"B7":{},"A7":{},"H8":{},"G8":{},"F8":{},"E8":{},"D8":{},"C8":{},"B8":{},"A8":{},"H9":{},"G9":{},"F9":{},"E9":{},"D9":{},"C9":{},"B9":{},"A9":{},"H10":{},"G10":{},"F10":{},"E10":{},"D10":{},"C10":{},"B10":{},"A10":{},"H11":{},"G11":{},"F11":{},"E11":{},"D11":{},"C11":{},"B11":{},"A11":{},"H12":{},"G12":{},"F12":{},"E12":{},"D12":{},"C12":{},"B12":{},"A12":{}}}},"tipState":{"pipettes":{"a5e801d0-35aa-11eb-a44f-31900ef05827":false},"tipracks":{"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1":{"H1":true,"G1":true,"F1":true,"E1":true,"D1":true,"C1":true,"B1":true,"A1":false,"H2":true,"G2":true,"F2":true,"E2":true,"D2":true,"C2":true,"B2":true,"A2":true,"H3":true,"G3":true,"F3":true,"E3":true,"D3":true,"C3":true,"B3":true,"A3":true,"H4":true,"G4":true,"F4":true,"E4":true,"D4":true,"C4":true,"B4":true,"A4":true,"H5":true,"G5":true,"F5":true,"E5":true,"D5":true,"C5":true,"B5":true,"A5":true,"H6":true,"G6":true,"F6":true,"E6":true,"D6":true,"C6":true,"B6":true,"A6":true,"H7":true,"G7":true,"F7":true,"E7":true,"D7":true,"C7":true,"B7":true,"A7":true,"H8":true,"G8":true,"F8":true,"E8":true,"D8":true,"C8":true,"B8":true,"A8":true,"H9":true,"G9":true,"F9":true,"E9":true,"D9":true,"C9":true,"B9":true,"A9":true,"H10":true,"G10":true,"F10":true,"E10":true,"D10":true,"C10":true,"B10":true,"A10":true,"H11":true,"G11":true,"F11":true,"E11":true,"D11":true,"C11":true,"B11":true,"A11":true,"H12":true,"G12":true,"F12":true,"E12":true,"D12":true,"C12":true,"B12":true,"A12":true}}}},"warnings":[{"type":"ASPIRATE_FROM_PRISTINE_WELL","message":"Aspirating from a pristine well. No liquids were ever added to this well"}]}],"errors":null},"substeps":{"fb70edb0-3a32-11eb-b8bc-f3f24a57cf69":{"substepType":"sourceDest","multichannel":false,"commandCreatorFnName":"transfer","parentStepId":"fb70edb0-3a32-11eb-b8bc-f3f24a57cf69","rows":[{"volume":5,"activeTips":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","labware":"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1","well":"A1"},"source":{"well":"A1","preIngreds":{},"postIngreds":{}},"dest":{"well":"A1","preIngreds":{},"postIngreds":{"__air__":{"volume":5}}}}]}}}}
makeWorker.js?44ce:28

This comes in second, replacing the previous result and restoring the INSUFFICIENT_TIPS error (which should be gone!)

worker result {"context":{"name":"","onmessage":null,"onmessageerror":null},"result":{"standardTimeline":{"timeline":[],"errors":[{"type":"INSUFFICIENT_TIPS","message":"Not enough tips to complete action"}]},"substeps":{"fb70edb0-3a32-11eb-b8bc-f3f24a57cf69":{"substepType":"sourceDest","multichannel":false,"commandCreatorFnName":"transfer","parentStepId":"fb70edb0-3a32-11eb-b8bc-f3f24a57cf69","rows":[{"volume":5,"activeTips":{"pipette":"a5e801d0-35aa-11eb-a44f-31900ef05827","labware":"002eeaf0-3a33-11eb-b8bc-f3f24a57cf69:fixture/fixture_tiprack_10_ul/1","well":"A1"},"source":{"well":"A1","preIngreds":{},"postIngreds":{}},"dest":{"well":"A1","preIngreds":{},"postIngreds":{"__air__":{"volume":5}}}}]}}}}
IanLondon commented 3 years ago

What should timeline middleware do when it's already computing?

Ideally, cancel the webworker's task... ? At least, ignore its results! Definitely: avoid using stale data!

Memoization must respond correctly:

  1. prevTimelineArgs & prevFeatureFlags - derived from selectors AFTER the action passed into the middleware is given the opportunity to update the state. Used to determine timelineNeedsRecompute. If we're already computing, and we get an action that does NOT result in these being changed, then we don't need to recompute. If we're already computing, and we get an action that DOES result in these being changed, we DO need to recompute (and can ignore the in-progress computation if possible)

  2. prevSubstepsArgs - like the above, derived from selectors AFTER the action passed into the middleware is given the opportunity to update the state. Used to determine whether the substeps need to be recomputed (if the timeline needs to be recomputed, the substeps do too, but sometimes eg renaming a labware, we only need the substeps. Conditions:

    • In all cases, if we need to recompute the timeline, we necessarily need to recompute the substeps bc right now the timeline is an input into the substep generation process.
    • If we're already computing the timeline + substeps, and we get an action that does NOT result in substep args being changed (but don't need to recompute the timeline again), we don't have to recompute substeps.
    • Same as above if we're already computing just substeps.
    • If we're already computing the timeline + substeps, and we get an action that DOES change substep args, we need to recompute substeps. Ideally, we could wait for the in-progress timeline to finish, and use that to run the substeps.
    • If we're already computing just substeps, and we get an action that DOES change substep args, we need to recompute substeps. Ideally we could abort the current computation.

QUESTION: calculating the substeps alone will create a COMPUTE_ROBOT_STATE_TIMELINE_SUCCESS that contains the cached timeline. This can easily lead to stale data problems. Can we just exclude the timeline from the action when we were just using the cached one?

QUESTION: can we decouple substep generation so that it doesn't need the timeline as its input? It's regenerating a timeline anyway... why does it really need a timeline as input? Maybe these 2 things can be independent!

INVESTIGATE: instead of making users wait for the worker to finish an in-progress computation while we know we want to recompute anyway, can we interrupt it somehow to save time? This might mean making generateRobotStateTimeline + generateSubsteps allow interruption via some callback etc that checks if computation should continue. Or maybe there's something we can take advantage of in the webworker API itself?

  1. prevSuccessAction - unlike the above, which are results of selectors looking at the new state, this one is a way we cache the result of the computation. Really here we care about the payload, which is the result of the webworker's work. If we're already computing, this is STALE and should not be used.