Preserve order of return values from groups by lpsinger · Pull Request #4858 · celery/celery

lpsinger · 2018-06-27T22:32:32Z

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.

auvipy

unit tests are failing, so need change in tests?

lpsinger · 2018-06-30T17:00:37Z

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

codecov · 2018-07-09T20:05:43Z

Codecov Report

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

@@            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 · 2018-07-10T04:32:47Z

@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.

Fixes celery#3781.

lpsinger · 2019-01-31T18:27:52Z

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

This is a temporary workaround for celery/celery#4858.

georgepsarakis

@lpsinger really nice work!

celery/canvas.py

celery/worker/request.py

celery/backends/redis.py

georgepsarakis · 2019-02-01T20:00:31Z

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

lpsinger · 2019-02-01T23:25:47Z

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 · 2019-02-02T05:21:25Z

@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

can you resume the effort? rebase needed

maybe-sybr · 2020-07-09T07:43:22Z

@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 · 2020-07-09T07:53:11Z

@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 · 2020-07-09T13:19:06Z

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

maybe-sybr · 2020-07-10T01:38:13Z

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.

lpsinger force-pushed the preserve-group-order branch from 026c813 to a1a3abc Compare June 27, 2018 23:46

auvipy reviewed Jun 28, 2018

View reviewed changes

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

lpsinger force-pushed the preserve-group-order branch from 4db6d6b to 5d231ed Compare July 9, 2018 20:05

georgepsarakis added Component: Canvas Component: Redis Results Backend Category: Results Backend labels Jul 10, 2018

auvipy added the Status: Design Decision Needed ✘ label Jul 10, 2018

lpsinger force-pushed the preserve-group-order branch 2 times, most recently from e0d8a1a to 511c7f1 Compare November 28, 2018 16:23

lpsinger force-pushed the preserve-group-order branch 2 times, most recently from d9b78cd to 605adfa Compare December 10, 2018 18:54

lpsinger added 2 commits January 31, 2019 13:25

Preserve order of return values from groups

8b85f9a

Fixes celery#3781.

Update for zadd arguments changed in redis-py 3

32ed0da

lpsinger force-pushed the preserve-group-order branch from 605adfa to 32ed0da Compare January 31, 2019 18:26

lpsinger added a commit to lpsinger/gwcelery that referenced this pull request Jan 31, 2019

Add ordered_group task to preserve group result order

02ffb2a

This is a temporary workaround for celery/celery#4858.

georgepsarakis reviewed Feb 1, 2019

View reviewed changes

celery/canvas.py Outdated Show resolved Hide resolved

celery/worker/request.py Outdated Show resolved Hide resolved

celery/backends/redis.py Outdated Show resolved Hide resolved

lpsinger added 4 commits February 1, 2019 17:50

Use more explicit loop variable name

862bf26

Handle group_index not set

02e18e5

Use zrange instead of zrangebyscore

5b5c34c

Set flake8 version to 3.7.1

aa9b61a

auvipy added this to the 4.5 milestone May 10, 2019

auvipy requested changes May 10, 2019

View reviewed changes

maybe-sybr mentioned this pull request Jul 10, 2020

Preserve order of group results with Redis result backend #6218

Merged

auvipy closed this Jul 10, 2020

auvipy modified the milestones: 4.5, 4.4.x Jul 10, 2020

maybe-sybr mentioned this pull request Aug 5, 2020

fix flaky test_add_chord_to_chord #6263

Merged

Uh oh!

Conversation

lpsinger commented Jun 27, 2018

Description

Example

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

lpsinger commented Jun 30, 2018

Uh oh!

codecov bot commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

georgepsarakis commented Jul 10, 2018

Uh oh!

lpsinger commented Jan 31, 2019

Uh oh!

georgepsarakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepsarakis commented Feb 1, 2019

Uh oh!

lpsinger commented Feb 1, 2019

Uh oh!

georgepsarakis commented Feb 2, 2019

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

maybe-sybr commented Jul 9, 2020

Uh oh!

auvipy commented Jul 9, 2020

Uh oh!

lpsinger commented Jul 9, 2020

Uh oh!

maybe-sybr commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jul 9, 2018 •

edited

Loading