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 '--pool=threads' support in command line options parsing #6787

Merged

Conversation

stack-overflow
Copy link
Contributor

Description

This PR fixes celery worker behaviour when parsing --pool=threads argument, and a related issue in command line --pool argument parsing.

First issue

The threads pool type is not recognised in command line args parsing. It's missing from the list of possible options, even though it's supported by Celery since 4.4.

Minimal example of the issue:
Run celery worker --pool=threads
This is the output:

Usage: celery worker [OPTIONS]
Try 'celery worker --help' for help.

Error: Invalid value for '-P' / '--pool': invalid choice: threads. (choose from prefork, eventlet, gevent, solo)

Second issue

When you specify any argument that is not specified in the ALIASES dict in celery/concurrency/__init__.py, you get an error. It happens, because the function maybe_patch_concurrency tries to get pool implementation before actually validating the command line arguments.

Minimal example:
Run celery worker --pool=xxx
This is the output:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.7/bin/celery", line 8, in <module>
    sys.exit(main())
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/celery/__main__.py", line 13, in main
    maybe_patch_concurrency()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/celery/__init__.py", line 145, in maybe_patch_concurrency
    concurrency.get_implementation(pool)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/celery/concurrency/__init__.py", line 28, in get_implementation
    return symbol_by_name(cls, ALIASES)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/kombu/utils/imports.py", line 56, in symbol_by_name
    module = imp(module_name, package=package, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 965, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'xxx'

I fixed that issue and now it outputs the proper error message:

# celery worker --pool=xxx

Usage: celery worker [OPTIONS]
Try 'celery worker --help' for help.

Error: Invalid value for '-P' / '--pool': invalid choice: xxx. (choose from prefork, eventlet, gevent, solo, processes, threads)

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request introduces 1 alert when merging f8c81bb into c93371d - view on LGTM.com

new alerts:

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

thedrow
thedrow previously approved these changes May 27, 2021
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #6787 (8e77e31) into master (025bad6) will increase coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6787      +/-   ##
==========================================
+ Coverage   70.63%   70.69%   +0.05%     
==========================================
  Files         138      138              
  Lines       16594    16597       +3     
  Branches     2089     2090       +1     
==========================================
+ Hits        11722    11733      +11     
+ Misses       4677     4668       -9     
- Partials      195      196       +1     
Flag Coverage Δ
unittests 70.69% <66.66%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
celery/bin/worker.py 0.00% <0.00%> (ø)
celery/__init__.py 75.86% <50.00%> (-1.34%) ⬇️
celery/concurrency/__init__.py 100.00% <100.00%> (+90.00%) ⬆️

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 c93371d...8e77e31. Read the comment docs.

@stack-overflow
Copy link
Contributor Author

@thedrow Shall I fix the failing codecov checks?

auvipy
auvipy previously approved these changes May 28, 2021
@auvipy
Copy link
Member

auvipy commented May 28, 2021

@thedrow Shall I fix the failing codecov checks?

It would be great

@stack-overflow stack-overflow dismissed stale reviews from auvipy and thedrow via 8e77e31 May 28, 2021 09:40
@stack-overflow
Copy link
Contributor Author

@auvipy Ok, I added 2 unit tests, covering cases of missing and existing concurrent.futures module.

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2021

This pull request introduces 1 alert when merging 8e77e31 into c93371d - view on LGTM.com

new alerts:

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

@auvipy auvipy merged commit bb18e1b into celery:master May 28, 2021
@meocong
Copy link

meocong commented May 30, 2021

@stack-overflow @auvipy I checkout your commit and used -P threads in command but it still failed, please check:
OS: OSX

Messages:
`
Usage: celery worker [OPTIONS]
Try 'celery worker --help' for help.

Error: Invalid value for '-P' / '--pool': invalid choice: threads. (choose from prefork, eventlet, gevent, solo)
`

@stack-overflow
Copy link
Contributor Author

@meocong thanks for reporting this. What Python version you use? Threads option dependens on concurrent.futures Python module existence in your system.

@meocong
Copy link

meocong commented May 30, 2021

@stack-overflow I use Python 3.7, -P threads is good and I want to have it back soon :D
P/S: I use trio + threads to run some Parallel tasks inside Celery Task, but trio is conflict with eventlet and gevent

tdruez added a commit to aboutcode-org/scancode.io that referenced this pull request May 31, 2021
Signed-off-by: tdruez <tdruez@nexb.com>
@auvipy
Copy link
Member

auvipy commented Jun 15, 2021

could you both recheck again?

@auvipy auvipy added this to the 5.1.x milestone Jun 15, 2021
thedrow pushed a commit that referenced this pull request Jun 28, 2021
* Fix '--pool=threads' support in command line options parsing

* Add unit tests for concurrency.get_available_pool_names
jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
…6787)

* Fix '--pool=threads' support in command line options parsing

* Add unit tests for concurrency.get_available_pool_names
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.

4 participants