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

Utilize SIG Build Docker Images #2515

Merged
merged 14 commits into from
Dec 28, 2021

Conversation

seanpmorgan
Copy link
Member

@seanpmorgan seanpmorgan commented Jul 1, 2021

Description

Move to managed SIG Build images.

@google-cla google-cla bot added the cla: yes label Jul 1, 2021
@bhack
Copy link
Contributor

bhack commented Jul 5, 2021

@angerson A TF 2.6 has still a python 3.6 wheel do you plan to push also a 3.6 image?

@bhack
Copy link
Contributor

bhack commented Jul 15, 2021

OK.. green lights here..

@bhack
Copy link
Contributor

bhack commented Nov 22, 2021

@bhack
Copy link
Contributor

bhack commented Nov 22, 2021

We are having some issues with the auditwheel patch now.
What was the origin of this patch? Is it still required?

#27 0.585 + sed -i 's/libresolv.so.2"/libresolv.so.2", "libtensorflow_framework.so.2"/g' /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json
#27 0.588 sed: can't read /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json: No such file or directory
#27 ERROR: executor failed running [/bin/sh -c bash tools/releases/tf_auditwheel_patch.sh]: exit code: 2
------

@seanpmorgan seanpmorgan changed the title [WIP] Testing new build image [WIP] Utilize SIG Build Docker Images Dec 22, 2021
@seanpmorgan
Copy link
Member Author

We are having some issues with the auditwheel patch now. What was the origin of this patch? Is it still required?

#27 0.585 + sed -i 's/libresolv.so.2"/libresolv.so.2", "libtensorflow_framework.so.2"/g' /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json
#27 0.588 sed: can't read /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json: No such file or directory
#27 ERROR: executor failed running [/bin/sh -c bash tools/releases/tf_auditwheel_patch.sh]: exit code: 2
------

Was solved in #2623

@seanpmorgan seanpmorgan mentioned this pull request Dec 22, 2021
@seanpmorgan
Copy link
Member Author

seanpmorgan commented Dec 23, 2021

We are having some issues with the auditwheel patch now. What was the origin of this patch? Is it still required?

#27 0.585 + sed -i 's/libresolv.so.2"/libresolv.so.2", "libtensorflow_framework.so.2"/g' /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json
#27 0.588 sed: can't read /usr/local/lib/python3.8/dist-packages/auditwheel/policy/policy.json: No such file or directory
#27 ERROR: executor failed running [/bin/sh -c bash tools/releases/tf_auditwheel_patch.sh]: exit code: 2
------

Just for visibility the reason for this auditwheel patch is to specify that tensorflow shared library should be added to the approved manylinux2010 libraries. If we remove this it will fail to export a manylinux2010 package.

SIG IO does a similar thing: https://github.com/tensorflow/io/blob/master/tools/build/auditwheel

@seanpmorgan seanpmorgan changed the title [WIP] Utilize SIG Build Docker Images Utilize SIG Build Docker Images Dec 23, 2021
@seanpmorgan
Copy link
Member Author

Nice to be able to cut a lot of this tech debt from the build file

@boring-cyborg boring-cyborg bot added the github label Dec 23, 2021
@seanpmorgan seanpmorgan changed the title Utilize SIG Build Docker Images [WIP] Utilize SIG Build Docker Images Dec 23, 2021
@seanpmorgan seanpmorgan removed the request for review from bhack December 23, 2021 07:10
Copy link
Contributor

@bhack bhack left a comment

Choose a reason for hiding this comment

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

@@ -1,11 +1,9 @@
#syntax=docker/dockerfile:1.1.5-experimental
FROM gcr.io/tensorflow-testing/nosla-cuda11.2-cudnn8.1-ubuntu18.04-manylinux2010-multipython as dev_container_cpu
ARG PY_VERSION
FROM tensorflow/build:latest-python$PY_VERSION as dev_container_cpu
Copy link
Contributor

@bhack bhack Dec 23, 2021

Choose a reason for hiding this comment

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

If you can add here a fake but future-proof IMAGE_TYPE arg I think that we could remove the overhead of maintaining another separated Dockerfile (if you suppose that the our devel and our Vscode/Codespaces images could be almost the same one):

ARG IMAGE_TYPE=latest-cpu
FROM tfaddons/dev_container:$IMAGE_TYPE

The main thing we are doing on that additional Dockerfile, other then let the user controlling the IMAGE_TYPE when they need the GPU image (currently not available) is that we are adding a default (guid/uid) non root user in the image build.

If we exclude that, with the unification we need to evaluate if without a default non root user, we can still official suggest that we prefer/support only Docker in rootless mode. This is currently the SIG Build policy that doesn't want to add a "default" user in the image.
It is hard to support both as rootless Docker vs root Docker are mutually exclusive for root and non root users permission on the host mounted volumes:

moby/moby#41497
https://discuss.tensorflow.org/t/adopting-open-source-dockerfiles-for-official-tf-nightly-ci/6050/9

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the holder for IMAGE_TYPE. And updated to push it correctly as gpu dev container. I think it should be good to merge and we can revisit once we have a means to get a cpu image.

@seanpmorgan seanpmorgan changed the title [WIP] Utilize SIG Build Docker Images Utilize SIG Build Docker Images Dec 28, 2021
@seanpmorgan
Copy link
Member Author

Made a new PR for the release:
tensorflow/community#406

@seanpmorgan seanpmorgan merged commit d19c3ef into tensorflow:master Dec 28, 2021
@seanpmorgan seanpmorgan deleted the new-build-image branch December 28, 2021 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants