Skip to content
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

Makes regen less greedy #6589

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Makes regen less greedy #6589

merged 2 commits into from
Jan 12, 2021

Conversation

mjhoffman65
Copy link
Contributor

@mjhoffman65 mjhoffman65 commented Jan 11, 2021 •
edited

Might fix #4298. This was originally part of #6576 (#6576 (review)). Related to #6542

@mjhoffman65
Makes regen less greedy
f524566 
Might fix celery#4298.  This was originally part of celery#6576.
@mjhoffman65 mjhoffman65 mentioned this pull request Jan 11, 2021
Merged
@maybe-sybr
Copy link
Contributor

@mjhoffman65 - apologies for the delay in getting #6542 in so you didn't need to worry about it. Just checking in quickly, can you tell me if this is significantly different from #6542 in any way other than rolling in the feedback from @auvipy about __nonzero__() vs __bool__()? I'll try to get either this or #6542 merged in the next day or so.

@maybe-sybr maybe-sybr self-assigned this Jan 12, 2021
@auvipy
Copy link
Member

auvipy commented Jan 12, 2021

@mjhoffman65 - apologies for the delay in getting #6542 in so you didn't need to worry about it. Just checking in quickly, can you tell me if this is significantly different from #6542 in any way other than rolling in the feedback from @auvipy about __nonzero__() vs __bool__()? I'll try to get either this or #6542 merged in the next day or so.

you can push on this pr branch too. both of you get credit O:)

@mjhoffman65
Copy link
Contributor Author

@mjhoffman65 - apologies for the delay in getting #6542 in so you didn't need to worry about it. Just checking in quickly, can you tell me if this is significantly different from #6542 in any way other than rolling in the feedback from @auvipy about __nonzero__() vs __bool__()? I'll try to get either this or #6542 merged in the next day or so.

you can push on this pr branch too. both of you get credit O:)

@maybe-sybr @auvipy I don't think this is different than #6542 other than the __bool__ change. Sounds good, if there's anything I can do let me know. Feel free to push on this branch too if you go with this one. No worries either way :-)

@auvipy auvipy added this to the 5.0.6 milestone Jan 12, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jan 12, 2021

This pull request fixes 1 alert when merging f0d4f2e into f9b0231 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@auvipy auvipy self-requested a review January 12, 2021 16:50
@auvipy auvipy merged commit 7dc76ff into celery:master Jan 12, 2021
@mjhoffman65 mjhoffman65 deleted the regen branch January 12, 2021 17:36
@maybe-sybr
Copy link
Contributor

In which case I'll just note that we didn't change to using a yield from noted on the other PR but that's not particularly important. Good to see this get in, thanks for resubbing it @mjhoffman65 and for the merge @auvipy

@maybe-sybr maybe-sybr mentioned this pull request Jan 12, 2021
Closed
@thedrow
Copy link
Member

thedrow commented Jan 13, 2021

FYI, __nonzero__ is for Python 2.
We need to use __bool__, always.

In which case I'll just note that we didn't change to using a yield from noted on the other PR but that's not particularly important.

I missed that, I'll make another PR for it.

thedrow
thedrow reviewed Jan 13, 2021
View reviewed changes
t/unit/utils/test_functional.py
@@ -141,6 +141,7 @@ def build_generator():
self.consumed_second_item = False
g = regen(build_generator())
assert bool(g)
assert g[0] == 1
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the test. I wasn't sure this was working correctly :)

@thedrow thedrow mentioned this pull request Jan 13, 2021
Merged
@thedrow thedrow modified the milestones: 5.0.6, 5.1.0 Feb 24, 2021
@thedrow thedrow added this to Done in Celery 5.1.0 Feb 24, 2021
jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
@mjhoffman65 @jeyrce
Makes regen less greedy (celery#6589)
336edc6 
* Makes regen less greedy

Might fix celery#4298.  This was originally part of celery#6576.

* adds assertion to ensure regen item is not lost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thedrow thedrow thedrow left review comments

@auvipy auvipy auvipy approved these changes

Assignees

@maybe-sybr maybe-sybr

Labels
Component: Task Serialization PR Type: Bugfix
Projects
No open projects
Celery 5.1.0
  
Done
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

group.skew() removes all tasks if group input is generator
4 participants
@mjhoffman65 @maybe-sybr @auvipy @thedrow