Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.9k
Allow celery to spawn rather than fork in concurrency (Addresses #6036)#9810
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Draft
rbehal wants to merge6 commits intocelery:mainChoose a base branch
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
+423 −8
Conversation
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
* Add spawn worker pool* Fix tests* Fix lint issues* Add spawn to zsh completion
codecovbot commentedJul 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #9810 +/- ##==========================================+ Coverage 78.62% 78.63% +0.01%========================================== Files 153 154 +1 Lines 19199 19209 +10 Branches 2547 2547 ==========================================+ Hits 15095 15105 +10+ Misses 3811 3809 -2- Partials 293 295 +2
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
kdassharma approved these changesJul 9, 2025
kdassharma commentedJul 9, 2025
+1 Huge fix |
auvipy requested changesJul 9, 2025
Member
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
interesting approach. did you also checked the previous attempt#7612 ? also it would be great if integration tests dont fail. may be additional integration tests will be very helpful in this case
ContributorAuthor
rbehal commentedJul 9, 2025
Thanks, will check the integration tests -- a lot of them are just timing out, would that be an issue on my end still? I checked that PR, I do not think ALWAYS using spawn is the best approach. They say fork is fundamentally broken, but I don't believe this to be true, I think it makes sense for many usecases. What might make better sense though is for the DEFAULT concurrency approach to be using spawn rather than fork, since it's less likely to be broken in many cases. |
Member
auvipy commentedJul 9, 2025
time outs are network issues in general. I also checked the PR again. I quite agree with you. it would be great if you can add sufficient integration tests for this tests |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ContributorAuthor
rbehal commentedJul 9, 2025
Thanks will work on the integration tests in the coming days and let you know if I have any questions. We also just made a fork of celery for internal use as we intend on pushing this to production in the next couple days to fix some critical issues for us -- so I'll let you know if anything issues comes up there, and would make changes appropriately. |
ContributorAuthor
rbehal commentedJul 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I added a basic integration test -- to be honest I was having quite some difficulty running the integration tests locally. Was getting build issues on the docker container related to the versions of gevent / greenlet with certain Python versions. Will see how the CI handles it and goes from there. Also, I believe the change applied by co-pilot is incorrect, and led to breaking the unit test.
|
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR introduces a newspawn execution pool for Celery workers, allowing processes to be created with Python’sspawn start method instead offork. It adds the implementation, updates tests and shell completion, and extends documentation and CLI support.
- Add
spawn.TaskPoolbased onprefork.TaskPoolwithspawnstart method - Update unit and integration tests to include and verify
spawnbehavior - Extend CLI completions and documentation to list and describe the
spawnpool
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| t/unit/concurrency/test_spawn.py | New unit tests forspawn.TaskPool |
| t/unit/concurrency/test_concurrency.py | Addspawn to available pool names in existing tests |
| t/integration/test_spawn_pool.py | Integration test fixture and basic task test for spawn |
| extra/zsh-completion/celery.zsh | Addspawn to CLI pool completion |
| docs/userguide/concurrency/spawn.rst | New userguide page for spawn start method |
| docs/userguide/concurrency/index.rst | Includespawn in concurrency overview |
| docs/internals/reference/index.rst | Registercelery.concurrency.spawn in reference |
| docs/internals/reference/celery.concurrency.spawn.rst | Stub reference module page forspawn |
| docs/internals/guide.rst | Listspawn among internal pool implementations |
| docs/history/whatsnew-5.5.rst | Documentspawn pool in changelog |
| docs/getting-started/introduction.rst | Mentionspawn in introduction concurrency list |
| celery/concurrency/spawn.py | Implementation of the spawn-based TaskPool |
| celery/concurrency/init.py | Add'spawn' alias to concurrency registry |
Comments suppressed due to low confidence (2)
t/unit/concurrency/test_spawn.py:66
- Pytest will only discover test classes whose names start with
Test. Rename this class toTestSpawnTaskPool(or similar) to ensure the test runs.
class test_spawn_TaskPool:t/unit/concurrency/test_spawn.py:68
- The test patches
billiard.forking_enablebut does not assert its behavior. Consider adding an assertion (e.g.,mock_forking_enable.assert_called_once_with(False)or similar) to verify that forking is disabled when using thespawnstart method.
@patch('billiard.forking_enable')ContributorAuthor
rbehal commentedJul 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
On the last CI action, it looked like a few of the integration tests were timing out and a few of the smoke tests were failing for a reason I couldn't decipher. I'm having a lot of trouble running this locally -- when i try to build with docker-compose ( which seems unrelated to my changes, as I'm getting this from the Greatly appreciate your time in any case. |
Member
auvipy commentedJul 12, 2025
the greenlet issue is unrelated. you can ignore that for a while. we need some time to come back to this after celery 5.6 release |
* feat: add option to disable prefetch* Add CLI flag for disabling prefetch* fix disable-prefetch to respect autoscale max* Add unit tests
for more information, seehttps://pre-commit.ci
| - Updated rabbitmq doc about using quorum queues with task routes (#9707) | ||
| - Add: Dumper Unit Test (#9711) | ||
| - Add unit test for event.group_from (#9709) | ||
| - Allow disabling of broker prefetch with the ``worker_disable_prefetch`` |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is slated for 5.7 an unrelated to this PR
auvipy requested changesJul 19, 2025
Member
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you shouldnt have create two pr on same branch. specialy on the main branch. please switch to new branch for different PR
palmwi2010 commentedOct 15, 2025
Tested this PR in my project and it works great. I was getting SIGABRT crashes with Aspose.Slides (C++ library) before, switching to Would love to see this merged - thanks for the work on this! |
Member
Nusnus commentedNov 14, 2025
Is it possible to fix the conflicts? |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
5 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.
Uh oh!
There was an error while loading.Please reload this page.
Description
Addresses this issue:
#6036
Allows for a separate concurrency multi-processing method other than
preforkwhich isspawn. Usesspawnunder the hood rather thanfork.This is pretty much necessary to use scalable concurrent celery worker processes in many cases when using external libraries and SDKs that have their own specific behaviour for threading. We ran into issues a lot with
gcloudandgRPCfor example when using fork, which spawn completely fixes.