aquariumbio / aquarium

The Aquarium Lab Operating System
http://klavinslab.org/aquaverse/
MIT License
58 stars 15 forks source link

incorrectly create 2 pending jobs when launching a protocol #413

Open gnomicosuw opened 4 years ago

gnomicosuw commented 4 years ago

launching a copied design using a modified protocol creates 2 pending jobs

Repro

Encountered and reproduced various manifestations of this when going through the tutorials. (I added notes to the repro steps. I assume they are all related)

Developer View

  1. create the inital "StreakPlate" protocol in the tutorial (no inputs or outputs)

Designer View

  1. create a design with the StreakPlate operation. Call this "design 1"
  2. launch the design

Manager View

  1. schedule the design
  2. run it

RESULT: everything works as expected

Developer View

  1. update the protocol to include an input and an output (including adding the Sample Type Definitions and inventory)

Designer View

  1. select the previous design (see screenshot)

bug_40

  1. select "copy" (see screenshot)

bug_41

NOTE: at this stage the StreakPlate in the design does not have inputs and outputs. Trying to launch this design and schedule it and run it will result in an error.

  1. switch to [ design ] and select StreakPlate to update the StreakPlate in the copied design (see screenshot)

bug_42

  1. set the input and output so they turn green (see screenshot)

bug_43

  1. launch the plan (see screenshot)

bug_44

Manager View

  1. click on "pending" plans (see screenshot)

Actual Result: there are 2 plans (one without inputs/outputs and one with inputs/outputs) Expected Result: there should only be 1 plan (the one with inputs/outputs)

bug_45

NOTES:

ADDITIONAL NOTES:

MORE ADDITIONAL NOTES (I believe this is correct. I still need to verify this behavior)

bjkeller commented 4 years ago

@gnomicosuw Please add a title. Right now, this just shows up as [BUG] in the issue list.

gnomicosuw commented 4 years ago

I figured out the key issue. It was arguably user error.

In step 9 above selecting the streakplate adds a second streakplate to the design and places it directly on top of the first one in the GUI. Note the user can drag & drop this second streakplate and the first one is still there underneath. The user can delete the first one and then everything works as expected. (i.e., when you launch the plan there is only one pending job, and it works correctly)

DISCUSSION

There remains a related issue that protocols do not get "locked" when a plan is launched. So if a protocol changes after a plan is launched but before it is scheduled and run, then that plan could error out at runtime. For example:

REPRO STEPS

(a) create a plan with a protocol (call this version "v1") (b) launch the plan (note the plan was created with version "v1" of the protocol) (c) significantly change the protocol (such as add inputs and outputs, and call this version "v2") (d) schedule and run the plan that was launched in step (b)

RESULT

The plan will fail because the system will try to use the current version of the protocol when running the plan (in this case version "v2" with inputs and outputs), but the plan was created and launched using "v1" of the protocol (without inputs and outputs).

SUGGESTION

There is value to updating protocols and using the updated protocols whenever launching new plans (even if the new plans are edited copies of existing plans), so the current system architecture seems appropriate in the general case.

This issue should probably be closed and separated into 2 separate feature requests - each with probably lower priority.

1) Avoid adding new protocols directly on top of existing ones in the GUI (which led to the user error)

2) Keep a static copy of each protocol when designing and launching a plan, so that the plan can be scheduled and run as it would have run when the plan was actually designed and launched. However, this is a much bigger issue than it might appear to be, and opens up an entire discussion around supporting versioning of protocols.

dvnstrcklnd commented 4 years ago

There are two potentially undesirable behaviors here.

One is that protocols can be positioned one on top of the other. This has happened more than once to me.

Two is that changing inputs and outputs on already deployed protocols is something that should only be done very carefully. It isn't likely to result in anything good unless you make accompanying changes to the protocol code.