8000 Preserve order of return values from groups by lpsinger · Pull Request #4858 · celery/celery · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Preserve order of return values from groups #4858

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Preserve order of return values from groups #4858

wants to merge 6 commits into from

Conversation

lpsinger
Copy link
Contributor

Description

Preserve the order of return values from groups under the redis backend. Fixes #3781.

Example

Here is test.py:

#!/usr/bin/env python
import random
import time

from celery import Celery, group

app = Celery('hello', broker='redis://', backend='redis://')


@app.task(shared=False)
def f(x):
    time.sleep(random.uniform(0, 1))
    return x


@app.task(shared=False)
def test():
    return (group(f.s(i) for i in range(10)) | f.s()).delay()


if __name__ == '__main__':
    app.start()

Start the worker like this:

$ ./test.py worker

And test it like this:

$ ./test.py shell
>>> test().get()
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

In previous versions of Celery, the return values would generally be out of order.

@lpsinger lpsinger force-pushed the preserve-group-order branch from 026c813 to a1a3abc Compare June 27, 2018 23:46
auvipy
auvipy reviewed Jun 28, 2018
View reviewed changes
Copy link
Member
@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

unit tests are failing, so need change in tests?

@lpsinger lpsinger force-pushed the preserve-group-order branch 4 times, most recently from 9cf9c72 to 4db6d6b Compare June 30, 2018 16:31
@lpsinger
Copy link
Contributor Author

The unit tests are passing now, but the integration tests are still failing.

@lpsinger lpsinger force-pushed the preserve-group-order branch from 4db6d6b to 5d231ed Compare July 9, 2018 20:05
@codecov
Copy link
codecov bot commented Jul 9, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4858   +/-   ##
=========================================
  Coverage          ?   82.91%           
=========================================
  Files             ?      142           
  Lines             ?    16175           
  Branches          ?     2022           
=========================================
  Hits              ?    13412           
  Misses            ?     2565           
  Partials          ?      198
Impacted Files Coverage Δ
celery/app/amqp.py 95.65% <ø> (ø)
celery/utils/abstract.py 25% <ø> (ø)
celery/app/base.py 62.6% <ø> (ø)
celery/backends/redis.py 91.36% <100%> (ø)
celery/worker/request.py 96.64% <50%> (ø)
celery/canvas.py 93.49% <71.42%> (ø)

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 7018d9e...aa9b61a. Read the comment docs.

@georgepsarakis
Copy link
Contributor

@lpsinger this is nice, but a rather extensive change. It also seems backwards incompatible, since the Redis structure is changed from List to Sorted Set, so it needs some more thought. I will have a look in the next few days, thanks.

@lpsinger lpsinger force-pushed the preserve-group-order branch 2 times, most recently from e0d8a1a to 511c7f1 Compare November 28, 2018 16:23
@lpsinger lpsinger force-pushed the preserve-group-order branch 2 times, most recently from d9b78cd to 605adfa Compare December 10, 2018 18:54
@lpsinger lpsinger force-pushed the preserve-group-order branch from 605adfa to 32ed0da Compare January 31, 2019 18:26
@lpsinger
Copy link
Contributor Author

We've been relying on this patch for several months in our project. Any update on reviewing it? Thanks!

lpsinger added a commit to lpsinger/gwcelery that referenced this pull request Jan 31, 2019
@lpsinger
Add ordered_group task to preserve group result order
02ffb2a 
This is a temporary workaround for
celery/celery#4858.
georgepsarakis
georgepsarakis reviewed Feb 1, 2019
View reviewed changes
Copy link
Contributor
@georgepsarakis georgepsarakis left a comment

Choose a reason for hiding this comment

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

@lpsinger really nice work!

@georgepsarakis
Copy link
Contributor

It seems that latest flake8 has incompatibilities. Would you mind setting the version to 3.7.1 so that the build proceeds?

@lpsinger
Copy link
Contributor Author
lpsinger commented Feb 1, 2019

It seems that latest flake8 has incompatibilities. Would you mind setting the version to 3.7.1 so that the build proceeds?

That had no effect. The flake8 warnings seem mostly unrelated. Should I revert that change?

@georgepsarakis
Copy link
Contributor

@lpsinger that is because the latest version is still installed. I was thinking to request the exact version (and I assume 3.7.1 will probably be compatible with Celery's codebase):

flake8==3.7.1

@auvipy auvipy added this to the 4.5 milestone May 10, 2019
auvipy
auvipy requested changes May 10, 2019
View reviewed changes
Copy link
Member
@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you resume the effort? rebase needed

@maybe-sybr
Copy link
Contributor

@auvipy - I've rebased this changeset onto the latest master and it still seems to function happily, resolving a problem I was having with group result ordering on top of Redis today. Is there still appetite to get this merged? If @lpsinger (/ping) isn't able, I can push an alternate PR and prep for merge.

@auvipy
Copy link
Member
auvipy commented Jul 9, 2020

@auvipy - I've rebased this changeset onto the latest master and it still seems to function happily, resolving a problem I was having with group result ordering on top of Redis today. Is there still appetite to get this merged? If @lpsinger (/ping) isn't able, I can push an alternate PR and prep for merge.

please take this

@lpsinger
Copy link
Contributor Author
lpsinger commented Jul 9, 2020

@maybe-sybr, I'm very happy for you to carry this forward. Thank you!

@maybe-sybr maybe-sybr mentioned this pull request Jul 10, 2020
Merged
@maybe-sybr
Copy link
Contributor

I've created #6218 to absorb the changes proposed here. The questions around changes to the Redis results backend typing are still relevant so I'll ping the commenter there to raise it again.

@auvipy auvipy closed this Jul 10, 2020
@auvipy auvipy modified the milestones: 4.5, 4.4.x Jul 10, 2020
@maybe-sybr maybe-sybr mentioned this pull request Aug 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgepsarakis georgepsarakis georgepsarakis left review comments

@auvipy auvipy auvipy requested changes

Assignees
No one assigned
Labels
Category: Results Backend Component: Canvas Component: Redis Results Backend Status: Design Decision Needed ✘
Projects
None yet
Milestone
4.4.x
Development

Successfully merging this pull request may close these issues.

Group tasks' results order getting messed up when using redis
4 participants
@lpsinger @georgepsarakis @maybe-sybr @auvipy
0