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

Ensure paused tasks don't run #535

Merged
merged 3 commits into from
Jan 22, 2019
Merged

Ensure paused tasks don't run #535

merged 3 commits into from
Jan 22, 2019

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Jan 17, 2019

Currently, tasks that start off in a PAUSED state will be [correctly] treated as PENDING tasks and run.

This PR changes that so PAUSES do not pass the is_pending() check.

In order to run a PAUSE task, you must put it in a different state (like RESUME)

(depends on #541)

@cicdw
Copy link
Member

cicdw commented Jan 17, 2019

I'm on board with the end result of this PR, but I'm a little concerned that the is_pending pipeline method does something different than the is_pending State method --> should Paused be a subclass of Running?

@jlowin
Copy link
Member Author

jlowin commented Jan 18, 2019

You know, that’s an excellent suggestion. It would automatically work. Is it weird for Paused to be running, but Resume to be pending?

@cicdw
Copy link
Member

cicdw commented Jan 18, 2019

Is it weird for Paused to be running, but Resume to be pending?

It's quirky, but I think that having the states respond to the pipeline correctly without any special-casing is preferable, because that will ultimately make it easier to reason about stateful behavior. Definitely open to push back though if there are other edge cases I'm not considering!

@jlowin
Copy link
Member Author

jlowin commented Jan 20, 2019

So I do have a possible reason for Paused to remain a Pending subclass, which is that only Pending states have cached_inputs.

However, the task isn't actually run until it's put into a Resume state, so the Paused state's inputs don't actually matter -- in fact they need to be transferred to the Resume.

I think this ties in to our ongoing discussion about resulthandlers.

@cicdw
Copy link
Member

cicdw commented Jan 20, 2019

Hmm yea, good point! However, Paused states can also be created by a user raising a PAUSE signal inside a task, i.e., after a run has started. In this case we would need cached inputs.

@cicdw
Copy link
Member

cicdw commented Jan 20, 2019

Because of that, actually, I’m OK with leaving Paused states as subclasses of Pending and have a special case for it 👍🏻

@jlowin
Copy link
Member Author

jlowin commented Jan 21, 2019

Now rebased on top of #541

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great

@jlowin jlowin merged commit 35a73f5 into master Jan 22, 2019
@jlowin jlowin deleted the handle-pause branch January 22, 2019 03:35
cicdw added a commit that referenced this pull request Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants