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

Update presidio containers to use gunicorn #1497

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RKapadia01
Copy link

Change Description

  • Modified the main application entry points in presidio-analyzer/app.py, presidio-anonymizer/app.py, and presidio-image-redactor/app.py to use the app instance directly instead of creating a new Server instance when running the application.
  • Added PORT and WORKERS environment variables to the Dockerfiles for all 3 services, to allow configuration of the server port and number of worker processes.
  • Updated the command from Flaks to Gunicorn in the Dockerfiles

Issue reference

This PR fixes issue #1496

Checklist

  • I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

@omri374
Copy link
Contributor

omri374 commented Dec 12, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

SharonHart
SharonHart previously approved these changes Dec 12, 2024
Copy link
Contributor

@SharonHart SharonHart left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks :) I've added two minor points

presidio-analyzer/Dockerfile Show resolved Hide resolved

[tool.poetry.extras]
server = ["flask"]
server = ["flask", "gunicorn"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the gunicorn license into the NOTICE file.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the NOTICE file, thanks

@@ -135,8 +135,9 @@ def supported_entities() -> Tuple[str, int]:
def http_exception(e):
return jsonify(error=e.description), e.code

server = Server()
Copy link
Contributor

@omri374 omri374 Dec 12, 2024

Choose a reason for hiding this comment

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

I might be nit picking, but I'm curious on what is the difference between instantiating Server here compared to within the if name == main. For instance, if I import app.py, the Server instance would be instantiated. I'm not sure if importing app.py is a use case, but it would be a side effect of this change in case somebody does this anywhere. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Thats a great pickup. When running with Gunicorn, it doesn't execute the code in the__main__ block. It just imports the module and looks for the app object to serve the application, which is why I moved the server code out of the __main__ block.

However, I didn't remove the __main__ block entirely, so if anyone wants to run just the flask development server they still have to ability to do so.

To avoid your concern of instantiating the server on import of app.py, we can avoid it by doing something like this:

def create_app():
    server = Server()
    return server.app

if __name__ == "__main__":
    app = create_app()
    port = int(os.environ.get("PORT", DEFAULT_PORT))
    app.run(host="0.0.0.0", port=port)

We can then run Gunicorn in the Dockerfile like this:

CMD poetry run gunicorn -w $WORKERS -b 0.0.0.0:$PORT 'app:create_app()'

This should ensure that if anyone is just importing app.py it won't create the server instance. If you're happy with this, I'll commit the changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! This is a better solution IMHO so let's go with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants