fix(canvas): add group index when unrolling tasks#8427
Merged
Nusnus merged 2 commits intocelery:mainfrom Aug 7, 2023
backmarket-oss:fix/add-group-index-when-unrolling-tasks
Merged
fix(canvas): add group index when unrolling tasks#8427Nusnus merged 2 commits intocelery:mainfrom backmarket-oss:fix/add-group-index-when-unrolling-tasks
Nusnus merged 2 commits intocelery:mainfrom
backmarket-oss:fix/add-group-index-when-unrolling-tasks
Conversation
This integration test ensures that tasks results are received in the same order as they were created when received by the callback.
When using `chord`, tasks results are not received in the order they were created. Setting the group index when unrolling tasks ensure that it is the case.
18 tasks
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #8427 +/- ##
=======================================
Coverage 87.42% 87.42%
=======================================
Files 148 148
Lines 18477 18477
Branches 3153 3153
=======================================
Hits 16153 16153
Misses 2034 2034
Partials 290 290
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Nusnus
approved these changes
Aug 7, 2023
Member
Nusnus
left a comment
There was a problem hiding this comment.
Well done! 💯
✅ High quality PR
✅ Thorough investigation
✅ Elegant fix + automatic test
Thank you for your contribution!
mkniewallner
added a commit
to backmarket-oss/celery
that referenced
this pull request
Aug 11, 2023
* test(integration): add test to assert `chord` order This integration test ensures that tasks results are received in the same order as they were created when received by the callback. * fix(canvas): add group index when unrolling tasks When using `chord`, tasks results are not received in the order they were created. Setting the group index when unrolling tasks ensure that it is the case.
mkniewallner
added a commit
to backmarket-oss/celery
that referenced
this pull request
Aug 11, 2023
* test(integration): add test to assert `chord` order This integration test ensures that tasks results are received in the same order as they were created when received by the callback. * fix(canvas): add group index when unrolling tasks When using `chord`, tasks results are not received in the order they were created. Setting the group index when unrolling tasks ensure that it is the case.
mkniewallner
added a commit
to backmarket-oss/celery
that referenced
this pull request
Aug 11, 2023
* ci: remove codecov * ci: run `python-package` on all PR branches * fix(canvas): add group index when unrolling tasks (celery#8427) * test(integration): add test to assert `chord` order This integration test ensures that tasks results are received in the same order as they were created when received by the callback. * fix(canvas): add group index when unrolling tasks When using `chord`, tasks results are not received in the order they were created. Setting the group index when unrolling tasks ensure that it is the case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
Fixes #8421.
As mentioned in the linked issue, since Celery 5.3.0, tasks results in
chordare not received by the callback in the same order as the tasks were created. This can be seen in backmarket-oss#1 that adds a test asserting the order. The root cause seems to be the removal of an apparently uselessziponresultshere. While removing it is a good call, thezipactually meant that we executed the generator, effectively settinggroup_indexto retrieve the results in the correct order.Although I wasn't sure where exactly would be the best place to fix the issue, I noticed that we could also set
group_indexwhile unrolling the tasks in agroup, effectively setting the correct order for the results, while keeping all tests green. This was made after an initial workaround in https://github.com/backmarket-oss/celery/pull/2 that was more a way to show the side effect that running the generator behindresultshad than a real attempt to fix the issue.I'm still not sure at this point if this is a proper way to fix the issue, so if you have other ideas in mind, I'm all ears.