Skip to content

Enable celery command in any environment#7902

Merged
potiuk merged 2 commits intoapache:masterfrom
zeroam:cli/celery
Mar 30, 2020
Merged

Enable celery command in any environment#7902
potiuk merged 2 commits intoapache:masterfrom
zeroam:cli/celery

Conversation

@zeroam
Copy link
Contributor

@zeroam zeroam commented Mar 27, 2020

Set celery command visible even if the executor is not CeleryExecutor
Instead, raise ArgumentError and display help message when the executor is
not CeleryExecutor

airflow command
Before:
image

After:
image

airflow celery command
After (executor is SequentialExecutor):
image

reference from: #7873, #7070
reviewed from: @mik-laj, @turbaszek


Issue link: WILL BE INSERTED BY boring-cyborg

Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@turbaszek
Copy link
Member

What would you say to write a nice message in case of airflow celery + LocalExecutor instead of raising an exception? I would feel better with "Hey this work only with celery, so no action was taken" than with stacktrace from exception :)

@zeroam
Copy link
Contributor Author

zeroam commented Mar 27, 2020

What would you say to write a nice message in case of airflow celery + LocalExecutor instead of raising an exception? I would feel better with "Hey this work only with celery, so no action was taken" than with stacktrace from exception :)

@turbaszek Thanks! How about this one?
image
This shows help message instead of Exception.

@turbaszek
Copy link
Member

turbaszek commented Mar 27, 2020

What would you say to write a nice message in case of airflow celery + LocalExecutor instead of raising an exception? I would feel better with "Hey this work only with celery, so no action was taken" than with stacktrace from exception :)

@turbaszek Thanks! How about this one?
image
This shows help message instead of Exception.

Definitely better! What about "celery command works only with CeleryExecutor, your current executor: ExecutorName. For more information check the help above"

Also, would you mind to extend current desription of celery sub command? For example:
"Start celery components. Works only when using CeleryExecutor for more information see: here_link_to_docs"

@zeroam
Copy link
Contributor Author

zeroam commented Mar 27, 2020

@turbaszek your suggestion is more clear! I've changed help message and changed celery subcommand help message

@zeroam zeroam force-pushed the cli/celery branch 2 times, most recently from 3fa0f1f to 5b6fdfc Compare March 28, 2020 14:21
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

Some minor suggest.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Start celery components. Works only when using CeleryExecutor. for more information, see '
'Start celery components. Works only when using CeleryExecutor. For more information, see '

@zhongjiajie
Copy link
Member

BTW, code LGTM

@turbaszek
Copy link
Member

Hi @zeroam Travis is sad due to some static checks. Please take a look at our pre-commits:
https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst

Set celery command visible even if the executor is not CeleryExecutor
Instead, raise ArgumentError and display help message when the executor is
not CeleryExecutor

reference from: apache#7873, apache#7070
reviewed from: @mik-laj, @turbaszek
@zeroam
Copy link
Contributor Author

zeroam commented Mar 29, 2020

@turbaszek Thanks for the advice!
I checked pre-commit and passed the static test.
but I failed in test_celery_command

FAILED tests/cli/commands/test_celery_command.py::TestCeleryStopCommand::test_if_right_pid_is_read - SystemExit: 2
FAILED tests/cli/commands/test_celery_command.py::TestCeleryStopCommand::test_same_pid_file_is_used_in_start_and_stop - SystemExit: 2
FAILED tests/cli/commands/test_celery_command.py::TestWorkerStart::test_worker_started_with_required_arguments - SystemExit: 2

I will fix it up

@codecov-io
Copy link

codecov-io commented Mar 30, 2020

Codecov Report

Merging #7902 into master will decrease coverage by 0.63%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7902      +/-   ##
==========================================
- Coverage   87.16%   86.52%   -0.64%     
==========================================
  Files         931      932       +1     
  Lines       45155    45191      +36     
==========================================
- Hits        39358    39101     -257     
- Misses       5797     6090     +293     
Impacted Files Coverage Δ
airflow/cli/cli_parser.py 97.24% <63.63%> (-0.89%) ⬇️
...flow/providers/apache/cassandra/hooks/cassandra.py 21.25% <0.00%> (-72.50%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0.00%> (-55.56%) ⬇️
airflow/providers/postgres/operators/postgres.py 47.82% <0.00%> (-52.18%) ⬇️
airflow/providers/redis/operators/redis_publish.py 50.00% <0.00%> (-50.00%) ⬇️
airflow/kubernetes/volume.py 52.94% <0.00%> (-47.06%) ⬇️
airflow/providers/mongo/sensors/mongo.py 53.33% <0.00%> (-46.67%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0.00%> (-45.08%) ⬇️
airflow/providers/redis/sensors/redis_key.py 61.53% <0.00%> (-38.47%) ⬇️
airflow/executors/celery_executor.py 50.67% <0.00%> (-37.84%) ⬇️
... and 10 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 8456b30...b56941d. Read the comment docs.

@turbaszek turbaszek requested a review from mik-laj March 30, 2020 05:17
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Great change. I think it's better to show all commands and do not make it depend on the current configuration. It's much better for discovery.

@potiuk potiuk merged commit 08f0617 into apache:master Mar 30, 2020
@zeroam zeroam deleted the cli/celery branch March 31, 2020 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants