adding gunicorn instead of uwsgi and modifying the base image. #362
+352
−289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…the new python 3.9
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
…m 8000 to 80, and changed the base image to point to the "quay" repo
…lways set gunicorn to run as user "nobody"
…it was causing issue. Also, changing the base image.
…ed by changing where the "git" command runs. I also had to install postgresql-devel in the runtime image so "local_settings" can be imported.
Feat/base image
|
Please find the ci env pod logs here |
|
Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Avantol13
reviewed
Oct 23, 2024
Dockerfile
Outdated
| FROM quay.io/cdis/python-build-base:${AZLINUX_BASE_VERSION} AS base | ||
|
|
||
| # Comment this in, and comment out the line above, if quay is down | ||
| # FROM 707767160287.dkr.ecr.us-east-1.amazonaws.com/gen3/python-build-base:${AZLINUX_BASE_VERSION} as base |
There was a problem hiding this comment.
is there a reason we're not preferring ECR?
There was a problem hiding this comment.
So that community may be able to build/ develop and I belive ECR has a bandwidth cost, while quay is free.
There was a problem hiding this comment.
is that acceptable for our own internal usage? Or do we need some way to automate flipping that to ECR as part of build?
Avantol13
reviewed
Oct 23, 2024
Dockerfile
Outdated
| USER gen3 | ||
|
|
||
|
|
||
| RUN python -m venv /venv |
There was a problem hiding this comment.
so poetry install will create a venv automatically when you install. And in fact, we're explicitly setting https://python-poetry.org/docs/configuration/#virtualenvsin-project, so it should be deterministic where the venv gets created. So as long as you run all future commands you want within the env with poetry run .... then it should just work.
I don't think we need (or want) any other venv setup or activation outside of the stock poetry support. It could create some confusing differences
Avantol13
reviewed
Oct 24, 2024
Dockerfile
Outdated
| ## For local development | ||
| # FROM quay.io/cdis/python-nginx-al2:feat_python-nginx AS base | ||
|
|
||
| FROM 707767160287.dkr.ecr.us-east-1.amazonaws.com/gen3/python-nginx-al2:feat_python-nginx as base |
There was a problem hiding this comment.
let's follow the warning and fix The 'as' keyword should match the case of the 'from' keyword
Avantol13
reviewed
Oct 24, 2024
docker-compose.yaml
Outdated
| @@ -0,0 +1,36 @@ | |||
| version: '3.8' | |||
There was a problem hiding this comment.
per previous conversations, I'd like to remove these separate ideas around local dev improvements to speed up the approval and review of the base image upgrades. We can have a separate PR and discussions to talk through ideas like this
Avantol13
reviewed
Oct 24, 2024
pyproject.toml
Outdated
| @@ -21,13 +21,13 @@ gen3authz = "^1.0.4" | |||
| hsclient = {git = "https://github.com/uc-cdis/hsclient", rev = "1.0.0"} | |||
| indexclient = "^2.1.0" | |||
| jsonschema = "^3.2" | |||
| psycopg2-binary = "^2.7" | |||
| gunicorn = "^22.0.0" | |||
There was a problem hiding this comment.
If we are not blocked from upgrading beyond these versions, I'd like to get in the habit of specifying >= rather than the default ^. If things break, we'll see it in unit tests and either update so the newer lib works, or at that point we'll restrict the versioning and create a ticket to upgrade.
As it is, we'll never automatically try to update to new major versions.
So, even though the other deps aren't like this yet, we could replace all ^ with >= at least with the new deps.
If you're feeling adventurous, we could try everything else too. With the exception of sqlalchemy, which I know won't work b/c 2.0 requires updates
Please find the detailed integration test report here Please find the ci env pod logs here |
|
Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am removing uwsgi to be replaced by gunicorn and making modifications to use the new python3.9 base image. I had to make some modifications for indexd to be compatible with the AL2 base image.