-
Notifications
You must be signed in to change notification settings - Fork 447
Use Python 3 in h Docker image #4901
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4901 +/- ##
==========================================
- Coverage 97.02% 97.02% -0.01%
==========================================
Files 444 444
Lines 24643 24672 +29
Branches 1320 1330 +10
==========================================
+ Hits 23910 23938 +28
- Misses 606 607 +1
Partials 127 127
Continue to review full report at Codecov.
|
24ed07b
to
8f67deb
Compare
8f67deb
to
1effd46
Compare
I wonder if we need to wait for a quiet time (e.g. when not so busy on groups work) to merge this... |
That's prob not be a bad idea @seanh. |
Dockerfile
Outdated
@@ -18,7 +18,7 @@ RUN apk add --no-cache \ | |||
libffi \ | |||
libpq \ | |||
nginx \ | |||
python2 \ | |||
python3 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If supervisor requires python2-do we still need to keep python2 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The py2-pip
dependency takes care of that. Strictly speaking this was redundant before.
Something to watch out for: the click library (which we use to implement the |
I agree it makes sense to pick a quiet time of day and sprint. I'd prefer to have a more definite timeline than "when we're done with groups work" so we can wrap this up. How about next Tuesday morning (London time) when it'll be the start of the sprint and fairly quiet traffic-wise? If there is a problem, we can just revert the single commit here and schedule work for another time.
I'm using Python 3 in my local dev environment and haven't encountered issues with the CLI tools. I've also done some basic testing of critical commands ( From reading the docs, the main issues flagged up are to do with CLI tools that read binary data from stdin/write to stdout. AFAIK, none of ours do. Regarding the section at the bottom about I/O encoding, Python is configured to use UTF-8 in the Docker image which also avoids several hazards. |
I think the start of the sprint sounds good. |
I'd like to wait at least another week from next Tuesday. I'd like some time to do a little Python 3 review of the code myself, but I'll be focusing on the groups stuff in the current sprint until end of Tuesday. The following Tuesday should work. |
OK, that's fine with me. Unless there are any objections I will mark Tuesday 10th April as 🐍🐍🐍-day. |
Thanks for the detailed investigation @seanh. I hadn't responded yet because I wanted to focus on other development tasks this week. I personally don't think it is absolutely necessary to do everything on that list before we switch over, in particular updating all dependencies (which has its own risks) and trying to get pylint clean. However I'm happy to let you noodle a way for a while and see what issues you can find. I'm sure it will be educational 🙂 |
1effd46
to
8d13129
Compare
8d13129
to
aa0e7b1
Compare
aa0e7b1
to
2f1fbe8
Compare
This may be useful to resolve the issue with supervisord still requiring Python 2 to be installed: https://github.com/ochinchina/supervisord (supervisord written in Go, and thus compiled to a static binary) |
2f1fbe8
to
a21c43e
Compare
a21c43e
to
fe258ba
Compare
Run h and its dependencies using Python 3. The stable version of supervisord (3.3.x) is not compatible with Python 3 so continue to install and run it with Python 2. This is due to be resolved with supervisord v4.x.
The stable version of supervisord still requires Python 2. To avoid having to install Python 2 alongside Python 3 for this one application, use a Golang implementation instead [1]. This makes the Python 3 Docker image only 4MB instead of 40MB larger than the Python 2 Docker image. [1] https://github.com/ochinchina/supervisord
fe258ba
to
bd17b9d
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I've deleted the big checklist comments above and moved them to a new issue: #5384 |
Transition the QA/production Docker image to use Python 3 to run the h web app and workers.
supervisord still requires Python 2, so to avoid having to install Python 2 alongside Python 3, I have replaced it with a Golang implementation which has all the features we need.
After this hits QA we should do some sanity testing of important parts of the site and check memory/CPU usage. After this hits production we should keep an eye on memory and CPU usage stats.
If we need to roll back for any reason, we can revert these commits and redeploy, or even just re-deploy the previous Docker image from the deployment build page.
Image size stats
Sizes reported by
docker inspect hypothesis/hypothesis:dev
:Image size (master): 154MB
Image size (this branch): 167MB
Testing
make docker
make run-docker