Fix #9738 : Add root_id and parent_id to .apply() #9784
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9784 +/- ##
=======================================
Coverage 78.61% 78.62%
=======================================
Files 153 153
Lines 19172 19178 +6
Branches 2539 2540 +1
=======================================
+ Hits 15073 15079 +6
Misses 3807 3807
Partials 292 292
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
thanks dude |
There was a problem hiding this comment.
Pull Request Overview
This PR adds and verifies eager .apply() behavior by propagating parent_id and root_id on task requests.
- Introduces unit and integration tests for single-task and nested apply() calls, including handling when a parent has no
root_id. - Updates
Task.apply()to populateparent_idandroot_idin the eager request payload.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| t/unit/tasks/test_tasks.py | Added unit tests for eager .apply() ID propagation |
| t/integration/test_tasks.py | Added integration tests for eager .apply() ID propagation |
| celery/app/task.py | Populates parent_id and root_id in Task.apply() requests |
Comments suppressed due to low confidence (2)
t/integration/test_tasks.py:498
- Test classes need to be named with an uppercase
Testprefix (e.g.TestApplyTasks) for pytest to discover their methods.
class test_apply_tasks:
t/integration/test_tasks.py:498
- We should also add tests for canvas operations (e.g.
chain(...).apply(),group(...).apply(),chord(...).apply()) to ensure they setparent_idandroot_idcorrectly.
class test_apply_tasks:
|
is it possible to improve / increase unit test coverage? |
Ok I'll check the output of |
Actually I don't know how to increase coverage for these specific changes since they seems to bee all covered by new tests |
|
oh sorry no need |
@auvipy I've added tests for pr #9740.
fixes #9738
Original PR changes
parent_task.requestTests Added
t/unit/tasks/test_tasks.pyt/integration/test_tasks.pyThe tests are quite similar between unit and integration - we could avoid the copy-paste with a global conftest, but there isn't one and it's probably not necessary for this simple case.
Canvas Operations Limitation
However, the current solution won't work for canvas operations (chain, group, chord) like it does with
apply_async. For example:The issue is that canvas classes have their own apply() methods in canvas.py that don't establish the proper ID context before calling individual tasks. The fix only handles the basic task.apply() method, but canvas operations bypass this.
For full canvas support, the methods in
canvas.py::_chain::apply(),canvas.py::group::apply()andcanvas.py::_chord: apply(), classes would need similar updates to establish the ID hierarchy like theirapply_asyncmethods do.