VEuPathDB / web-eda

Web browser code for EDA-based applications
Apache License 2.0
0 stars 0 forks source link

Do not autostart jobs on configuration change #1600

Closed asizemore closed 1 year ago

asizemore commented 1 year ago

Observed that in an app (seen in multiple apps, both viz types), and i create a visualization, if i go to change params to a job that has not started, i go back to the original params, go back to the not-started params, that job is already done! So a job got autostarted somehow.

More specifically,

  1. Choose app, say alpha div box
  2. Choose params and run computation if necessary
  3. make plot
  4. Choose params that give a not started status
  5. Go back to params used for the plot. Pause a moment.
  6. Go back to the params in (4). You'll see the job is complete!
dmfalke commented 1 year ago

I think the reason this is happening is that in step 5, a request is made to the data service, even thought then job doesn't exist. My guess is that the data service is calling the compute service to get the job status, but without autostart=false, which would cause a job to be started. If this is true, I think the data service should be setting autostart=false to prevent the creation of the job. I also think the client should not request plot data when the job doesn't exist. My guess is that there is some intermediate state when rendering that causes the viz to get rendered, before the job status is known (by the client), this causing a request to be made to the data service. This likely happens with any job status. If we address that, then it will likely resolve this issue. But we should also address the issue with the data service, assuming I'm right.

dmfalke commented 1 year ago

The reason the visualization is being rendered in step 5 is that the compute and the computeJobStatus are out of sync during a render phase. This is happening because we are updating computeJobStatus in a useEffect hook. useEffect callbacks are called after the current data is rendered and committed to the DOM. This means that there is a render cycle in which the two values are out of sync (when the compute changes, and when the useEffect callback here is scheduled to run after the current data is rendered).

There are a few ways to remedy this, but the simplest is to use the computationId as a key prop for ComputationInstance. A good overview can be found here: https://beta.reactjs.org/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes.

dmfalke commented 1 year ago

This seems like an important bug to fix before release. Otherwise, we could end up with a lot of jobs that are never actually used. I'm going to self-assign.

SheenaTomko commented 1 year ago

This issue looks fixed. Could not test on Alpha Div because it would re-compute even without me telling it to. Tested on beta diversity and it looked good (although I found another bug which I reported here: https://github.com/VEuPathDB/service-eda-compute/issues/46).

asizemore commented 1 year ago

@SheenaTomko what do you mena by alpha div would re-compute without you telling it to?

SheenaTomko commented 1 year ago

Using Anopheles albimanus and alpha div box plot, if I set up parameters in step 1, hit "Generate Alpha Diversity results" in step 2, set up the plot in step 3, and then go back to step 1 and change either the data or the method, it automatically runs step 2 and turns green and then starts replotting the data in step 3 within a second of me changing the parameter in step 1

asizemore commented 1 year ago

when you change the data or method in step 1, do you see that job status as Not Started ever? Or it's only just green for Complete? Sounds like only Complete but i want to make sure, or there's another issue :)

SheenaTomko commented 1 year ago

I think it never shows as not started, it always shows as running (for a ms) or complete

asizemore commented 1 year ago

okay perfect. That's expected behavior. Thank you!