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

Support for Django Model methods as tasks. #2600

Closed

Conversation

alexhayes
Copy link

I'm submitting this PR more for review and suggestions for possible future inclusion.

Essentially I wanted a way to easily turn Django Model methods into tasks.

Up until now I've used celery.contrib.methods.task and a @refresh decorator to ensure I don't have a stale model instance. However this approach meant the entire model is serialized and sent across the wire, obviously this is not very efficient and completely unnecessary considering that you then refresh the model in the worker... Obviously the solution outlined in the docs gets around both these issues however when you have an application that relies heavily on methods in models and those methods need to be turned into tasks, creating tasks for each is not very DRY and a pain when refactoring.

Thus, I wanted a solution that would only publish the minimum information required to the broker and using this data get a refresh model instance in the worker. The @model_task decorator achieves this by using base class DjangoModelBaseTask that relies on some slight refactoring of Task internals.

My thoughts/questions/comments thus far;

  1. I'm yet to confirm if the solution executes the retrieval of the model instance in the parent worker (which obviously should be avoided).
  2. Is the refactoring work in Task an acceptable approach?
  3. Naming of methods augment_args_for_run and augment_args_for_send
  4. Use of augment_args_for_run in celery/app/trace.py - note that previously it wasn't checking for self.__self__ but now it will be. Will this be a problem? Note, all tests are passing on python 2.7 & 3.4.
  5. The solution doesn't yet work for chains - I'm working on a solution in celery.canvas.Signature.
  6. The import of from django.db.models.loading import get_model occurs within DjangoModelBaseTask.augment_args_for_run so that an import error does not occur when building docs - not ideal...
  7. How to go about creating tests - this would probably require Django, which I imagine is not really desired.

I'd be happy to break DjangoModelBaseTask out into a separate project/repository which would get around the requirement for Django however it obviously relies on the slight refactoring of Task - it's also something that I imagine a lot of other users could benefit from (frankly, I can't believe I'm the first person to think of this...).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 59d5069 on alexhayes:feature/django-model-method-task into * on celery:3.1*.

@alexhayes
Copy link
Author

Commit e3650bf has a working apply_async implementation however apply is not working as yet.

I'm working on a potential solution.

@thedrow
Copy link
Member

thedrow commented Jun 1, 2015

What's the status here?

@malinoff
Copy link
Contributor

malinoff commented Jun 1, 2015

I don't think this will be merged, since @ask is saying that task methods are failed experience (and I'm completely agree with him).

@thedrow
Copy link
Member

thedrow commented Jun 1, 2015

Do you have an alternative implementation?

@alexhayes
Copy link
Author

Current status is I was waiting on input as to if task methods would definitely be removed in 3.2. I've spoken to @ask and he says that they won't be removed if the bugs can be sorted out.

So I don't forget...

===8<===
@alexhayes: is there any chance that you can be persuaded to not remove task methods in 3.2?
@ask: yeah, if the problems with it are fixed :)
@alexhayes: are there specific bugs?
@ask: like self not being set twice for retry
@alexhayes ok, well I'll make that part of my mission for #2600
@ask: task.self is just added as first argument in apply_async
@ask: no way to know if it's already "bound"
@alexhayes: Ok, I'll keep working on #2600 then with the view that it's definitely not going to get merged unless I can solve that problem
@ask: to solve it properly, it would probably need to be sent as a message header in the new task protocol
===>8===

@alexhayes
Copy link
Author

I'm closing this as I have created django-cereal which is a much better solution.

There are still a number of other issues, such as #2137, when using task methods but these aren't really related to this issue.

My focus now will be to fix issues related to task methods so they can be retained in 3.2.

@alexhayes alexhayes closed this Jun 19, 2015
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.

None yet

4 participants