Dequeue atomic

[AlbertoCasasOrtiz]

Fixes #267.

Most changes are due to indentation. Real changes are the addition of atomic to make everything a single transaction:

with transaction.atomic():

Select for update, which locks the trial so other workers cannot modify or lock them. If already locked by another worker, it skips it.

.select_for_update(skip_locked=True)

And a status check that, if for any reason a worker gets a trial already locked and in a non-available status, will skip it:

if trial.status not in ["stopped", "reprocess"]:
    raise Http404

[AlbertoCasasOrtiz]

@carmichaelong I just fixed conflicts on this branch

[csherry04]

@AlbertoCasasOrtiz It looks like maybe this second section got copied in with merge but it reassigning the things just assigned in the atomic transaction. The second version (not atomic) seems to maybe be the one with updated logic for the delayed uploads? Perhaps then can get rid of the first one and wrap the second one in an atomic transaction?

[AlbertoCasasOrtiz]

Thank you @csherry04. I just updated it accordingly (to avoid conflicts with dev and duplicated code, I modified from original code again to be atomic).

[AlbertoCasasOrtiz]

I just created a new test for this. Also, changed some database calls to be more efficient.

[carmichaelong]

Thanks @AlbertoCasasOrtiz. I've left some comments throughout. Happy to chat about strategies to address too if that's helpful some time.

Two other thoughts about the tests:

• Less related to this PR specifically but more generally for growing the tests, might be helpful to add a different test to make sure dequeue() returns the correct status codes under different conditions.
• If you choose to add the suggestion to order trials, could be good to add a test to make sure the order in which they are dequeue'ed is correct.

[carmichaelong]

Previously this used [0] to grab the first trial which would be caught and raise APIException which returns a 500 status code, which is handled in app.py.

Now, first() is used which can return None, and go down a different branch and return a 200 status code, which isn't handled yet.

This would slightly change the logic, but I'd suggest if first() returns None, perhaps we should also raise a 404 like above.

[carmichaelong]

It could be good to make sure this is ordered, perhaps by created_at (rather than updated_at)? Open to ideas of what might work best here.

[carmichaelong]

typo with a new line?

[carmichaelong]

patch() changes the object globally. Along with the wait time in delayed_save(), this likely affects worker 2's save() as well, which then may wait so long that worker 1 may not be in stopped state anymore.