Switch between thread & global for app.backend #7961

chenseanxy · 2022-12-15T14:43:54Z

Note: Before submitting this pull request, please review our contributing
guidelines.

Description

For app.backend, switch between self._local and self._global as cache, depending
on whether the environment is fully thread-safe (eg. eventlet), and whether the
backend is thread-safe (eg. Redis)

Fixes:

- Fixes celery#7960 and celery#6819

for more information, see https://pre-commit.ci

codecov · 2022-12-15T14:51:52Z

Codecov Report

Base: 90.15% // Head: 90.14% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (28470a0) compared to base (8a92e0f).
Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7961      +/-   ##
==========================================
- Coverage   90.15%   90.14%   -0.01%     
==========================================
  Files         138      138              
  Lines       17091    17104      +13     
  Branches     2312     2712     +400     
==========================================
+ Hits        15408    15419      +11     
- Misses       1430     1431       +1     
- Partials      253      254       +1

Flag	Coverage Δ
unittests	`90.12% <88.88%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
celery/app/base.py	`96.36% <86.66%> (-0.31%)`	⬇️
celery/backends/base.py	`93.03% <100.00%> (+0.01%)`	⬆️
celery/backends/redis.py	`92.79% <100.00%> (+0.01%)`	⬆️
celery/worker/worker.py	`97.74% <100.00%> (+0.01%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

auvipy · 2022-12-15T14:55:37Z

celery/app/base.py

@@ -1,5 +1,6 @@
 """Actual App instance implementation."""
 import inspect
+import multiprocessing


as celery use billiard which is a fork of multiprocessing,, should we use multiprocessing here? can multiprocessing.current_process() imported from billiard?

Did not realize it was a thing, will switch to billiard

Meanwhile fixed flake8 (cannot open another fork atm, committed here) PyCQA/flake8#1689

- Flake8 6.0 PyCQA/flake8#1689

auvipy · 2022-12-15T15:40:28Z

Not sure if the the failure is related
=========================== short test summary info ============================
FAILED t/integration/test_canvas.py::test_chain::test_simple_chain - celery.e...
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
==== 1 failed, 4 passed, 2 skipped, 2 xfailed, 5 rerun in 381.31s (0:06:21) ====

chenseanxy · 2022-12-15T15:43:35Z

Hmmm will have to look into this during the weekends.. Getting timeouts left right and center with only Redis, seems suspicious

chenseanxy · 2022-12-15T17:29:30Z

Interestingly they do pass when run locally:

developer@cf668022eb6c:~/celery$ TEST_BROKER=redis://redis TEST_BACKEND=redis://redis PYTEST_PLUGINS=celery.contrib.pytest pytest -xvs t/integration -k test_chain
=================================================================================== test session starts ====================================================================================
platform linux -- Python 3.9.9, pytest-7.2.0, pluggy-1.0.0 -- /home/developer/.pyenv/versions/python3.9/bin/python3.9
cachedir: .pytest_cache
rootdir: /home/developer/celery, configfile: pyproject.toml
plugins: order-1.0.1, cov-4.0.0, subtests-0.9.0, github-actions-annotate-failures-0.1.7, timeout-2.1.0, rerunfailures-10.3, click-1.1.0
collected 206 items / 157 deselected / 49 selected                                                                                                                                         

t/integration/test_canvas.py::test_chain::test_simple_chain PASSED
t/integration/test_canvas.py::test_chain::test_single_chain PASSED
t/integration/test_canvas.py::test_chain::test_complex_chain PASSED
t/integration/test_canvas.py::test_chain::test_group_results_in_chain XFAIL (Task is timeout)
t/integration/test_canvas.py::test_chain::test_chain_of_chain_with_a_single_task PASSED
t/integration/test_canvas.py::test_chain::test_chain_on_error PASSED
t/integration/test_canvas.py::test_chain::test_chain_inside_group_receives_arguments PASSED
t/integration/test_canvas.py::test_chain::test_eager_chain_inside_task PASSED
t/integration/test_canvas.py::test_chain::test_group_chord_group_chain PASSED
t/integration/test_canvas.py::test_chain::test_group_result_not_has_cache PASSED
t/integration/test_canvas.py::test_chain::test_second_order_replace PASSED
t/integration/test_canvas.py::test_chain::test_parent_ids PASSED
... ...

Also re-tested with https://github.com/chenseanxy/celery-eventlet-backend-imports using eventlet, prefork and threads, all cases seem to be fine & fixed (backend only created once)

developer@921707809c8b:~/app$ celery -A app worker --loglevel=INFO -c 4 -P threads
[2022-12-15 17:25:25,213: INFO/MainProcess] Importing backend from redis://redis/0
 
 -------------- celery@921707809c8b v5.3.0b1 (dawn-chorus)
--- ***** ----- 
-- ******* ---- Linux-5.10.0-19-amd64-x86_64-with-glibc2.31 2022-12-15 17:25:25
- *** --- * --- 
- ** ---------- [config]
- ** ---------- .> app:         tasks:0x7fb9720ac190
- ** ---------- .> transport:   redis://redis:6379/0
- ** ---------- .> results:     redis://redis/0
- *** --- * --- .> concurrency: 4 (thread)
-- ******* ---- .> task events: OFF (enable -E to monitor tasks in this worker)
--- ***** ----- 
 -------------- [queues]
                .> celery           exchange=celery(direct) key=celery
                

[tasks]
  . app.add

[2022-12-15 17:25:25,257: INFO/MainProcess] Connected to redis://redis:6379/0
[2022-12-15 17:25:25,258: INFO/MainProcess] mingle: searching for neighbors
[2022-12-15 17:25:26,265: INFO/MainProcess] mingle: all alone
[2022-12-15 17:25:26,276: INFO/MainProcess] celery@921707809c8b ready.
[2022-12-15 17:25:46,176: INFO/MainProcess] Task app.add[ea1f6167-d208-48c8-8087-c1e9d271223d] received
[2022-12-15 17:25:46,179: INFO/MainProcess] Task app.add[681a18fe-aaa9-453e-8ec1-ec6598f6ef73] received
[2022-12-15 17:25:46,180: INFO/MainProcess] Task app.add[ea1f6167-d208-48c8-8087-c1e9d271223d] succeeded in 0.002875827999559988s: 3
[2022-12-15 17:25:46,182: INFO/MainProcess] Task app.add[05141eaa-1a2e-461c-b922-a8e94647001c] received
[2022-12-15 17:25:46,182: INFO/MainProcess] Task app.add[681a18fe-aaa9-453e-8ec1-ec6598f6ef73] succeeded in 0.0019039429998883861s: 3
[2022-12-15 17:25:46,183: INFO/MainProcess] Task app.add[3b489a71-bd74-4fa2-a7e3-8772636698f1] received
[2022-12-15 17:25:46,184: INFO/MainProcess] Task app.add[05141eaa-1a2e-461c-b922-a8e94647001c] succeeded in 0.0011963140004809247s: 3
[2022-12-15 17:25:46,185: INFO/MainProcess] Task app.add[fafeabc7-ed74-4053-8498-c763d2c36207] received
[2022-12-15 17:25:46,187: INFO/MainProcess] Task app.add[68903f90-ca1d-49ae-9d6e-fb9e63046b06] received
[2022-12-15 17:25:46,187: INFO/MainProcess] Task app.add[3b489a71-bd74-4fa2-a7e3-8772636698f1] succeeded in 0.003291294000518974s: 3
... ...

Wondering if it's some kind of issue with redis broker's setup in CI

Nusnus · 2022-12-15T17:33:49Z

Not sure if the the failure is related
=========================== short test summary info ============================
FAILED t/integration/test_canvas.py::test_chain::test_simple_chain - celery.e...
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
==== 1 failed, 4 passed, 2 skipped, 2 xfailed, 5 rerun in 381.31s (0:06:21) ====

I'm 99.9% positive this has nothing to do with this PR.
I'd clasify this as instability issues of the tests in general.

@chenseanxy
I've re-run only the failed runs so we can move on with this PR if all comments are handled :)

auvipy · 2022-12-15T17:36:39Z

we need to have input from more team members. so lets wait until next week

Nusnus · 2022-12-15T17:40:56Z

celery/app/base.py

@@ -1249,14 +1252,32 @@ def amqp(self):
        """AMQP related functionality: :class:`~@amqp`."""
        return instantiate(self.amqp_cls, app=self)

+    def inform_green(self, is_green):


Not a show-blocker, but I'd recommend adding a small unit test according to the code coverage warning

Nusnus · 2022-12-15T17:52:59Z

we need to have input from more team members. so lets wait until next week

I agree. Added @thedrow as a reviewer.

Also reviewed it myself and to my understanding it looks good.

@chenseanxy Your confirmation for the validity of the change is https://github.com/chenseanxy/celery-eventlet-backend-imports, correct?
Some more unit tests inside the Celery testing suite could ease this PR into merging, although I see what makes it look "ready". Give it a thought, see if there are useful tests you can add so in case this will be broken in the future by some other PR, we'll have a test that will let us know its broken as the current tests do not include the setup you used in celery-eventlet-backend-imports

Nusnus · 2022-12-15T17:57:32Z

Not sure if the the failure is related
=========================== short test summary info ============================
FAILED t/integration/test_canvas.py::test_chain::test_simple_chain - celery.e...
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
==== 1 failed, 4 passed, 2 skipped, 2 xfailed, 5 rerun in 381.31s (0:06:21) ====

I'm 99.9% positive this has nothing to do with this PR. I'd clasify this as instability issues of the tests in general.

@chenseanxy I've re-run only the failed runs so we can move on with this PR if all comments are handled :)

Changed my mind, all of the redis setups are failing specifically.
This is probably indeed due to something with the PR.

P.S
Threading indeed is hard to test so local runs aren't sufficient by themselves.. I'd even run the whole PR 3 times when all of the tests are passing, just to see if it wasn't luck and it passes consistently.

Nusnus · 2022-12-15T17:59:28Z

I see different tests failing.
@chenseanxy Lets try something - I'll let the PR run the failed tests a few more times.
Let's see if we can find some pattern between the failures of a few runs, to see if we have a smoking gun.

auvipy · 2022-12-21T17:18:26Z

can we get #7974 merge first and pull here to recheck the failing integration tests? btw what does rabbitmq_redis does different?

Nusnus · 2022-12-22T06:05:38Z

@chenseanxy The PR #7974 is merged.
Also notice the master branch was renamed to main.

Please rebase to include the fixes @auvipy added and lets see if this PR goes green :)

chenseanxy and others added 2 commits December 15, 2022 22:39

Switch between thread & global for app.backend

9b337a2

- Fixes celery#7960 and celery#6819

[pre-commit.ci] auto fixes from pre-commit.com hooks

06c75d3

for more information, see https://pre-commit.ci

auvipy requested a review from matusvalo December 15, 2022 14:53

auvipy reviewed Dec 15, 2022

View reviewed changes

chenseanxy added 4 commits December 15, 2022 22:58

Flake8

75c8491

Fixes CI:

015770d

- Flake8 6.0 PyCQA/flake8#1689

Switch from multiprocessing to billiard

291be10

Flake8

28470a0

auvipy requested a review from Nusnus December 15, 2022 15:57

auvipy self-requested a review December 15, 2022 17:37

Nusnus reviewed Dec 15, 2022

View reviewed changes

Nusnus requested a review from thedrow December 15, 2022 17:46

auvipy added this to the 5.3 milestone Dec 21, 2022

CharlieTruong mentioned this pull request Feb 7, 2023

result_backend_thread_safe config shares backend across threads #8058

Merged

Nusnus modified the milestones: 5.3, Future Feb 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch between thread & global for app.backend #7961

Switch between thread & global for app.backend #7961

chenseanxy commented Dec 15, 2022 •

edited

codecov bot commented Dec 15, 2022 •

edited

auvipy Dec 15, 2022

chenseanxy Dec 15, 2022

auvipy commented Dec 15, 2022

chenseanxy commented Dec 15, 2022 •

edited

chenseanxy commented Dec 15, 2022

Nusnus commented Dec 15, 2022

auvipy commented Dec 15, 2022

Nusnus Dec 15, 2022

Nusnus commented Dec 15, 2022

Nusnus commented Dec 15, 2022

Nusnus commented Dec 15, 2022

auvipy commented Dec 21, 2022

Nusnus commented Dec 22, 2022

Switch between thread & global for app.backend #7961

Are you sure you want to change the base?

Switch between thread & global for app.backend #7961

Conversation

chenseanxy commented Dec 15, 2022 • edited

Description

codecov bot commented Dec 15, 2022 • edited

Codecov Report

auvipy Dec 15, 2022

Choose a reason for hiding this comment

chenseanxy Dec 15, 2022

Choose a reason for hiding this comment

auvipy commented Dec 15, 2022

chenseanxy commented Dec 15, 2022 • edited

chenseanxy commented Dec 15, 2022

Nusnus commented Dec 15, 2022

auvipy commented Dec 15, 2022

Nusnus Dec 15, 2022

Choose a reason for hiding this comment

Nusnus commented Dec 15, 2022

Nusnus commented Dec 15, 2022

Nusnus commented Dec 15, 2022

auvipy commented Dec 21, 2022

Nusnus commented Dec 22, 2022

chenseanxy commented Dec 15, 2022 •

edited

codecov bot commented Dec 15, 2022 •

edited

chenseanxy commented Dec 15, 2022 •

edited