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

Add support for retrying tasks in admin #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gmuj
Copy link

@gmuj gmuj commented May 6, 2017

Hi, I have been using this code to retry tasks from admin within the old django-celery library. Maybe other people will find this useful.

Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

I like the idea of having a admin action like this in general, but the implementation isn't straight forward and may hide essential errors.

def retry_tasks(self, request, queryset):
with current_app.default_connection() as connection:
for state in queryset:
module_name, task_name = state.name.rsplit('.', 1)
Copy link
Member

Choose a reason for hiding this comment

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

This should use celery.utils.imports.symbol_by_name which Celery uses. It's effectively importing kombu's utility of the same name, but it's good to stick with the Celery version of it.

task.apply_async(**kwargs)
except TypeError:
# it must be a class task
task().apply_async(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, class based tasks are essentially dead in Celery 4.x, so it's better to have this raise a TypeError if something goes on during the call of apply_async.

if task:
kwargs = {
'args': ast.literal_eval(state.args),
'kwargs': ast.literal_eval(state.kwargs),
Copy link
Member

Choose a reason for hiding this comment

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

ast.literal_eval may work for simple arguments, but it's a different thing for more complex objects that can be serialized but may not be able to evaluated by the ast module. I wonder if there is a way for catching ValueError exceptions or whatever a faulty evaluation may cause and showing to the user in the admin that the retry didn't work because it couldn't parse the arguments? Alternatively, would using Celery's serialization system make this less brittle since we could use it to deserialize the argument values?

Copy link
Author

Choose a reason for hiding this comment

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

TaksState model has args and kwargs defined as TextField (https://docs.djangoproject.com/en/1.11/_modules/django/db/models/fields/#TextField), since args is tuple and kwargs is dictionary they will be converted to strings. An alternative will be to save them in database as json then we can easily decode them and not use literal_eval.

@codecov
Copy link

codecov bot commented May 9, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@5230e4c). Click here to learn what that means.
The diff coverage is 44.44%.

Impacted file tree graph

@@          Coverage Diff           @@
##             master    #2   +/-   ##
======================================
  Coverage          ?   73%           
======================================
  Files             ?    10           
  Lines             ?   426           
  Branches          ?    54           
======================================
  Hits              ?   311           
  Misses            ?   111           
  Partials          ?     4
Impacted Files Coverage Δ
django_celery_monitor/admin.py 53.07% <44.44%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5230e4c...5d69975. Read the comment docs.

@sameerkumar18
Copy link

In case someone is looking for a Django Admin Action to manually retry Celery tasks, check this

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.

3 participants