Add annotations to minimise differences with celery-aio-pool's tracer.py. by ShaheedHaque · Pull Request #7925 · celery/celery

ShaheedHaque · 2022-11-27T13:50:29Z

Description

As per #7874, add annotations to trace.py to improve it's documentation, and make it easier to compare with celery-aio-pool's tracer.py. See also #the-wondersmith/celery-aio-pool/pull/4.

(Note: unit tests are currently badly broken)

codecov · 2022-11-27T14:32:16Z

Codecov Report

Base: 89.89% // Head: 89.90% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (29ac7a6) compared to base (b5bc40f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7925   +/-   ##
=======================================
  Coverage   89.89%   89.90%           
=======================================
  Files         128      128           
  Lines       15896    15910   +14     
  Branches     2140     2143    +3     
=======================================
+ Hits        14290    14304   +14     
  Misses       1370     1370           
  Partials      236      236

Flag	Coverage Δ
unittests	`89.88% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
celery/app/trace.py	`98.61% <100.00%> (+0.01%)`	⬆️
celery/app/base.py	`96.66% <0.00%> (+0.01%)`	⬆️
celery/canvas.py	`94.72% <0.00%> (+0.04%)`	⬆️

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.

lgtm-com · 2022-11-27T14:53:46Z

This pull request introduces 1 alert when merging 9d3046f into 87613c7 - view on LGTM.com

new alerts:

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

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com · 2022-11-27T16:14:01Z

This pull request introduces 1 alert when merging 4468397 into 87613c7 - view on LGTM.com

new alerts:

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

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

celery/app/trace.py

….py.

lgtm-com · 2022-11-29T11:43:27Z

This pull request introduces 1 alert when merging 29ac7a6 into 87613c7 - view on LGTM.com

new alerts:

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

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

ShaheedHaque · 2022-11-29T21:51:10Z

@auvipy I think I accidentally removed you as a reviewer, but I am not sure how to add you back.

ShaheedHaque · 2023-01-03T23:57:26Z

@auvipy @clokep Any chance of getting this reviewed and merged? I have merged the corresponding celery-aio-pool PR.

clokep

I don't see any issues with it.

clokep · 2023-01-04T12:18:15Z

celery/app/trace.py

+            request: celery.app.task.Context,
+            exc: Union[Exception, Type[Exception]],
+            state: str = FAILURE,
+            call_errbacks: bool = True) -> Tuple[Info, Any, Any, Any]:


I believe that I.state is a string, but that can always be tightened up in the future.

Suggested change

call_errbacks: bool = True) -> Tuple[Info, Any, Any, Any]:

call_errbacks: bool = True) -> Tuple[Info, Any, str, Any]:

…s tracer.py. (celery#7925)" This reverts commit 0233c3b.

* Revert "Add annotations to minimise differences with celery-aio-pool's tracer.py. (#7925)" This reverts commit 0233c3b. * Added smoke test: test_worker_consume_tasks_after_redis_broker_restart * Removed Redis xfail from tests now that the bug is fixed * Renamed smoke tests CI jobs

Nusnus · 2024-01-17T13:28:34Z

This PR was reverted as it introduced a bug, unfortunately.
At the time, it was impossible to know. Now we have the means.

The work needs to be done again, apologies & thank you 🙏

ShaheedHaque · 2024-01-17T14:11:52Z

@Nusnus OK, sorry for the inconvenience.

I have reviewed the code and am not able to see what caused the issue? AFAICS, this commit consists of annotations which are not supposed to have any run time effect. Did some supporting import go wrong perhaps?

Nusnus · 2024-01-17T21:43:14Z

@ShaheedHaque

I have reviewed the code and am not able to see what caused the issue? AFAICS, this commit consists of annotations which are not supposed to have any run time effect. Did some supporting import go wrong perhaps?

My first attempt was just reverting single lines - anything that wasn’t pure type annotation change. I also added from __future__ import annotations and ran pyupgrade, which further cleaned the original contribution.
I am sure I missed something, because it didn’t work. I was so tired when I stumbled upon this, so I just reverted the entire PR, and the original bug was gone, so I went with it (due to being just a documentation PR and the bug being an urgent production-breaking issue).

@Nusnus OK, sorry for the inconvenience.

No worries - on the contrary though, you’re welcome to redo the work now that you can trust the CI to let you know if something breaks, so go ahead - and tag me for review 🤙

ShaheedHaque mentioned this pull request Nov 27, 2022

Minimise whitespace, comma and quote differences with Celery's trace.py. the-wondersmith/celery-aio-pool#4

Merged

auvipy self-requested a review November 27, 2022 14:03

ShaheedHaque force-pushed the srh_reduce_trace_diffs branch 2 times, most recently from 66c1233 to 9d3046f Compare November 27, 2022 14:28

ShaheedHaque force-pushed the srh_reduce_trace_diffs branch 2 times, most recently from 108e90a to 4468397 Compare November 27, 2022 15:46

clokep reviewed Nov 28, 2022

View reviewed changes

celery/app/trace.py Outdated Show resolved Hide resolved

clokep reviewed Nov 28, 2022

View reviewed changes

celery/app/trace.py Outdated Show resolved Hide resolved

Add annotations to minimise differences with celery-aio-pool's tracer…

29ac7a6

….py.

ShaheedHaque force-pushed the srh_reduce_trace_diffs branch from 4468397 to 29ac7a6 Compare November 28, 2022 17:29

ShaheedHaque requested review from clokep and removed request for auvipy November 29, 2022 08:30

auvipy requested review from auvipy and removed request for clokep November 30, 2022 04:06

ShaheedHaque mentioned this pull request Jan 4, 2023

Bare-Bones Python 3.10+ Asyncio Coroutine Support For Celery 6+ #7874

Open

4 tasks

clokep approved these changes Jan 4, 2023

View reviewed changes

thedrow merged commit 0233c3b into celery:main Jan 4, 2023

Nusnus added a commit to Katz-Consulting-Group/celery that referenced this pull request Jan 16, 2024

Revert "Add annotations to minimise differences with celery-aio-pool'…

74d5b51

…s tracer.py. (celery#7925)" This reverts commit 0233c3b.

Nusnus mentioned this pull request Jan 16, 2024

Bugfix: Worker not consuming tasks after Redis broker restart #8796

Merged

Nusnus added a commit to Katz-Consulting-Group/celery that referenced this pull request Jan 17, 2024

Revert "Add annotations to minimise differences with celery-aio-pool'…

4565023

…s tracer.py. (celery#7925)" This reverts commit 0233c3b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add annotations to minimise differences with celery-aio-pool's tracer.py.#7925

Add annotations to minimise differences with celery-aio-pool's tracer.py.#7925
thedrow merged 1 commit intocelery:mainfrom
ShaheedHaque:srh_reduce_trace_diffs

ShaheedHaque commented Nov 27, 2022 •

edited

Loading

Uh oh!

codecov bot commented Nov 27, 2022 •

edited

Loading

Uh oh!

lgtm-com bot commented Nov 27, 2022

Uh oh!

lgtm-com bot commented Nov 27, 2022

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 29, 2022

Uh oh!

ShaheedHaque commented Nov 29, 2022

Uh oh!

ShaheedHaque commented Jan 3, 2023 •

edited

Loading

Uh oh!

clokep left a comment

Uh oh!

clokep Jan 4, 2023

Uh oh!

Nusnus commented Jan 17, 2024

Uh oh!

ShaheedHaque commented Jan 17, 2024 •

edited

Loading

Uh oh!

Nusnus commented Jan 17, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	call_errbacks: bool = True) -> Tuple[Info, Any, Any, Any]:
	call_errbacks: bool = True) -> Tuple[Info, Any, str, Any]:

Uh oh!

Conversation

ShaheedHaque commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented Nov 27, 2022

Uh oh!

lgtm-com bot commented Nov 27, 2022

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 29, 2022

Uh oh!

ShaheedHaque commented Nov 29, 2022

Uh oh!

ShaheedHaque commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

clokep Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

Nusnus commented Jan 17, 2024

Uh oh!

ShaheedHaque commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nusnus commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShaheedHaque commented Nov 27, 2022 •

edited

Loading

codecov bot commented Nov 27, 2022 •

edited

Loading

ShaheedHaque commented Jan 3, 2023 •

edited

Loading

ShaheedHaque commented Jan 17, 2024 •

edited

Loading

Nusnus commented Jan 17, 2024 •

edited

Loading