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.

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

Fix UnboundLocalError when running celery worker in foreground #6833

Merged
merged 2 commits into from Aug 17, 2021

Conversation

michael-k
Copy link
Contributor

@michael-k michael-k commented Jun 30, 2021

Description

Add the indentation missed in commit 56a6246. The bug is not present in branch master.

Fixes #6830

@thedrow thedrow self-requested a review June 30, 2021 15:41
@lgtm-com
Copy link

lgtm-com bot commented Jun 30, 2021

This pull request introduces 1 alert and fixes 1 when merging 7b1647d into 171ab02 - view on LGTM.com

new alerts:

  • 1 for Non-exception in 'except' clause

fixed alerts:

  • 1 for Unreachable code

Copy link
Contributor

@maybe-sybr maybe-sybr left a comment

Choose a reason for hiding this comment

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

Whoops, looks like a conflict resolution mistake when backporting 6e5580 onto the 5.0 release branch (as 56a6246). Those two commits should be pretty much identical but the merge from master into #6599 probably confused things.

lgtm! Thanks for the PR @michael-k. Leaving for @thedrow to merge due to the pending review or I'll aim to come back and hit the button myself later today.

@maybe-sybr
Copy link
Contributor

maybe-sybr commented Jul 1, 2021

Interesting that I can't seem to start the unit test checks on this PR... I assumed it was because @michael-k might be a first time contributor but that's not the case :/ Perhaps we need to amend to workflow rules to match our stable/release branch names as well?

@auvipy
Copy link
Member

auvipy commented Jul 2, 2021

Interesting that I can't seem to start the unit test checks on this PR... I assumed it was because @michael-k might be a first time contributor but that's not the case :/ Perhaps we need to amend to workflow rules to match our stable/release branch names as well?

yup

@thedrow
Copy link
Member

thedrow commented Jul 21, 2021

I have updated the workflow.
@michael-k Would you mind rebasing, please?

@michael-k
Copy link
Contributor Author

michael-k commented Jul 21, 2021

Would you mind rebasing, please?

There are no new commits in branch 5.0 🤷

Edit: I've recreated the commit to trigger a new workflow run.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

needs another rebase

@michael-k
Copy link
Contributor Author

needs another rebase

Rebase onto what? Before I can rebase, you need to push something to branch 5.0. The changes that pre-commit requests are unrelated to this PR and should be done in 5.0 first.

@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2021

This pull request fixes 3 alerts when merging 3927a84 into 171ab02 - view on LGTM.com

fixed alerts:

  • 1 for Non-exception in 'except' clause
  • 1 for Unreachable code
  • 1 for Module is imported with 'import' and 'import from'

@thedrow
Copy link
Member

thedrow commented Jul 21, 2021

Would you mind rebasing, please?

There are no new commits in branch 5.0 🤷

Edit: I've recreated the commit to trigger a new workflow run.

Yes I should have cherry-picked it myself.🤦 Sorry.

@thedrow
Copy link
Member

thedrow commented Jul 21, 2021

CI will run now in this branch.

@thedrow
Copy link
Member

thedrow commented Jul 21, 2021

The CI still isn't running so this needs more work. I'll keep you posted.

@codecov
Copy link

codecov bot commented Jul 21, 2021

Codecov Report

Merging #6833 (fb6b8a6) into 5.0 (b221d69) will decrease coverage by 0.01%.
The diff coverage is 69.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##              5.0    #6833      +/-   ##
==========================================
- Coverage   75.42%   75.41%   -0.02%     
==========================================
  Files         138      138              
  Lines       16418    16422       +4     
  Branches     2050     2050              
==========================================
+ Hits        12384    12385       +1     
- Misses       3818     3821       +3     
  Partials      216      216              
Flag Coverage Δ
unittests 75.41% <69.31%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
celery/__main__.py 0.00% <0.00%> (ø)
celery/bin/amqp.py 0.00% <0.00%> (ø)
celery/bin/base.py 0.00% <0.00%> (ø)
celery/bin/beat.py 0.00% <0.00%> (ø)
celery/bin/call.py 0.00% <0.00%> (ø)
celery/bin/celery.py 0.00% <0.00%> (ø)
celery/bin/control.py 0.00% <0.00%> (ø)
celery/bin/events.py 0.00% <0.00%> (ø)
celery/bin/list.py 0.00% <0.00%> (ø)
celery/bin/migrate.py 0.00% <0.00%> (ø)
... and 178 more

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 b221d69...fb6b8a6. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2021

This pull request fixes 1 alert when merging cd61c10 into b221d69 - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@maybe-sybr
Copy link
Contributor

maybe-sybr commented Jul 21, 2021

Looks like the CI failures are unrelated to this diff. Also the pre-commit job put a huge, nasty style commit on the end of this PR which should be sorted out on 5.0 (or master and cherry picked back) rather than here.

py10 failure

  _________________ ERROR collecting t/unit/backends/test_s3.py __________________
  t/unit/backends/test_s3.py:6: in <module>
      from moto import mock_s3
  .tox/3.10-unit/lib/python3.10/site-packages/moto/__init__.py:12: in <module>
      from .cloudformation import mock_cloudformation, mock_cloudformation_deprecated  # flake8: noqa
  .tox/3.10-unit/lib/python3.10/site-packages/moto/cloudformation/__init__.py:2: in <module>
      from .models import cloudformation_backends
  .tox/3.10-unit/lib/python3.10/site-packages/moto/cloudformation/models.py:11: in <module>
      from .parsing import ResourceMap, OutputMap
  .tox/3.10-unit/lib/python3.10/site-packages/moto/cloudformation/parsing.py:366: in <module>
      class ResourceMap(collections.Mapping):
  E   AttributeError: module 'collections' has no attribute 'Mapping'

Hmm, I think this should be collections.abc.Mapping. Is this broken on master also?

pypy3 failure

     Running setup.py install for ephem: started
    Running setup.py install for ephem: finished with status 'error'

Seems like we need 6c8c93c, which was part of the #6635 diff to get CI working of pypy3. The squashed merge commit is 117cd9c and should probably be cherry picked onto the 5.0 branch and then have this rebased on top.

lint failure

  An unexpected error has occurred: CalledProcessError: command: ('/home/runner/work/celery/celery/.tox/lint/bin/python', '-mvirtualenv', '/home/runner/.cache/pre-commit/repo_sawvi81/py_env-python3.7', '-p', 'python3.7')
  return code: 1
  expected return code: 0
  stdout:
      RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.7'

lint failure

It's trying to run under python3.7 which isn't available in the job environment. Surprising, I thought the lint job was supposed to run using tox in a python3.9 environment.

@lgtm-com
Copy link

lgtm-com bot commented Jul 22, 2021

This pull request introduces 1 alert and fixes 1 when merging fb6b8a6 into b221d69 - view on LGTM.com

new alerts:

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

fixed alerts:

  • 1 for Unreachable code

@thedrow
Copy link
Member

thedrow commented Jul 22, 2021

@maybe-sybr Thanks for helping me with the investigation. ❤️

@thedrow thedrow added this to the 5.1.x milestone Aug 17, 2021
@thedrow thedrow merged commit 2b7c83d into celery:5.0 Aug 17, 2021
@thedrow thedrow linked an issue Aug 17, 2021 that may be closed by this pull request
18 tasks
@michael-k michael-k deleted the fix-6830 branch November 22, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnboundLocalError when running celery worker in foreground
4 participants