-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
I'm on board with the end result of this PR, but I'm a little concerned that the |
You know, that’s an excellent suggestion. It would automatically work. 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! |
So I do have a possible reason for However, the task isn't actually run until it's put into a I think this ties in to our ongoing discussion about resulthandlers. |
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. |
Because of that, actually, I’m OK with leaving Paused states as subclasses of Pending and have a special case for it 👍🏻 |
Now rebased on top of #541 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
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)