Publish dev container #1888

seanpmorgan · 2020-05-26T01:55:39Z

Closes #1414

Discussion Points:

This adds a bit of computation time to the action on commit push. I don't think this is a big deal as this doesn't slow down feedback to contributors
There is no addons repository included in the docker image. I believe this is the correct pattern since we want users to load their own repository within the container
Because of 2 there is no pre-compiled bazel cache. We could add it but then the calls to bazel in the repository would need to be pointed to that cache. Given that our build only takes a couple of minutes I'd prefer to do this in another PR if we want to.

I tested the image in VScode using a dev-container and it worked as expected.

.github/workflows/build_dev_container.sh

bhack · 2020-05-26T10:29:34Z

I am looking at the compressed size of devel image TF vs TFA:

https://hub.docker.com/r/tensorflow/tensorflow/tags
https://hub.docker.com/r/tfaddons/tensorflow/tags

The TF CPU devel is 1/3 of our devel image and we have 1GB of compressed overhead over tensorflow GPU devel (uncompressed our is 9.2GB).

bhack · 2020-05-26T13:01:50Z

I think it could be useful to have a default small size 1GB image to lower the download overhead but I think that is still not possible to handle multiple configuration (GPU/CPU) in the dev-containers microsoft/vscode-remote-release#2143 (comment).

The new devcontainer support PR is at microsoft/vscode-dev-containers#347

bhack · 2020-05-26T15:41:21Z

Could you add

sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-8 100
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-8 100

bhack · 2020-05-26T16:17:56Z

If you could set the alias also for clang-format it could be useful

seanpmorgan · 2020-05-27T00:34:34Z

I am looking at the compressed size of devel image TF vs TFA:

https://hub.docker.com/r/tensorflow/tensorflow/tags
https://hub.docker.com/r/tfaddons/tensorflow/tags

The TF CPU devel is 1/3 of our devel image and we have 1GB of compressed overhead over tensorflow GPU devel (uncompressed our is 9.2GB).

I think it could be useful to have a default small size 1GB image to lower the download overhead but I think that is still not possible to handle multiple configuration (GPU/CPU) in the dev-containers microsoft/vscode-remote-release#2143 (comment).

As discussed offline this is a consequence of tensorflow/tensorflow#38352 . There is not much we can do without deviating from the custom-op recommended images.

seanpmorgan · 2020-05-27T00:38:17Z

Could you add

sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-8 100
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-8 100

Sure, but could you expand on why its needed. All of our documentation uses gcc and nvcc to compile. I know there was work on getting TF to compile using cuda-clang but not sure if we want to encourage that?

bhack · 2020-05-27T00:47:44Z

I've not checked in detail where we use what but I was thinking as a general rule to not going to ovveride to much settings properties in vsocde devcontainer about specific strings or to monitor if we change clang version cause there we are on another repository.
I prefer that we have the control in the image we are generating so we have constant alias.
If we don't care about clang probably we could do It only for clang-format?

CONTRIBUTING.md

bhack · 2020-05-30T15:59:01Z

I think that we need to update a little bit https://github.com/tensorflow/addons/blob/master/CONTRIBUTING.md after merging this. Can you open a ticket as a reminder?

tools/docker/dev_container.Dockerfile

tools/install_deps/clang-format.sh

bhack · 2020-06-08T14:34:26Z

tools/docker/dev_container.Dockerfile

@@ -0,0 +1,25 @@
+#syntax=docker/dockerfile:1.1.5-experimental
+FROM tensorflow/tensorflow:2.1.0-custom-op-ubuntu16 as dev_container_cpu


Here you lost the build ARGS to prepare the switch between cpu and gpu that was in https://github.com/seanpmorgan/addons/pull/14/files#diff-76fe3c36c889ccfcf62f6e35439d030fR3-R5

bhack · 2020-06-08T14:39:30Z

tools/build_dev_container.sh

+
+set -x -e
+
+DOCKER_BUILDKIT=1 docker build \


We could duplicate this command with the build_args to publish the GPU image. See the ARGS comment below.

bhack · 2020-06-08T14:40:22Z

.github/workflows/release.yml

+          set -e -x
+          docker login --username ${{ secrets.DOCKER_USER }} --password ${{ secrets.DOCKER_PW }}
+          bash bash .github/workflows/github_build_dev_container.sh
+          docker push tfaddons/dev_container:latest-cpu


We could duplicate this command to push the latest without suffixes (that is the GPU image)

bhack · 2020-06-16T09:49:32Z

As a reminder if you merge #1808 you need to add the requirements here.

bhack · 2020-07-02T13:08:25Z

CONTRIBUTING.md


 Compile the custom ops
 ```
+export TF_NEED_CUDA=1 # If GPU is to be used


I think that we need to set this only in the GPU image

Yes, but also if it's built from source locally.

bhack · 2020-07-10T14:15:19Z

custom-ops images are not produced for RC https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/ci_build/release/ubuntu_16/custom_op/release.sh#L39

# Conflicts: # CONTRIBUTING.md

seanpmorgan · 2020-09-29T02:30:53Z

@bhack @tensorflow/sig-addons-maintainers so there are still a few sharp edges here, but it aligns with the recent merge of SIG-IO:
tensorflow/io#1087

I would prefer that we merge this semi-hardcoded cpu devel build and then I'll open an issue and start work on a seperate GPU build. This got a bit stale and it'd be easier to get this merged and then upgrade it IMO. Also waiting for clarity from SIG-Build about custom-op images and their future. Does that seem okay?

* Add dev container build * Add docker upload on commit push * Modify pre-commit to work in dev container

seanpmorgan added 4 commits May 25, 2020 21:36

* Add dev container build

1222eea

* Add docker upload on commit push

e8489e3

* Modify pre-commit to work in dev container

6058361

* Move to workflows since we need to run a command to clear space

f77e5d8

boring-cyborg bot added the github label May 26, 2020

googlebot added the cla: yes label May 26, 2020

bhack reviewed May 26, 2020

View reviewed changes

.github/workflows/build_dev_container.sh Outdated Show resolved Hide resolved

bhack mentioned this pull request May 26, 2020

Tensorflow addons microsoft/vscode-dev-containers#347

Merged

seanpmorgan changed the title ~~Publish dev container~~ [WIP] Publish dev container May 27, 2020

This comment has been minimized.

Sign in to view

seanpmorgan added 2 commits May 27, 2020 20:44

* Update dockerfile dependencies

f85e2af

* Modularize build script

8b0f05e

bhack reviewed May 28, 2020

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bhack mentioned this pull request May 29, 2020

Try to use tensorflow:devel baselines seanpmorgan/addons#13

Closed

This comment has been minimized.

Sign in to view

bhack mentioned this pull request Jun 6, 2020

Handle the so loading in dev mode #1909

Merged

seanpmorgan added 3 commits June 7, 2020 22:27

Update to CPU only dev container for now

e456790

* Symlink clang-format

0b53e58

* Minimize diff

f2f782f

seanpmorgan changed the title ~~[WIP] Publish dev container~~ Publish dev container Jun 8, 2020

bhack reviewed Jun 8, 2020

View reviewed changes

tools/docker/dev_container.Dockerfile Show resolved Hide resolved

bhack reviewed Jun 8, 2020

View reviewed changes

tools/install_deps/clang-format.sh Show resolved Hide resolved

bhack reviewed Jun 8, 2020

View reviewed changes

* Install requirements

4fd6fb3

bhack reviewed Jul 2, 2020

View reviewed changes

bhack mentioned this pull request Aug 29, 2020

Bump black #2135

Merged

Merge branch 'master' into dev-container

e89a515

# Conflicts: # CONTRIBUTING.md

bhack approved these changes Sep 29, 2020

View reviewed changes

seanpmorgan merged commit 6552d57 into tensorflow:master Sep 29, 2020

seanpmorgan deleted the dev-container branch September 29, 2020 19:03

jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020

Publish dev container (tensorflow#1888)

cf96fc8

* Add dev container build * Add docker upload on commit push * Modify pre-commit to work in dev container

		@@ -0,0 +1,25 @@
		#syntax=docker/dockerfile:1.1.5-experimental
		FROM tensorflow/tensorflow:2.1.0-custom-op-ubuntu16 as dev_container_cpu

Publish dev container #1888

Publish dev container #1888

Uh oh!

Conversation

seanpmorgan commented May 26, 2020

Uh oh!

Uh oh!

bhack commented May 26, 2020

Uh oh!

bhack commented May 26, 2020

Uh oh!

bhack commented May 26, 2020

Uh oh!

bhack commented May 26, 2020

Uh oh!

seanpmorgan commented May 27, 2020

Uh oh!

seanpmorgan commented May 27, 2020

Uh oh!

bhack commented May 27, 2020

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

bhack commented May 30, 2020

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

bhack Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

bhack Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

bhack Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhack commented Jun 16, 2020

Uh oh!

bhack Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

seanpmorgan Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

bhack commented Jul 10, 2020

Uh oh!

seanpmorgan commented Sep 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bhack Jun 8, 2020 •

edited

Loading