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

Include Production WSGI (Gunicorn) to replace Flask Default Server #263

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

Conversation

KaiyiLiu1234
Copy link
Collaborator

To Address: #259

A side effect of using Gunicorn is that it will replace the address with default address and default port (8000). This can be resolved by binding (ex. binding model_server to 0.0.0.0:8100). Binding with 0.0.0.0 is acceptable but according to Flask, it is better to also introduce nginx server to act as a reverse proxy. Will this be necessary to include?

Also, since model_server AND offline_trainer (and in future online_trainer) expects to use their own Flask Apps, supervisord is needed to run multiple CMD for model_server and offline_trainer in Dockerfile.

Currently, this changes here works for metal docker compose so long as python3.8 -u src/server/model_server.py is replaced by gunicorn -b -0.0.0.0.8100 -src.server.model_server:app. However, there is more functionality that can be introduced depending on what is acceptable or not.

Add Gunicorn to Dockerfile

Signed-off-by: Kaiyi <[email protected]>
@sthaha
Copy link
Contributor

sthaha commented Jun 14, 2024

@KaiyiLiu1234 https://developers.redhat.com/articles/2023/08/17/how-deploy-flask-application-python-gunicorn#containerization is good reference to follow

Binding with 0.0.0.0 is acceptable but according to Flask, it is better to also introduce nginx server to act as a reverse proxy. Will this be necessary to include?

I am not sure if the recommendation applies to containers / k8s.

Gunicorn should not be run as root because it would cause your application code to run as root, which is not secure. However, this means it will not be possible to bind to port 80 or 443. Instead, a reverse proxy such as nginx or Apache httpd should be used in front of Gunicorn.

The part applies to containers is don't run as root. However, binding to port 80/443 isn't needed for containers.

You can bind to all external IPs on a non-privileged port using the -b 0.0.0.0 option. Don’t do this when using a reverse proxy setup, otherwise it will be possible to bypass the proxy.

Again, this is fine for a containerized env. I feel following the article linked should be good enough to fix our issue.

Included Gunicorn Instances for Model Server, Online Trainer (inactive),
and Offline Trainer. All three of these applications make use of one
Flask App, so three Gunicorn Instances should be set up to manage them
all.

Signed-off-by: Kaiyi <[email protected]>
Incorporate Gunicorn Server setup for Model Server and Offline
Trainer. The setup is currently placed in a dockerfile test
as changes need to be made in using the Kepler Model Server
image with gunicorn rather than with python.

Signed-off-by: Kaiyi <[email protected]>
@KaiyiLiu1234 KaiyiLiu1234 changed the title WIP Include Production WSGI (Gunicorn) to replace Flask Default Server Include Production WSGI (Gunicorn) to replace Flask Default Server Jun 26, 2024
@KaiyiLiu1234
Copy link
Collaborator Author

@sthaha @sunya-ch I incorporated the Gunicorn changes into a test dockerfile for now. How we interact with the image will change now in docker composes and kubernetes resources (for instance command should not be python model_server.py - it should instead use gunicorn or no command as the CMD for the dockerfile can handle launching the gunicorn servers for model server, offline trainer and online trainer). I can also include in the makefile commands to run the new gunicorn test dockerfile in this PR or in a future PR?

@KaiyiLiu1234 KaiyiLiu1234 requested a review from sthaha June 26, 2024 13:03
Added Makefile Tests which will easily build, run, and clean
 Model Server with Gunicorn Servers for Model Server and Offline
Trainer.

Signed-off-by: Kaiyi <[email protected]>
Makefile Outdated
@@ -51,27 +55,42 @@ run-model-server:
$(CTR_CMD) run -d --platform linux/amd64 -e "MODEL_TOPURL=http://localhost:8110" -v ${MODEL_PATH}:/mnt/models -p 8100:8100 --name model-server $(TEST_IMAGE) /bin/bash -c "python3.8 tests/http_server.py & sleep 10 && python3.8 src/server/model_server.py"
while ! docker logs model-server | grep -q Serving; do echo "waiting for model-server to serve"; sleep 5; done

run-model-server-gunicorn-complete:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run-model-server-gunicorn-complete:
run-model-server-prod:

Makefile Outdated
@@ -51,27 +55,42 @@ run-model-server:
$(CTR_CMD) run -d --platform linux/amd64 -e "MODEL_TOPURL=http://localhost:8110" -v ${MODEL_PATH}:/mnt/models -p 8100:8100 --name model-server $(TEST_IMAGE) /bin/bash -c "python3.8 tests/http_server.py & sleep 10 && python3.8 src/server/model_server.py"
while ! docker logs model-server | grep -q Serving; do echo "waiting for model-server to serve"; sleep 5; done

run-model-server-gunicorn-complete:
$(CTR_CMD) run -d --platform linux/amd64 -e "MODEL_TOPURL=http://localhost:8110" -v ${MODEL_PATH}:/mnt/models -p 8105:8105 -p 9109:9109 --name model-server-gunicorn-complete $(GUNICORN_TEST_IMAGE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$(CTR_CMD) run -d --platform linux/amd64 -e "MODEL_TOPURL=http://localhost:8110" -v ${MODEL_PATH}:/mnt/models -p 8105:8105 -p 9109:9109 --name model-server-gunicorn-complete $(GUNICORN_TEST_IMAGE)
$(CTR_CMD) run -d \
--platform linux/amd64 \
-e "MODEL_TOPURL=http://localhost:8110" \
-v ${MODEL_PATH}:/mnt/models \
-p 8105:8105 -p 9109:9109 \
--name model-server-prod \
$(GUNICORN_TEST_IMAGE)

Comment on lines 4 to 10
workers = int(os.environ.get('GUNICORN_PROCESSES', '2'))

threads = int(os.environ.get('GUNICORN_THREADS', '4'))

port = os.environ.get('GUNICORN_PORT', '8100')

bind = os.environ.get('GUNICORN_BIND', '0.0.0.0:' + port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
workers = int(os.environ.get('GUNICORN_PROCESSES', '2'))
threads = int(os.environ.get('GUNICORN_THREADS', '4'))
port = os.environ.get('GUNICORN_PORT', '8100')
bind = os.environ.get('GUNICORN_BIND', '0.0.0.0:' + port)
workers = int(os.environ.get('GUNICORN_PROCESSES', '2'))
threads = int(os.environ.get('GUNICORN_THREADS', '4'))
port = os.environ.get('GUNICORN_PORT', '8100')
bind = os.environ.get('GUNICORN_BIND', '0.0.0.0:' + port)

Copy link
Contributor

Choose a reason for hiding this comment

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

how about we call it gunicorn_config.py ?

Copy link
Contributor

Choose a reason for hiding this comment

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

lets keep config under a config dir

Comment on lines 32 to 33
# ENTRYPOINT ["python3.8", "cmd/main.py"]
#ENTRYPOINT [ "python3.8", "-u", "src/server/model_server.py" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# ENTRYPOINT ["python3.8", "cmd/main.py"]
#ENTRYPOINT [ "python3.8", "-u", "src/server/model_server.py" ]

Comment on lines 11 to 15
COPY src/estimate src/estimate
COPY src/server src/server
COPY src/train src/train
COPY src/util src/util
COPY cmd cmd
Copy link
Contributor

Choose a reason for hiding this comment

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

why not copy the entire src and cmd ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is my bad that when we run the test the built ./model will be inside the src folder. We need to fix that first then the src will be cleaned enough for entire copy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can check it out.

@@ -18,4 +18,5 @@ EXPOSE 8101
# port for Offline Trainer
EXPOSE 8102


Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated change?

Fix code errors requested by sunil.

Signed-off-by: Kaiyi <[email protected]>
@KaiyiLiu1234 KaiyiLiu1234 requested a review from sthaha July 6, 2024 01:17
cmd/gunicorn.sh Outdated
Comment on lines 3 to 9
echo "Starting Model Server"
export GUNICORN_PORT=$PORT_MODEL_SERVER
gunicorn --config config/gunicorn_config.py src.server.model_server:app &
echo "Starting Offline Trainer"
export GUNICORN_PORT=$PORT_OFFLINE_TRAINER
gunicorn --config config/gunicorn_config.py src.train.offline_trainer:app &
wait
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create separate containers for model-server and trainer?
This removes the issue of handling (unexpected) termination.

# port for Offline Trainer
EXPOSE ${PORT_OFFLINE_TRAINER}

CMD ["sh", "cmd/gunicorn.sh"]
Copy link
Contributor

@sthaha sthaha Jul 6, 2024

Choose a reason for hiding this comment

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

Is the idea to replace current Dockerfile with this?

@KaiyiLiu1234 KaiyiLiu1234 changed the title Include Production WSGI (Gunicorn) to replace Flask Default Server WIP:Include Production WSGI (Gunicorn) to replace Flask Default Server Jul 13, 2024
Separated Gunicorn Offline Trainer and Model Server into separate
containers.

Signed-off-by: Kaiyi <[email protected]>
@KaiyiLiu1234 KaiyiLiu1234 changed the title WIP:Include Production WSGI (Gunicorn) to replace Flask Default Server Include Production WSGI (Gunicorn) to replace Flask Default Server Jul 24, 2024
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