I had neglected to pin my version of gunicorn, and the run command started breaking this morning when I re-deployed my app and it automatically upgraded to 20.0.
Downgrading my version of gunicorn back to 19.9 fixed the issue.
This is the command I'm using to run my app:
gunicorn 'app:create_app()' --workers 4 --threads 4 --bind 0.0.0.0:$PORT
The error is:
Failed to find application object 'create_app()' in 'app'
I have experienced this issue also, i.e.
Failed to find application object 'create_app()' in 'app'
and pinning to version 19.9.0 solves the issue.
I initially though the fix was to change the gunicorn command from:
gunicorn --bind 0.0.0.0:$PORT app:create_app()
to:
gunicorn --bind 0.0.0.0:$PORT app:create_app
(notice the brackets after create_app are now gone). Initially, all seems well:
website_1 | [2019-11-10 19:18:54 +0000] [1] [INFO] Starting gunicorn 20.0.0
website_1 | [2019-11-10 19:18:54 +0000] [1] [INFO] Listening at: http://0.0.0.0:8000 (1)
website_1 | [2019-11-10 19:18:54 +0000] [1] [INFO] Using worker: sync
website_1 | [2019-11-10 19:18:54 +0000] [11] [INFO] Booting worker with pid: 11
But alas it is only a mirage because when you try load your flask website/endpoint it will say:
[2019-11-10 19:20:28 +0000] [11] [ERROR] Error handling request /
website_1 | Traceback (most recent call last):
website_1 | File "/usr/local/lib/python3.7/site-packages/gunicorn/workers/sync.py", line 134, in handle
website_1 | self.handle_request(listener, req, client, addr)
website_1 | File "/usr/local/lib/python3.7/site-packages/gunicorn/workers/sync.py", line 175, in handle_request
website_1 | respiter = self.wsgi(environ, resp.start_response)
website_1 | TypeError: create_app() takes 0 positional arguments but 2 were given
This is clearly a problem with gunicorn version 20.0.0.
It must be related to this change: https://github.com/benoitc/gunicorn/commit/3701ad9f26a7a4c0a081dfd0f6e97ecb272de515#diff-0b90f794c3e9742c45bf484505e3db8dR377 via #2043 .
One way to fix it on your side is to have exported myapp = create_app()
in your main module and ten start with app:myapp
. This should work, let me know if it doesn't.
I will look if something need to be done there. @berkerpeksag why the removal of the eval
was needed there?
It must be related to this change: 3701ad9#diff-0b90f794c3e9742c45bf484505e3db8dR377 via #2043 .
One way to fix it on your side is to have exported
myapp = create_app()
in your main module and ten start withapp:myapp
. This should work, let me know if it doesn't.I will look if something need to be done there. @berkerpeksag why the removal of the
eval
was needed there?
I have made this change in my application and fixed the crashing, Gunicorn is now able to run my application by saving the result of create_app()
in a variable and exporting it so that it can be used in my Gunicorn run command
# app.py
def create_app():
...
my_app = create_app()
gunicorn "app:my_app" --workers 8
I can confirm that doing what @benoitc and @jrusso1020 suggested above fixes the problem. Thanks all!
I don't see the fix working if parameter passing is required at launchtime like:
gunicorn --chdir hcli_core path
"hcli_core:HCLI("hcli_core sample hfm
").connector".
Parameter passing works in 19.9.0 but fails in 20.0.0.
@benoitc in case it's helpful to know, the flask docs recommend the app:create_app()
pattern when using gunicorn. I suspect some of your new users first try gunicorn as a result of building flask apps, and they'll attempt to use the now-broken recommendation from those docs (that was my experience at least).
I can reach out to that team to ask them to update, however I'll wait for @berkerpeksag to weigh in on the dropping of exec
in case it makes sense to bring it back in.
@tjwaterman99 well I am not sure I like passing arguments this way to an app. I don't think it's a good idea. Arguments should be passed via the env.
Our own example of Flask usage is doing what I describe . I'm thinking the current way is simpler to handle. Thoughts?
cc @tilgovi @berkerpeksag ^^
FWIW we are running into this as well.
I expect out there are quite a lot of people following the "application factory" Flask pattern.
There is a work around for sure, but at least the changelog should mention this as breaking change.
I don't think we ever intentionally supported usages such as app:callable()
and app:callable(some, args)
. I'd say it was an unfortunate side effect of using eval()
in the previous implementation.
The current implementation is pretty close to what Django's import_string()
does:
https://github.com/django/django/blob/master/django/utils/module_loading.py#L7-L24
I'd be happy to improve documentation, add a release note, and raise a more descriptive error message.
I don't think we ever intentionally supported usages such as app:callable() and app:callable(some, args). I'd say it was an unfortunate side effect of using eval() in the previous implementation.
Yes I agree. We never supported such way to start an application as far as I am looking.
I'm +1 for a more explicit error. Maybe we should raise an error if the application name is not a simple name?
Please keep in mind that this was explicit behaviour mentioned in public documentation for one of the major wsgi frameworks (flask), and has previously been supported by your project. Removing the eval prevents lazy initiation of an application, which is a problem if an application is 1) provided by a library, and 2) incurs non-trivial setup costs. If there is no security or other reason why an eval is inappropriate, would it not be simpler just to continue to support your existing behaviour?
In case anybody encounters a similar case, the appropriate workaround from Python 3.7 onwards would be faking a module-level variable by creating a module-level __getattr__
, as per this PEP. That would allow lazy initiation (a la application factories) without hitting the breaking change in gunicorn 20.0.0.
Well, we never supported such behaviour really, none of our docs or examples use it. That doesn't fit the command line.
But right this is really a breaking change and unexpected . I would be then in favor to put back the eval
and warn the user about a deprecated behaviour. Maybe also to replace it and let people use a "factory" design pattern we could add a setting --init-args
:
gunicorn -b :8000 --init-args="arg1,arg2" app:factory_method
Or something like it. Thoughts?
@benoitc Supporting factory methods with an explicit command-line flag would be excellent 馃槃 Maybe something like:
$ gunicorn -b :8000 \
--callable \
--callable-arg "abc" \
--callable-arg "xyz" \
--callable-kwarg "key" "value" \
app:factory_method
(Or maybe another base name, like --factory
)
I've been running into issues with this change because there's no longer a way for me to easily run tests. Because (i) my app depends on environment variables, (ii) test collection loads all modules (for doctests) and (iii) I can no longer defer app construction until after import, I can't test my project without adding a long string of environment variables before every test command, and testing takes longer than it used to.
Since I'm on Python 3.7, I think I can hack around this with a module-level __getattr__
, but for pre-3.7 I don't think there's any solution to this issue besides downgrading.
I think supporting factory methods with a command-line flag would solve this problem. If I'm missing an obvious solution though, other suggestions would also be appreciated 馃檭
@tjwaterman99 well I am not sure I like passing arguments this way to an app. I don't think it's a good idea. Arguments should be passed via the env.
Our own example of Flask usage is doing what I describe . I'm thinking the current way is simpler to handle. Thoughts?
I agree, I think passing arguments via the environment is more intuitive and encourages users to have their configuration live in one place. However supporting callable objects / factories is important for at least Flask, and perhaps other frameworks too.
+1 for raising a warning, and providing instructions on how to use Gunicorn with factories before deprecating exec
next release.
It's unfortunate that this happened. We have have two choices of how to respond. We could change the behavior back or we can help everyone migrate.
If we were to change the behavior back, it might make sense to pull the release from PyPI, but I think this is too drastic. Gunicorn never documented or suggested this usage.
Therefore, I suggest we just help everyone adapt and apologize for the inconvenience.
We should reach out to Flask with a PR to update their documentation. I'm happy to do that. I think others are already documenting the migration path here.
I'll add to the suggestions that it can be useful to have a _separate_ module or script that imports the application factory, calls it, and exports it. That can serve as the Gunicorn entry point and it can be omitted from doctest and other tooling so that it doesn't trigger unwanted imports when running these tools in development. Something like a __main__.py
or web.py
script can work for this.
In the future, we should make release candidates available even when we think releases should be safe. We could have caught this with a release candidate and then had an opportunity to document the breaking change in our release notes, or deprecate it for a cycle.
I don't think it makes sense to add support for initialization arguments on the command line. It's too late for this release; we already support custom applications for advanced use cases; and many frameworks have their own recommended ways to pass settings to applications. Gunicorn should not need to provide its own. Trying to add arguments to fix this issue expands the surface area for this kind of breaking change in the future. We should aim to minimize the CLI surface of Gunicorn as much as is practical.
We should reach out to Flask with a PR to update their documentation. I'm happy to do that. I think others are already documenting the migration path here.
I see that @bilalshaikh42 already has done this at https://github.com/pallets/flask/pull/3421
(One of the Flask maintainers here)
While I fully agree with getting rid of eval
there, I think there should be explicit support for app factories! The whole point of an app factory is to avoid having an importable app
object (since using that often results in circular dependency hell).
In the flask run
cli (only for development) we actually added explicit support for app factories, because they are so common.
Sure, creating a wsgi.py
containing from myapp. import make_app; app = make_app()
is easy. But I either need to maintain this file separately (which is inconvenient because now pip install myapp
won't install everything needed to just run it), or put it in my package (which means that now you could import it from within the app itself which would be wrong)
In Flask we went for an explicit way of checking for a callable app factory and calling it without resorting to eval
- maybe you could consider something like this as well? If you want less magic you could even use different CLI args for pointing to an application and for pointing to an app factory.
In the future, we should make release candidates available even when we think releases should be safe. We could have caught this with a release candidate and then had an opportunity to document the breaking change in our release notes, or deprecate it for a cycle.
Not sure if RCs really help - generally people don't install/upgrade with --pre
(also because of how badly this works - it doesn't only affect explicitly specified packages, but all nested dependencies no matter how deep, so it's very easy that some dependency of a dependency pulls in a broken prerelease), so anyone who just didn't pin their versions won't spot any breakage until they actual release.
For what it's worth, zope.hookable
provides an easy way to implement a lazy factory-like approach with essentially no overhead (due to an optional C extension). It doesn't do anything about passing extra arguments, though.
# app.py
from zope.hookable import hookable
def make_app():
def _(environ, start_response, value=b'Hello'):
start_response('200 OK',
[('Content-Type', 'text/plain')])
return [value]
return _
@hookable
def app(environ, start_response):
real_app = make_app()
app.sethook(real_app)
return real_app(environ, start_response, b"First time")
$ gunicorn app:app
[2019-11-12 05:53:47 -0600] [12457] [INFO] Starting gunicorn 20.0.0
[2019-11-12 05:53:47 -0600] [12457] [INFO] Listening at: http://127.0.0.1:8000 (12457)
[2019-11-12 05:53:47 -0600] [12457] [INFO] Using worker: sync
[2019-11-12 05:53:47 -0600] [12460] [INFO] Booting worker with pid: 12460
...
% http localhost:8000
HTTP/1.1 200 OK
Connection: close
Content-Type: text/plain
Date: Tue, 12 Nov 2019 11:53:49 GMT
Server: gunicorn/20.0.0
Transfer-Encoding: chunked
First time
% http localhost:8000
HTTP/1.1 200 OK
Connection: close
Content-Type: text/plain
Date: Tue, 12 Nov 2019 11:53:51 GMT
Server: gunicorn/20.0.0
Transfer-Encoding: chunked
Hello
Sure, creating a
wsgi.py
containingfrom myapp. import make_app; app = make_app()
is easy. But I either need to maintain this file separately (which is inconvenient because nowpip install myapp
won't install everything needed to just run it), or put it in my package (which means that now you could import it from within the app itself which would be wrong)
Another reason have a wsgi.py
in your project is wrong: some tools imports all modules in a project; eg. pytest does when looking for doctests.
Another Flask maintainer here. @ThiefMaster already said everything I wanted to say, so I'm mostly reiterating my support for the feature.
I agree with getting rid of eval
, and I avoided it in flask run
. You can add a more constrained version of the previous behavior. If the command line option has parens, assume it's a factory that returns the real app. Use literal_eval
to parse the contents of the parens, then call the factory with the parsed params.
I think the factory pattern without a wsgi.py
file is pretty valuable. I'd like to help find a way to keep it in Gunicorn.
Would someone like to put a PR together for literal_eval
of factory-like application strings? This would be in gunicorn.util.import_app
.
Need to add tests, but here's Flask's code ported to Gunicorn: https://github.com/benoitc/gunicorn/compare/master...davidism:import-factory
@davidism If you're interested, here's a function that might be useful for loading apps from app factories (with doctests 馃槃). It uses Python's built-in AST parser to differentiate attribute names and function calls (rather than a regex). It also supports keyword arguments in the factory function. Everything is still evaluated using ast.parse
and ast.literal_eval
, so there are no eval
calls:
import ast
from types import ModuleType
from typing import Any
def get_app(module: ModuleType, obj: str) -> Any:
"""
Get the app referenced by ``obj`` from the given ``module``.
Supports either direct named references or app factories, using `ast.literal_eval` for safety.
Example usage::
>>> import collections
>>> get_app(collections, 'Counter')
<class 'collections.Counter'>
>>> get_app(collections, 'Counter()')
Counter()
>>> get_app(collections, 'import evil_module') # doctest: +ELLIPSIS
Traceback (most recent call last):
...
ValueError: Could not parse 'import evil_module' as a reference to a module attribute or app factory.
>>> get_app(collections, '(lambda: sys.do_evil)()')
Traceback (most recent call last):
...
ValueError: App factories must be referenced by a simple function name
>>> get_app(collections, '(1, 2, 3)')
Traceback (most recent call last):
...
ValueError: Could not parse '(1, 2, 3)' as a reference to a module attribute or app factory.
"""
# Parse `obj` to an AST expression, handling syntax errors with an informative error
try:
# Note that mode='eval' only means that a single expression should be parsed
# It does not mean that `ast.parse` actually evaluates `obj`
expression = ast.parse(obj, mode='eval').body
except SyntaxError as syntax_error:
raise ValueError("Could not parse '{}' as a reference to a module attribute or app factory.".format(obj)) from syntax_error
# Handle expressions that just reference a module attribute by name
if isinstance(expression, ast.Name):
# Expression is just a name, attempt to get the attribute from the module
return getattr(module, expression.id)
# Handle expressions that make a function call (factory)
if isinstance(expression, ast.Call):
# Make sure the function name is just a name reference
if not isinstance(expression.func, ast.Name):
raise ValueError("App factories must be referenced by a simple function name")
# Extract the function name, args and kwargs from the call
try:
name = expression.func.id
args = [ast.literal_eval(arg) for arg in expression.args]
kwargs = {keyword.arg: ast.literal_eval(keyword.value) for keyword in expression.keywords}
except ValueError as value_error:
raise ValueError("Could not evaluate factory arguments, please ensure that arguments include only literals.") from value_error
# Get and call the function, passing in the given arguments:
return getattr(module, name)(*args, **kwargs)
# Raise an error, we only support named references and factory methods
raise ValueError("Could not parse '{}' as a reference to a module attribute or app factory.".format(obj))
If the decision is made to only support zero-argument app factories, all of the argument processing code can be removed. It will still work well to safely differentiate names and factory calls (and be useful for giving specific error messages to users when they try to pass arguments to the factory)
@ThiefMaster I am still unconvinced we should support such pattern. How is it useful? Why not using environment variables to pass custom args or a config if it is really needed?
Sure, creating a wsgi.py containing from myapp. import make_app; app = make_app() is easy. But I either need to maintain this file separately (which is inconvenient because now pip install myapp won't install everything needed to just run it), or put it in my package (which means that now you could import it from within the app itself which would be wrong)
I don't understand that, why does such file have to be maintained separately?
If it's in the package, then it's importable. So if you have a bigger project someone will eventually just import it instead of using current_app
etc.. ie more work when dealing with PRs containing this kind of mistakes.
If it's outside the package, you won't get it when doing a pip install
.
FWIW, I don't really care about passing arguments. Usually those are not needed (env vars are indeed the way to go). But at least being able to point to a callable app factory instead of an app object is incredibly useful!
Why not using environment variables to pass custom args or a config if it is really needed?
pytest
loads every module in the project to find tests. If you have a global app=Flask()
object that depends on environment variables or a config file, that object will be loaded when running tests. It's useful to be able to run tests without setting environment variables or config files. The app factory pattern is great for this.
The factory with arguments pattern is somewhat common due to some popular Flask tutorials, which is why I supported it in flask run
. I agree it's preferable to use the environment to configure the app, so I'd be fine with a more reduced version that supports calling a factory without arguments.
# an app is a name only
$ gunicorn module:app
# a factory has (), but no args allowed
$ gunicorn module:factory()
@tilgovi I agree. My main issue is that we didn't expected that would break anyone, hence why i was suggesting to put back the eval (or something safer) and deprecate it. On the other hand, yes that behavior was never supported and was an unfortunate effect of using eval
:/
@davidism interesting. But how would it be different from using a callable object as an application?
I'm not sure what you mean, could you give a more specific example? A factory returns a WSGI application, it's not itself a WSGI application.
@davidism I mean something lke this
def make_app():
from mymodule.application import MainApp
return MainApp()
application = make_app()
then someone run gunicorn -b :8000 somemodule:application
That causes application
to always be evaluated when importing the code, defeating the purpose of the factory.
A WSGI callable can also be a class instance, so perhaps this is what was intended:
class Application:
_app = None
def __call__(self, environ, start_response):
if self._app is None:
from wherever import make_app
self._app = make_app()
return self._app(environ, start_response)
application = Application()
(The zope.hookable
example is essentially the same, just less overhead in the steady-state condition.)
Having a WSGI app that creates the real WSGI app is not ideal. It's now an extra function call on every request to proxy to the real entry point. Setup should be done before the first request, but now it's deferred until then, making the first request take (potentially a lot) longer.
The functionality in question here is a factory function that creates that object based on runtime / environment config, which is useful for decoupling the application parts, avoiding circular imports, and easier test isolation. Having somewhere in the code that explicitly calls the factory sort of defeats the decoupling purpose, as I guarantee you users will then think "oh, I should import this app object around now" when they should instead use the features available to them in Flask.
At this point, all we're asking for is "if the import string ends with parens, call the imported name to get the app".
I think we have an abundance of ways around this, but that just means we're not the target audience. I know that I can do things like ship a script that's outside the package as an entrypoint for my container and configure pytest to ignore it, etc, etc, but we want to care about the people for whom this breaks who are following tutorials and might not understand the traceback.
A very limited "if the app object is callable with zero arguments, then invoke it as a factory" pattern might work, but it fails if the callable is actually a WSGI application that's decorated badly and doesn't reveal its arguments so easily from introspection. If we want to be generous, we should support everything we had before while avoiding eval
, so I think that's the path we should do.
I really appreciate all the suggestions and help resolving this, everyone.
I like both the suggestions from @davidism and @connorbrinton using literal_eval
.
That causes application to always be evaluated when importing the code, defeating the purpose of the factory.
Well it would init the app on runtime and return a callable used by the workers. That's not that different.
My main reserve about that pattern is it encourage people to run some code pre-spawn which can break the expectations on HUP or USR2. Also it breaks the current UI. Will it work with future usages of gunicorn?
Anyway the choices are the following then:
(1) is a hard take, but also the logical path, considering we never supported it.
(2) some kind of aesthetic and breaks the command line UI, it needs some tests/examples to test in gunicorn, use something like literal_evals
We can support (2) but I would like to have some testing . Also should we document it?
@tilgovi @jamadden @berkerpeksag @sirkonst what is your preference?
Looks like another breaking use case was attribute access, which won't be solved by literal_eval
.
For example, in Plotly Dash you use the Dash
object, which internally has a Flask
instance as the server
attribute. Some people were using:
gunicorn "module:app.server"
I'm not sure this should be supported though. flask run
doesn't support it either. Seems like the Dash
object should have a __call__
method that passes to Flask.__call__
. Additionally, Dash's docs say to do server = app.server
and point Gunicorn at that, so this seems to be mostly a case of some incorrect information being passed around.
@davidism i have been sick today but I will have a look this sunday about it for a release on monday. The suggestion from @tilgovi is good and the general plan is to have a safe eval replacing the old eval.
Those I think we should warn the user he's using such initializtion. Thoughts ? cc @tilgovi
I'll try to turn the branch I linked above into a PR with tests on Saturday, unless you wanted to create a different implementation.
@davidism go ahead. I will be back on sunday where I will review it if neeed :) Thanks!
A little behind, working on this now.
@connorbrinton cool idea to use ast.parse
, I'll try that out and include you as a co-author on the commit if I go with it.
Just wanted to chime in that there is a somewhat popular (and quite old) Stack Overflow answer which is directing users towards the v19 behavior, which might need an update depending on what choice is made: https://stackoverflow.com/questions/8495367/using-additional-command-line-arguments-with-gunicorn
fixed in master. thanks @davidism for the patch!
all cases handled are in this tests: https://github.com/benoitc/gunicorn/commit/19cb68f4c3b55da22581c008659ee62d8c54ab2b#diff-5832adf374920d75d4bf48e546367f53R67
Most helpful comment
fixed in master. thanks @davidism for the patch!
all cases handled are in this tests: https://github.com/benoitc/gunicorn/commit/19cb68f4c3b55da22581c008659ee62d8c54ab2b#diff-5832adf374920d75d4bf48e546367f53R67