Fix docker

[shivanth]

#4134

[balopat]

Thank you, I think this looks good. As mentioned in the issue, can you add a test to ensure that it builds the right thing? I'm thinking of something simple, like, after building the image, assert that docker run cirq-image python -c "import cirq; assert cirq.__version__ is not None" succeeds or something similar.

[shivanth]

I've added have the test , but unfortunately will need to disable it since the Github actions image does not have docker in it. The added test is also a longrun test, it will increase the pipeline completion time.
Will add a pytest marker and remove this test from CI executions

[balopat]

This is great! We don't need the marker - we should just limit that test to Linux only.

[shivanth]

we should just limit that test to Linux only.

Is there a way to selectively run tests based on os type/version?
One other way would be to setup a different github action to run docker tests.

[balopat]

we should just limit that test to Linux only.

Is there a way to selectively run tests based on os type/version?
One other way would be to setup a different github action to run docker tests.

I'd pull out the @only_on_posix decorator from below to a file called dev_tools/markers.py, import it in dev_tools/__init__.py and then you can just use it as @dev_tools.only_on_posix:

Cirq/dev_tools/bash_scripts_test.py

Line 24 in 700a6c0

def only_on_posix(func):

[balopat]

we should just limit that test to Linux only.

Is there a way to selectively run tests based on os type/version?
One other way would be to setup a different github action to run docker tests.

I'd pull out the @only_on_posix decorator from below to a file called dev_tools/markers.py, import it in dev_tools/__init__.py and then you can just use it as @dev_tools.only_on_posix:

Cirq/dev_tools/bash_scripts_test.py

Line 24 in 700a6c0

def only_on_posix(func):

Hmm...the Mac platform might match posix. We should use platform.

Also, as it's just a single test, an even simpler approach is just to have a simple if statement on top of the test which tests for Linux and if it's not, then use pytest.skip()

[shivanth]

@balopat requesting review

[balopat]

I came up with a minimal python cirq installed docker image. This should serve as the base image for cirq docker

FROM python:3.8-slim AS compile-image

# Install dependencies.
# rm -rf /var/lib/apt/lists/* cleans up apt cache. See https://docs.docker.com/develop/develop-images/dockerfile_best-practices/
RUN DEBIAN_FRONTEND=noninteractive apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y \
     python3-pip \
     locales \
     && rm -rf /var/lib/apt/lists/*

# Configure UTF-8 encoding.
RUN sed -i -e 's/# en_US.UTF-8 UTF-8/en_US.UTF-8 UTF-8/' /etc/locale.gen && locale-gen
ENV LANG en_US.UTF-8  
ENV LANGUAGE en_US:en  
ENV LC_ALL en_US.UTF-8 

# Make python3 default
RUN rm -f /usr/bin/python \
     && ln -s /usr/bin/python3 /usr/bin/python

# Install cirq
RUN pip3 install cirq

We have to decide what we want to do with Docker.

1. A base docker image that has cirq pre-installed - this is the minimum requirement
2. Another image , like the above , for pre-release versions as well.
3. Since notebooks are a thing with cirq, it might be worthwhile investing in an image that can support notebooks out of the box.

I like the combined stable / pre multi-stage image.
The notebook idea is valid as well - but then we have to figure out how to copy the right notebooks to the image - some notebooks will only work with pre and won't work with the stable version...I guess the user can figure that out then and use the pre image?

[balopat]

LGTM, thank you, it's much better for now.
Eventually we can evolve this further if there is user feedback.

[CirqBot]

Automerge cancelled: A required status check is not present.

Missing statuses: ['cla/google']

[shivanth]

xref #3736