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

Unlike what doc states, --app does not behave like -A #4558

Closed
hartym opened this issue Feb 26, 2018 · 16 comments · Fixed by #6223
Closed

Unlike what doc states, --app does not behave like -A #4558

hartym opened this issue Feb 26, 2018 · 16 comments · Fixed by #6223

Comments

@hartym
Copy link

hartym commented Feb 26, 2018

Steps to reproduce

  • Try to run celery --app foo *anything*.
  • Look at the "celery usage" output instead of the "anything" expected output.

Expected behavior

"-A foo" and "--app foo" to behave the same way.

Actual behavior

"-A foo" works, while "--app foo" just outputs the "celery usage" screen

Environment

software -> celery:4.1.0 (latentcall) kombu:4.1.0 py:3.6.3
            billiard:3.5.0.3 py-amqp:2.2.2
platform -> system:Darwin arch:64bit imp:CPython
loader   -> celery.loaders.app.AppLoader
settings -> transport:amqp results:django-db
@auvipy
Copy link
Member

auvipy commented Aug 12, 2018

whats the status with celery 4.2.1?

@hartym
Copy link
Author

hartym commented Aug 15, 2018

Exact same with v4.2.1 (windowlicker).

@hartym
Copy link
Author

hartym commented Aug 15, 2018

There is in fact a behavioral difference that depends on arguments position:

celery worker -A foo works
celery worker --app foo works
celery -A foo worker works
celery --app foo worker does not work and show celery usage

@auvipy
Copy link
Member

auvipy commented Aug 15, 2018

do you have any suggested improvement in mind?

@hartym
Copy link
Author

hartym commented Aug 19, 2018

Well, that I think it should behave the same whatever the order of arguments is. I struggled and lost a bit of time because I first chosed the way that does not work (you know, Murphy's law). I tried to look in the argparsing code of celery to see if I could patch it, but wasn't clear to me how it worked, hence opening the issue. If you point me to the right direction (like where it should be done, and probably how it is tested today), I may be able to submit a patch.

@thedrow
Copy link
Member

thedrow commented Aug 19, 2018

We're adding both -A and --app here:

group.add_argument('-A', '--app', default=None)

They should be entirely interchangeable.
We haven't done anything else to define that argument.

The relevant test which verifies this passes (and you are welcome to run the tests yourself):

cmd.setup_app_from_commandline(['--app=%s' % (appstr,),

I cannot reproduce this behavior locally on my Linux machine.
Could this be a bug in argparse that only reproduces on OSX? I find it unlikely but possible.

Unless you can provide me with new information on how to reproduce this bug (e.g. it only happens on OSX or some certain order of arguments) I think it's is safe to close this one.
This does not mean that there is no problem to be fixed. We're just not sure it's a problem within Celery's codebase.

@hartym
Copy link
Author

hartym commented Aug 19, 2018

@thedrow Thanks for the infos.

It does indeed appear with certain order of arguments, using "--app x" (not "--app=x", notice the lack of equal sign) before the celery subcommand (worker, inspect, ...). It is 100% reproductible on linux, here is how.

I launch a brand new linux image (docker) running "python:3" official image. The image was run using docker run -it --rm python:3 bash, and here is the "bootstrap" script I run by hand after launching the container:

$ pip install celery
$ cat > foo.py
from celery import Celery
app = Celery("foo")
^D

(edited for copypaste mistake)

Minimalist, right ? Now run in this image:

# celery -A foo report

software -> celery:4.2.1 (windowlicker) kombu:4.2.1 py:3.7.0
            billiard:3.5.0.4 py-amqp:2.3.2
platform -> system:Linux arch:64bit imp:CPython
loader   -> celery.loaders.app.AppLoader
settings -> transport:amqp results:disabled

Works as expected. Let's switch to --app foo:

# celery --app foo report
usage: celery <command> [options]

Show help screen and exit.

... etc ...

Note that it does work with --app=foo, just not with --app foo (notice the lack of equal sign). But celery usage says: Global Options: -A APP, --app APP.

I don't agree on the fact that it's "plain argparse". I think that the culprit code is somehow related to CeleryCommand._relocate_args_from_start which (as far as I understand) change sys.argv order before those parameters as passed to argparse, so that global arguments are moved after the celery command (like "worker", "inspect", etc).

I understand the goal, but it does not work 100% with arguments as understood by argparse, nor as advertised by the celery usage help.

Behaviour is 100% same on OSX and Linux. I tried to write an unit test but my lack of understanding the celery codebase (and especially, what the MockCommand does) made me fail at that.

@hartym
Copy link
Author

hartym commented Aug 19, 2018

(note that this bug is probably also affecting all other global options passed before the command name and that have arguments, like --broker BROKER, --result-backend RESULT_BACKEND, --loader LOADER, --config CONFIG, --workdir WORKDIR ...)

Example:

  • # celery report -A foo --workdir . works
  • # celery --workdir . report -A foo shows celery usage
  • # celery --workdir=. report -A foo works again

@hartym
Copy link
Author

hartym commented Aug 19, 2018

@thedrow
Copy link
Member

thedrow commented Aug 19, 2018

Thank you for the analysis. This is indeed a bug and I am able to reproduce it as you said.

I used celery --app=foo report which is why I thought this was not reproducible.

@hartym
Copy link
Author

hartym commented Aug 19, 2018

I did see the "=" sign in answers, hence the small emphasis on not using it.

Note that I'd gladly work on a fix, but the complexity added by the argparse-related hack to reorder arguments and the fact that for now, tests does not check cases with arguments before and after the subcommand makes me feel slightly uncomfortable about submitting a patch, as I don't know how to test it.

However, I do understand why the hack exist, as this is a struggle I often have with argparse (having "global arguments" work both before and after subcommand). I think it's a UX defect of argparse, and I understand that celery want to work around it. But this is something that can really annoy users, especially users like me that only use celery every insert long period of time here (and Murphy's law forces me to use the only case that does not work, by default ...).

If you can give a few pointers on how to test cases with arguments before and after the subcommand in the unit tests, I can give it a shot. It looks like the test you pointed me to skips argparsing, as MockCommand overrides parse_options:

  19   │ class MockCommand(Command):
  20   │     mock_args = ('arg1', 'arg2', 'arg3')
  21   │
  22   │     def parse_options(self, prog_name, arguments, command=None):
  23   │         options = {'foo': 'bar', 'prog_name': prog_name}
  24   │         return options, self.mock_args

@JulienPalard
Copy link
Contributor

Just hit this issue too, with 4.4.6. It's indeed related to _relocate_args_from_start, which behave differently depending on the number of dashes:

>>> from celery.bin.celery import CeleryCommand
>>> CeleryCommand()._relocate_args_from_start(['-A', 'hkis', 'worker'], 0)
['worker', '-A', 'hkis']  # Good, both "-A" and "hkis" got pushed to the end.
>>> CeleryCommand()._relocate_args_from_start(['--app', 'hkis', 'worker'], 0)
['hkis', 'worker', '--app']  # Bad, only `--app` got pushed to the end, leaving `hkis` alone :(

Walking the history

It was once a simple function:

def remove_options_at_beginning(self, argv, index=0):
    if argv:
        while index < len(argv):
            value = argv[index]
            if value.startswith("--"):
                pass
            elif value.startswith("-"):
                index += 1
            else:
                return argv[index:]
            index += 1
    return []

which already behaved differently for -- (dropping the flag) and - (dropping two elements, like if it has a value), which kind of make sense.

The trace to this function is, in my case:

__main__.py(16)main()
bin/celery.py(322)main()
bin/celery.py(496)execute_from_commandline()
bin/base.py(305)execute_from_commandline()
bin/celery.py(482)handle_argv()
bin/celery.py(439)_relocate_args_from_start()

Which looks strange to me is that execute_from_commandline already parses the -A/--app using argparse, so it should not be needed to remove it or move it at the end.

@JulienPalard
Copy link
Contributor

While trying to fix it I discoverd the bug hit any command argument using two dashes and taking a value, for example:

celery -l debug -A hkis worker  # Works as it's being rewritten celery -A hkis worker -l debug

vs

celery --loglevel debug -A hkis worker  # Fails, as being rewritten celery debug -A hkis worker --loglevel

@auvipy auvipy added this to the 4.4.x milestone Jul 12, 2020
auvipy pushed a commit that referenced this issue Jul 14, 2020
* FIX: -A and --args should behave the same.

Closes #4558

The added test should fail like this, without this patch:

AssertionError: assert 't.unit.bin.test_celery.APP' == 'worker'

* Remove dead code.

* Feel that this should be kept untouched.
@auvipy
Copy link
Member

auvipy commented Aug 10, 2020

Julien can you please check a regression report which might cause by the fix you provide for this issue?

@JulienPalard
Copy link
Contributor

Sorry for the latency I was on holidays (I'm still until september). I think my commit should be reverted and this issue reopened, it, at least, "documented" something for future implementers, in the git log. Various things that broke should be added in the tests.

Celery argument parsing is too complicated for me, I'm not willing to spend more time on it. I think Celery should slowly deprecate odd usages like giving subcommand arguments before the subcommand itself, it would simplify the code and indirectly close this issue. But I don't feel legitimate to push this, it's a long term process, as a lot of people and scripts (some of them hidden deep in docker images I bet) are relying on those odd orders and will break.

@auvipy
Copy link
Member

auvipy commented Aug 21, 2020

let's come back from vacation! we will handle it after you are available

jeyrce pushed a commit to jeyrce/celery that referenced this issue Aug 25, 2021
* FIX: -A and --args should behave the same.

Closes celery#4558

The added test should fail like this, without this patch:

AssertionError: assert 't.unit.bin.test_celery.APP' == 'worker'

* Remove dead code.

* Feel that this should be kept untouched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants