Add a Task class specialised for Django#8491
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8491 +/- ##
==========================================
+ Coverage 81.24% 81.25% +0.01%
==========================================
Files 148 149 +1
Lines 18542 18553 +11
Branches 3165 3166 +1
==========================================
+ Hits 15064 15075 +11
Misses 3191 3191
Partials 287 287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ab51725 to
01de765
Compare
I might not have phrased it in the best way possible (especially in the PR description). I would like it to be well explained in the docs though, and I'm trying to show it with a simple example. Let me know if that helps... You're right: it may or may not be desired, but in my experience, it was not desired in the vast majority of the cases. |
yes you can go for it. use that model and use a more nice example.... |
|
Nice!! Perhaps this needs revisiting now that Django v5.0.1 is shipping. |
auvipy
left a comment
There was a problem hiding this comment.
do you plan to add anything more to this PR?
|
No, I don't think so. |
|
I will wait for other team mates opinion before merge |
I'll check with @thedrow if he's available for another review. Will do myself as well. |
this failed only after merged. it was OK during last run in the PR tox: 3.9-smoke scheduling tests via LoadScopeScheduling t/smoke/tests/test_tasks.py::test_replace::test_sanity[celery_setup_worker-celery_redis_broker-celery_redis_backend] |
|
I saw the error and was wondering if it was some flakyness? Seems like the smoke tasks tests worked on other python versions... 🤔 Do we know where this comes from? |
Checkout the latest run. It was indeed a hiccup. I already restarted the CI yesterday :) |
hiccup a.k.a Redis container hiccups causes Celery worker to hang -> CI reaches timeout. |
|
Thanks for this improvement! |
|
This broke cases when class CeleryApp(Celery):
registry_cls = 'tenant_schemas_celery.registry:TenantTaskRegistry'
task_cls = 'tenant_schemas_celery.task:TenantTask'The check only looks at the value passed in the c-tor, instead of the attribute being overriden, too. |
|
Oh I didn't think about this use case. I think it should be supported out of the box. Do you think you could write a patch for Celery? If not, could at least open an issue for it, and @ me into it, please? This description is clear enough to me, I think. |
|
@browniebroke I will look into preparing an MR, will let you know if I fail :) |
|
@maciej-gol I started working on a fix here: #9038 |
* Add test case for customized task as class attribute with Django As reported in #8491 (comment) * Fix detection of customized task as class attribute with Django --------- Co-authored-by: Asif Saif Uddin <auvipy@gmail.com>
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
Add a task specialised to use with Django. Offers an API to call
.delay()on tasks at the end of the database transaction.Rationale
A common pitfall using Celery with Django is to call
.delay()in a middle of a database transaction and have the task start executing before the transaction is committed. I think it's partly due to the fact that doing the call properly is a bit cumbersome.This PR adds a specialised task that aim at offering a nicer API for it. I've added the initial code and some docs to explain how it would be used, but before going further, I'd like to know whether this would be a welcome addition or if it's considered too out of scope.