--reload flag changed semantics #1422

Open
Doerge opened this Issue Jan 5, 2017 · 17 comments

Projects

None yet

5 participants

@Doerge
Doerge commented Jan 5, 2017 edited

I just upgraded from the 19.6.0 release to the latest commit c54426c

I previously ran gunicorn like gunicorn --reload mymodule:app during development. However now I get the following error:

$ gunicorn --reload mymodule:app
usage: gunicorn [OPTIONS] [APP_MODULE]
gunicorn: error: No application module specified.

If I put an option after the reload flag it works.

$ gunicorn --reload -w=1 mymodule:app
[2017-01-05 11:02:33 +0000] [102] [INFO] Starting gunicorn 19.6.0
[2017-01-05 11:02:33 +0000] [102] [INFO] Listening at:  (102)
[2017-01-05 11:02:33 +0000] [102] [INFO] Using worker: sync
[2017-01-05 11:02:33 +0000] [105] [INFO] Booting worker with pid: 105

Calling it like gunicorn --reload auto mymodule:app also works, but then the argument isn't really optional. Perhaps the reload type should be non-optional to avoid confusion?

@berkerpeksag
Collaborator

Thanks for the report. I think this has been fixed in master in 8dbb296. Could you please try the master branch?

@Doerge
Doerge commented Jan 5, 2017

Thanks for replying.

The commit I used (c54426c) is 2 after the one you point at. I installed it like:

pip3 install --force-reinstall --upgrade git+git://github.com/benoitc/gunicorn@c54426c1e4b26b30c5feee2cec4450656ae147a3
@berkerpeksag
Collaborator

Oops, you're right! I think we've hit a bug in argparse: http://bugs.python.org/issue9338

Another workaround:

$ gunicorn --reload -- mymodule:app
@berkerpeksag
Collaborator

Here is a test case:

diff --git a/tests/test_config.py b/tests/test_config.py
index 9f9a2fe..f00aa60 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -285,3 +285,17 @@ def test_always_use_configured_logger():
     c.set('statsd_host', 'localhost:12345')
     # still uses custom logger over statsd
     assert c.logger_class == MyLogger
+
+
+@pytest.mark.parametrize("options, expected", [
+    (["myapp:app"], "off"),
+    (["--reload=poll", "myapp:app"], "poll"),
+    (["--reload", "myapp:app"], "auto"),
+    (["--reload", "--", "myapp:app"], "auto"),
+])
+def test_reload(options, expected):
+    cmdline = ["prog_name"]
+    cmdline.extend(options)
+    with AltArgs(cmdline):
+        app = NoConfigApp()
+    assert app.cfg.reload == expected
@berkerpeksag
Collaborator

I think we've hit a bug in argparse: http://bugs.python.org/issue9338

I was wrong. Issue 9338 is different than this.

Here is a pure Python snippet:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument("args", nargs="*", help=argparse.SUPPRESS)
parser.add_argument("--reload", nargs="?", const="auto", default="off", help=argparse.SUPPRESS)

tests = [
    ("app:app", 'off'),
    ("--reload app:app", 'auto'),
    ("--reload poll app:app", 'poll'),
    ("--reload -- app:app", 'auto'),
    ("app:app --reload", 'auto'),
]

for args, expected in tests:
    got = parser.parse_args(args.split()).reload
    print("[%s] Test: %25s Expected: %10s Got: %10s" % ("P" if got == expected else "X", args, expected, got))

My vote is to document using -- as a workaround.

@berkerpeksag
Collaborator

If I apply the patch from http://bugs.python.org/issue9338 and replace

parser.add_argument("args", nargs="*", help=argparse.SUPPRESS)

with

parser.add_argument("args", nargs="+", help=argparse.SUPPRESS)

in the snippet I gave in #1422 (comment), --reload app:app works as expected.

Since the app:app part is the only required argument, it might make sense to change nargs from "*" to "+" in line 82 in gunicorn/config.py:

parser.add_argument("args", nargs="+", help=argparse.SUPPRESS)
@benoitc
Owner
benoitc commented Jan 8, 2017

@berkerpeksag but we need the patch in Python for it right?

@berkerpeksag
Collaborator

I can commit the patch (it's not ready to commit btw) to next Python 3.5 and 3.6 releases, but that would only work for 3.5+.

I see two viable solutions:

  • Document the current behavior and the -- workaround
  • Since this change is not released yet we can introduce a new setting like --reloader-backend and keep the old behavior of --reload
@benoitc
Owner
benoitc commented Jan 8, 2017

@berkerpeksag i'm asking myself if we should keep the old behaviour at all ...

@benoitc
Owner
benoitc commented Jan 8, 2017

ie. we can just force the user to choose the reload type it needs when using the --reload setting. Which would be simpler.

@benoitc
Owner
benoitc commented Jan 8, 2017

@tilgovi just saw the pacth was yours, what do you think about it also? Should we keep the old behaviour and add another setting to the long list we have? Or force the usage of the new behaviour implying to pass the reload type?

@berkerpeksag
Collaborator
berkerpeksag commented Jan 12, 2017 edited

It would be nice to keep the old behavior. Another alternative solution is to remove --reload=... options and use the best options available implicitly.

@tilgovi
Collaborator
tilgovi commented Jan 12, 2017

👍 for just using the best available

@berkerpeksag
Collaborator

Ok, I will open a PR to implement it this week :)

@berkerpeksag berkerpeksag added a commit to berkerpeksag/gunicorn that referenced this issue Jan 13, 2017
@berkerpeksag berkerpeksag Use the best available reloader when --reload is passed
Fixes #1422
fb824c0
@mark-adams
Contributor

I just commented about this on #1430 but I wanted to echo the same idea here.

If we simply always use the best available, we could box the user in. I've ran into scenarios where I was on an inotify-supported Linux system but using a FS mount that didn't support inotify. If we changed to use best available, users in that scenario would be out of luck without a way to force polling.

@tilgovi
Collaborator
tilgovi commented Jan 20, 2017

It's not the presence of kernel support, but the Python library that allows using it. In this case, the user could install into a virtualenv that doesn't have the library. Is that still enough of a pain that it's likely to cause problems for many users?

@tilgovi
Collaborator
tilgovi commented Jan 20, 2017

If the user has an app that needs to use inotify for its own reasons, maybe.

An alternatively might be to use automatic selection with the CLI flag but allow the config file to specify explicitly, if we really want to avoid breaking change to the flag AND preserve the option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment