search

mu-semtech / mu-python-template

Template for running Python/Flask microservices
MIT License
4 stars 8 forks source link

Feature/use gunicorn in dev mode #19

Open sergiofenoll opened 10 months ago

sergiofenoll commented 10 months ago

This PR changes the template so that the same mechanism (gunicorn) that is used for running in production is also used in development mode. This should ensure that the behaviour of services using this template is consistent between development mode and production mode.

Why

With the previous setup, in development mode the template would use Flask to serve the code, which meant that route handlers can take as long as you want, whereas in production mode there's a timeout for workers set by Gunicorn, so a route handler can't take more than a certain amount of time before it gets aborted. This disparity is quite annoying because you might make something that works perfectly locally in dev-mode and then as soon as you deploy it you notice it breaks with errors you can't seem to reproduce locally.

How

Gunicorn provides a --reload option so that we don't lose the live code reload functionality of the template, but to get this working it was necessary to replace the worker class. It seems that the workers we used meinheld don't work well alongside gunicorn's reload mechanism: notice that in this issue the reporter is using meinheld workers. Meinheld itself also seems quite unmaintained, as the last commit for it was over 3 years ago and its official website has been squatted, so it might make sense to replace the worker class regardless.

[!WARNING] I know next to nothing about Gunicorn's architecture and the working of WSGI servers. I'm not really sure if the suggested worker is "better" than what we're currently using. So thorough testing with existing services is needed before merging this.

Nice extras

A lot of our services make use of apscheduler to build cronjobs. Due to how Flask's debug mode interacted with this library, in development mode usage of this library would result in 2 schedulers being instantiated. Every cron job would run twice. Although this didn't happen in production mode, it could be quite annoying/confusing when developing using this template. Switching to gunicorn for development mode has inadvertently resolved this. For some extra context see this StackOverflow post.

Encountered issues

Gunicorn's --reload mechanism seems to be broken for certain errors: https://github.com/benoitc/gunicorn/issues/2244 This would mean that the reloader stops working if there's e.g. a syntax error in your file while developing a service, which is quite likely to happen. To be determined if this gets fixed if we use a newer version of gunicorn (the currently used version is 2 years old). Adding inotify enables that mechanism for the reloader, in which case it can survive the mentioned errors. By changing the service-code import mechanism we can circumvent this issue. The reloader tracks the files it needs to listen to based on imports. Because we used to set the import inside a try-except block, the reloader wouldn't correctly pick up file changes if importing the file throws. By moving this import outside of the try-except block, we keep the reloader happy and also "fix" https://github.com/mu-semtech/mu-python-template/issues/12: a missing dependency or a syntax error in service-code will not silently print a single log line and set up a web server with no endpoints, instead it crashes the service.

MikiDi commented 10 months ago

I support this, since it only seems to make things better! The Gunicorn/Meinheld setup was introduced here, but was no more than a choice for a WSGI server rather than a specific commitment to Meinheld workers. Using this PR image in some of our frequently used dev setups will be the best way to test this further if we want to build more confidence before merging.

nvdk commented 9 months ago

@sergiofenoll (or someone with access to this repo): could you set up a build? that will make it easier to test :angel:

sergiofenoll commented 9 months ago

@nvdk I don't think I can enable the build under the semtech namespace (I think someone with write rights for the organization/repo would have to recreate the PR and use that to push).

I have enabled the Woodpecker on my fork though, you should be able to use this image: sergiofenoll/mu-python-template:feature-use-gunicorn-in-dev-mode

sergiofenoll commented 9 months ago

This is something I've noticed while using this locally: while SyntaxErrors get handled properly by the reloading mechanism, but other errors like NameErrors (which can occur from something as simple as trying to use a variable that hasn't been defined) cause the whole service to crash. See: https://github.com/benoitc/gunicorn/issues/2746

We could circumvent this by setting restart: always in the docker compose file when developing a Python template, but that's less than ideal. It might be worth it to look into adding an external reloader (like the JavaScript template has with nodemon) or implementing something using watchman or something like that.