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

Simplify Docker image #815

Merged
merged 21 commits into from Dec 6, 2020
Merged

Simplify Docker image #815

merged 21 commits into from Dec 6, 2020

Conversation

dalcde
Copy link
Contributor

@dalcde dalcde commented Dec 1, 2020

Use python:3.8.2-buster as the base image. This reduces the final image
size by 500MB and significantly reduces the amount of total disk space
occupied.

There is room for further reducing the image size, by basing on
python:3.8.2-slim-buster, or even debian:buster itself. However, that
would require quite a bit more work. Basing on the non-slim version has
the advantage that all python build dependencies are automatically
included, because python:3.8.2-buster builds python from scratch and
keeps the build dependencies.

The xfvb part of the old Dockerfile has been removed, but not yet added
to the CircleCI config.

Partially addressed #810

Use python:3.8.2-buster as the base image. This reduces the final image
size by 500MB and significantly reduces the amount of total disk space
occupied.

There is room for further reducing the image size, by basing on
python:3.8.2-slim-buster, or even debian:buster itself. However, that
would require quite a bit more work. Basing on the non-slim version has
the advantage that all python build dependencies are automatically
included, because python:3.8.2-buster builds python from scratch and
keeps the build dependencies.

The xfvb part of the old Dockerfile has been removed, but not yet added
to the CircleCI config.
@dalcde
Copy link
Contributor Author

dalcde commented Dec 1, 2020

Since I don't have experience with CircleCI, some pointers in that direction would be helpful

@rth
Copy link
Member

rth commented Dec 1, 2020

Thanks @dalcde ! To use this image in CI of this PR you would need to build it, upload under some temporary path to Docker Hub and change the path in https://github.com/iodide-project/pyodide/blob/f7adad7eb30a54ce4ed82e6e15950931c77dd1cc/.circleci/config.yml#L6 Once it passes I will then be able to upload it under the pyodide account.

There are no tests in packages/micropip/micropip/. However, to determine
that, python needs distutils to be present in the host, and it is not.
Since firefox and chrome are run in headless mode. We probably don't
need xvfb...
@dalcde
Copy link
Contributor Author

dalcde commented Dec 2, 2020

I can reproduce the test_exceptions fail on my machine, with InternalError: too much recursion, as well as the test_compile ones with RecursionError: maximum recursion depth exceeded during compilation. The others pass on my machine, but the fact that test_open_url_cgi fails for both chrome and firefox is a bit suspect.

The docker container now contains libtinfo5
@dalcde
Copy link
Contributor Author

dalcde commented Dec 2, 2020

#716 explains/observes the test_compile fails.

@rth
Copy link
Member

rth commented Dec 2, 2020

Yes, for now maybe manually install the same firefox version as the previous setup. It shouldn't matter too much size wise I think. Then we can investigate this. I also leaves the flexibility to update firefox versions instead of being fixed what's provided by the distribution.

The test_open_url_cgi failure is indeed suspicious..

@dalcde
Copy link
Contributor Author

dalcde commented Dec 2, 2020

The test_open_url_cgi failure happens consistently with both browsers, and I believe is responsible for the pandas fail as well. However, I cannot reproduce it on my own machine, just on CI.

@rth
Copy link
Member

rth commented Dec 5, 2020

I also can't reproduce test_open_url_cgi failure with this image locally. Let me try to reset CI cache.

# Get firefox 70.0.1 and geckodriver
RUN wget -qO- https://ftp.mozilla.org/pub/firefox/releases/70.0.1/linux-x86_64/en-US/firefox-70.0.1.tar.bz2 | tar jx \
&& ln -s $PWD/firefox/firefox /usr/local/bin/firefox \
&& wget -qO- https://github.com/mozilla/geckodriver/releases/download/v0.26.0/geckodriver-v0.26.0-linux64.tar.gz | tar zxC /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of pipes!

@@ -119,7 +111,7 @@ jobs:
- run:
name: test
command: |
pytest src packages/*/test* packages/micropip/micropip/ pyodide_build -v -k chrome
pytest src packages/*/test* pyodide_build -v -k chrome
Copy link
Member

Choose a reason for hiding this comment

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

There idea there was to run micropip doctests, but you are right that there aren't any at the moment.

@dalcde
Copy link
Contributor Author

dalcde commented Dec 5, 2020 via email

@rth
Copy link
Member

rth commented Dec 5, 2020

do we actually want to cache emsdk?

I agree, we can probably stop caching it. I'm currently looking into refactoring CI following discussion in #76 (comment) and we can probably do it there once we have a better measure of the time to build emsdk in CI.

@rth
Copy link
Member

rth commented Dec 5, 2020

It's a permission issue. After connecting with SSH to the CI instance, in the test server logs I see,

[05/Dec/2020 19:20:23] source: 127.0.0.1:42216 - "GET /src/tests/data/data.cgi HTTP/1.1" 200 -
----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 42216)
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/http/server.py", line 1179, in run_cgi
    os.execve(scriptfile, args, env)
PermissionError: [Errno 13] Permission denied: '/root/repo/src/tests/data/data.cgi'
----------------------------------------
[05/Dec/2020 19:20:23] source: 127.0.0.1:42216 - CGI script exit status 0x7f00

at the same time the permissions should be OK,

root@d08cc4bdd647:~/repo# whoami
root
root@d08cc4bdd647:~/repo# ls -lh /root/repo/src/tests/data/data.cgi
-rwxr-xr-x 1 root root 78 Dec  5 19:17 /root/repo/src/tests/data/data.cgi

/usr/bin/env python in the header and just executing that script also works fine. So I'm not sure what's wrong exactly.

Even before this, I was thinking that we could replace our CGI test server with pytest-httpserver which in my experience is quite convenient, easier to debug and simpler. I'll look into it in a separate PR.

@rth rth added this to the 0.16.0 milestone Dec 5, 2020
Use python:3.8.2-buster as the base image. This reduces the final image
size by 500MB and significantly reduces the amount of total disk space
occupied.

There is room for further reducing the image size, by basing on
python:3.8.2-slim-buster, or even debian:buster itself. However, that
would require quite a bit more work. Basing on the non-slim version has
the advantage that all python build dependencies are automatically
included, because python:3.8.2-buster builds python from scratch and
keeps the build dependencies.

The xfvb part of the old Dockerfile has been removed, but not yet added
to the CircleCI config.
There are no tests in packages/micropip/micropip/. However, to determine
that, python needs distutils to be present in the host, and it is not.
Since firefox and chrome are run in headless mode. We probably don't
need xvfb...
@rth rth mentioned this pull request Dec 6, 2020
4 tasks
@rth
Copy link
Member

rth commented Dec 6, 2020

OK, I pushed this image as iodide/pyodide-env:8 with a change in versioning scheme to indicate that this image is not linked to pyodide releases schedule, and that its version might be incremented at any time.

Should be good to go as soon as CI passes. In terms of size it's indeed smaller by 500MB,

iodide/pyodide-env      8                     ba01d06f764a        3 minutes ago       1.75GB
iodide/pyodide-env      0.16.1                27c6ce8a246c        5 months ago        2.23GB

@rth rth merged commit 81df4e2 into pyodide:master Dec 6, 2020
@dalcde dalcde deleted the docker branch December 7, 2020 00:08
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.

None yet

2 participants