Deprecating stop_task: A Discussion

[EJH2]

Since it's inception, stop_task (in my opinion) has sat in a rather weird place:

• It has current and total arguments, neither of which matter because the progress is discarded for the default 100% once it "errors".
• The example use case shows catching an error, sending task metadata that the task has failed, and then ignoring the result
  • Is this not the same as simply letting the error propagate to the frontend?
  • If you have a desire to have a custom exception message, why not simply raise Exception(exception_message_string) or the like instead?
• stop_task doesn't actually stop the task on it's own, you HAVE to raise an exception anyways for it to stop. All stop_task does is tell any currently running bars to stop listening.
• An edge case presents itself in websocket progress bars if the task "failure" message fails to get through. The post-run handler will only receive information related to the "Ignored result", and will cause any progress bars that are listening to display an incorrect error if it fails to properly disconnect from the stop_task call (see Unsupported states crash Progress reporting #54).
  • This can be fixed with making the Ignore exception raise inside of stop_task, and pass it metadata that the post-run handler can try to decipher whether the Ignore exception means "Ignore", or "We actually mean FAILURE but we'd rather do it the hard way instead". This would require having to type-check every result that passes through the handler, and if it's an Ignore exception, look to see if specific metadata is present to then change the state and result to send to the progress bar. However, what's the point of doing that vs. actually just raising the error in the first place?

stop_task may have been a great idea at the time, but I feel that it's rather antiquated for the reasons listed above. Obviously, this would be a breaking change, so that must be taken into consideration. @czue @OmarWKH thoughts?

[czue]

I think you lay out a compelling case and I'm happy to deprecate/remove support for it. It's not a feature that I've ever had the need for, and I just merged in the enhancement since it seemed to be useful to others.

Re-reading that PR, it does seem like the core use case of providing input on a failure within the task is better handled through the ways you've outlined.

[eeintech]

The first time I used celery-progress, I got confused with stop_task. I thought it would nicely handle the killing of the task (eg. raise the exception) and freeze the progress bar displaying the error message. But it did none of those things so I quickly move away from it.
If it can be made more useful (eg. my initial expectation and/or more) I think it could be nice for novices like me to have a quick hook to stop the on-going progress.

[EJH2]

If we choose to keep stop_task, the only suitable solution that comes to mind would be something like this:

def stop_task(exc):
    raise (exc if isinstance(exc, Exception) else Exception(exc))

This would have the exact same effect as the what the previous implementation aimed to accomplish, with the caveat that the bar would still go to 100%. In order for us to maintain the current state of the progress bar vs filling it completely, we would have to make changes to the client-side JavaScript to somehow interpret that from the response, and we would have to modify how Progress.get_info sets the progress state for "completed" tasks to detect failures vs. successes, as right now both states just return the "completed" task state.

In the end, all of this is just extra work to offer a crutch for actually just raising the error yourself. Heck, it even takes more characters to type stop_task(exc=exc) instead of raise exc. ConsoleProgressRecorder's method doesn't even do anything! While I understand the valid concern for whether or not we should cater to entry-level programmers, I would think that if they've somehow managed to set up a Django webserver that requires progress bars, they would have at some point come across the try/except logic and exception raising.

[EJH2]

Although, for a possible counter-argument, keeping stop_task in the modified form would allow users to subclass the bar and add their own logic. So I guess the argument at that point becomes "it's a crutch for actual Python" vs. "user extendibility". Decisions, decisions...

[czue]

@EJH2 you have my support on whatever you think makes sense on this 😄

Sorry if I'm acting a bit hands off - I just don't have any strong opinions on this particular topic. Plus, I think you're doing a great job stewarding the lib and trust your judgment.

[OmarWKH]

I'm in favour of dropping stop_task and letting the user simply raise an exception.

The only use case for stop_task is if you want to ignore the task (which prevents state updates), but you also want to update the state and save the exception. Right? I'm not sure why you would want to do that.

[eeintech]

Another thought that came to my mind, stop_task may only makes sense if a start_task method existed. However, I don't think it should be the focus of the celery-progress utility, instead make it as simple as possible to manage the progress/status of a running task. If the two start and end points (creating/activating and ending/killing) stay true to Celery, then one should have no problem implementing those using Celery's documentation.

[EJH2]

It seems that the general consensus is fine with removing the method, so I'll put out a PR that corrects this.