Arvados

• crunch_dispatch should look for state=RunningOnServer (instead of active=true) when deciding which pipeline instances it should be trying to run
• Workbench pipeline instance index and show pages should look at state instead of active/complete flags when deciding what label to put on each pipeline instance
• Workbench pipeline instance "specify inputs and start" page (show?) should only enable the "start" button only if state is Ready or Paused -- and should only show the "choose inputs" page if state=New or state=Ready
• arv-run-pipeline-instance should set state=New when creating a pipeline instance
• arv-run-pipeline-instance should set state=RunningOnClient if --run-here is used and arv-run-pipeline-instance is in fact still running
• arv-run-pipeline-instance should set state=Paused before exiting, if --run-here is used (unless of course state=Failed or Complete by that point)
• arv-run-pipeline-instance should set state=RunningOnServer if --run-on-server was used
• API server should change from "New" to "Ready" when a pipeline instance is created or updated, if all required parameters are provided (unfortunately this duplicates some of the "ready to run?" logic from arv-run-pipeline-instance, for now)

• is_ready should be an instance method, and operate on the instance's own components. Could also call it components_look_ready? (might be more clear that it doesn't just test state==Ready or something like that). Then:
  

  - if PipelineInstance.is_ready pi.components
  + if pi.components_look_ready?

• Perhaps if there are no components, components_look_ready? should return false -- seems like state should probably stay at New instead of Ready in this case.
• Is there a reason to accept "false", "0", etc. as false? I prefer to expect required to be a genuine false. (This seems like a bad hole to start digging. If we really must have a list of values that are false, we should at least use the same list throughout the application. But I'd be more inclined to go with "if required is something other than true or false, the component is not correctly specified and therefore never considered ready, so we refuse to switch to RunningOnServer or Ready.")
• In tests, {"required" => "true"} should probably be {"required" => true} unless you're deliberately testing "other values that evaluate to true" behavior.
• Should test {"required" => false}
• Maybe add a little comment to the migration =begin / =end blocks to explain why they're commented out.
• In elsif state_changed? block
  • This is probably the part that will remain after we get rid of vestiges of success and active
  • Do something for Paused
  • Fail validation if someone updates to state="bogus"

• Should set :state => 'RunningOnClient' here?

• I think we should automatically state to "Ready" if the client sets/changes state to "New" but the components look ready? (If I'm reading it correctly, as it stands the client could create a new instance with components that look ready and set state to New, but the server would not admit that it thinks the instance is Ready until the client saves it again with different components or state. Perhaps change the last "elsif" to a distinct "if" block?)

Oh, and update PipelineInstance.queue to look for state = 'RunningOnClient' instead of active = true:

  def self.queue
    self.where('active = true')
  end

...then crunch-dispatch will leave RunningOnClient pipeline instances alone.

@ eff3277

Hm. What I wrote above doesn't make sense -- I should have said RunningOnServer, right? So instead of

self.where("active = true or state = 'RunningOnClient'")

we should have

self.where("state = ?", RunningOnServer)

(PipelineInstance.queue is only used by crunch_dispatch so it should only return RunningOnServer, not RunningOnClient -- and active=true is not necessary to check any more now that we have "state" -- right?)

Everything else looks good.

@ faf355b

If you start a pipeline, then stop it, it goes to "Paused" state, right? In that case I think this should use editable:true for either @object.state=='New' or @object.state=='Ready'

+  <% if @object.state == 'New' %>
+    <%= render partial: 'show_components_editable', locals: {editable: true} %>
+  <% else %>
+    <%= render partial: 'show_components_editable', locals: {editable: false} %>
+  <% end %>

Remove obsolete code instead of leaving it commented out (we always have git history if we want them back):

-      @instance[:active] = moretodo
+#      @instance[:active] = moretodo

In the "cleanup" function I think the main thing is to prevent "says RunningOnClient but in fact the client has stopped", so instead of

    if @instance
#      @instance[:active] = false
      @instance[:state] = 'New'  # should it be Failed?
      @instance.save
    end

maybe just

if @instance and @instance[:state] == 'RunningOnClient'
  @instance[:state] = 'Paused'
  @instance.save
end

(If the instance is in any other state when a-r-p-i crashes or gets ^C, it should probably be left alone.)

Things sure do look better with "state==X" than with "active && !success" etc.!