Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel workflow invocations when histories are deleted. #4580

Merged
merged 2 commits into from Sep 12, 2017

Conversation

Projects
None yet
3 participants
@jmchilton
Copy link
Member

commented Sep 8, 2017

With test case!

xref #3555 (third point)

@jmchilton jmchilton force-pushed the jmchilton:deleted_history branch from 1b3b064 to 09e5a5c Sep 8, 2017

@@ -156,6 +156,10 @@ def invoke(self):
# invocations.
return self.progress.outputs

if workflow_invocation.history.deleted:
log.info("Cacelled workflow evaluation due to delted history")

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 8, 2017

Member

s/Cacelled/Cancelled/
s/delted/deleted/

# Wait for all the datasets to complete, make sure the workflow invocation
# is not complete.
invocation = self._invocation_details(uploaded_workflow_id, invocation_id)
assert invocation['state'] != 'scheduled', invocation

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 8, 2017

Member

Maybe it would be helpful to use invocation['state'] in LIST_OF_POSSIBLE_STATES instead, because 'failed' would pass the assertion.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 8, 2017

Author Member

If failed passed the assertion, the cancelled check below would fail the test. If I make it a list of [new, ready] I would need to update the list if we add more non-terminal states (one could imagine adding queued or waiting states for instance). I could reuse stuff in galaxy.models to deal with state enumerations - but I'd ultimately like the API tests to not use any of Galaxy's internals (xref #4536). So I guess I slightly prefer the current variant - but I can make the change if you still would like to see it.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 8, 2017

Member

True, but:

  • if the state is already cancelled at this point, the test would not fail
  • it'd more difficult to debug the problem if the state here is failed, but the test fails only later.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 8, 2017

Author Member

I'm not sure I agree - but I've made a bunch of changes including this and I like the tests as a whole better now.

@galaxybot galaxybot added this to the 17.09 milestone Sep 8, 2017

@jmchilton jmchilton force-pushed the jmchilton:deleted_history branch 2 times, most recently from 283177f to c8e87a8 Sep 8, 2017


time.sleep(.5)
# Review the pasued steps to allow the workflow to continue.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 11, 2017

Member

s/pasued/paused/


return target_state_reached

def _assert_workflow_non_terminal(self, workflow_id, invocation_id):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 11, 2017

Member

Shouldn't the function name be _assert_invocation_non_terminal ?

jmchilton added some commits Sep 8, 2017

Cancel workflow invocations when histories are deleted.
With test case, rebased with spelling fixes thanks to @nsoranzo.

xref #3555 (third point)
Rework invocation state tracking in API tests.
At the insistence of @nsoranzo, be more explicit about waiting for workflow test in API tests. While I was in there I also setup new abstractions for waiting on state and updated various workflow cancelling tests to use test_history context for better error reporting.

@jmchilton jmchilton force-pushed the jmchilton:deleted_history branch from c8e87a8 to 7efe561 Sep 11, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2017

@nsoranzo Fixed up your last two comments and rebased, thanks for the catches.

@nsoranzo nsoranzo merged commit e1cdb74 into galaxyproject:dev Sep 12, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@nsoranzo

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

Thanks @jmchilton!

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2017

Thanks for detailed review and merge @nsoranzo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.