Skip to content

Add initial vscode devcontainer support #1309

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

Closed
wants to merge 1 commit into from

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Mar 14, 2020

Initial support for Vscode devcontainer. See #1305

@gabrieldemarmiesse
Copy link
Member

gabrieldemarmiesse commented Mar 14, 2020

Thank you for the pull request! Is there a way we could test in the CI that this setup is working?

All tools that we make use of to develop are tested in the CI to ensure that they work, that users don't spend time on something buggy and that we don't break it in the future.

@bhack
Copy link
Contributor Author

bhack commented Mar 14, 2020

I don't know how Microsoft Vscode team test It in CI. /cc @Chuxel any hint?

@bhack
Copy link
Contributor Author

bhack commented Mar 15, 2020

Seems to me that the test with the IDE is quite manual:
https://github.com/microsoft/vscode-dev-containers/blob/master/README.md#testing-a-definition

@bhack
Copy link
Contributor Author

bhack commented Mar 15, 2020

@gabrieldemarmiesse What is the scope of https://github.com/tensorflow/addons/blob/master/tools/docker/gpu_tests.Dockerfile#L14-L16.
Cause when you launch the devcontainer in vscode you have already the checkout in /workspace/adddons.
So in the "interactive dev mode" I don't think it is useful to have also /addons in the image.
What do you think?

@bhack
Copy link
Contributor Author

bhack commented Mar 15, 2020

Ok I've created another build target inside gpu_tests.Dockerfile.

@gabrieldemarmiesse
Copy link
Member

What is the scope of https://github.com/tensorflow/addons/blob/master/tools/docker/gpu_tests.Dockerfile#L14-L16.

It's to be able to lauch the container easily to run the gpu tests. It's being run with this script in google's CI if you want some background (github actions has no gpu, we need google's ci).

@bhack
Copy link
Contributor Author

bhack commented Mar 15, 2020

Ok, I've also edited that script.

@bhack
Copy link
Contributor Author

bhack commented Mar 16, 2020

@gabrieldemarmiesse I think it is done. We could just switch the URL before merge when microsoft/vscode-dev-containers#260 is merged upstream.
What else in the todo here?

@gabrieldemarmiesse
Copy link
Member

Seeing how this has grown into something a lot more complexe, that it's not possible to test automatically in the CI and that this is not critical to be able to work/debug addons with Visual Studio Code, I believe the best course of action is to split this functionality from TF addons. We need to be very selective with what we support. Adding dev code in this repository means that the maintainers are commiting to maintain it for years to come.

@bhack , could you either put the dockerfile and json in the https://github.com/microsoft/vscode-dev-containers repo, or create a repo in your account? We can then add a note in the CONTRIBUTING.md of Addons redirecting Visual Studio Code users to your repo. It's then clear who has the responsibility of maintenance/testing/updating/adding new features.

If we see that a significant portion of users use this setup (maybe count the number of stars/traffic on the repo?), we can always put it back in Tensorflow Addons to ensure maintenance.

@bhack
Copy link
Contributor Author

bhack commented Mar 16, 2020

Seeing how this has grown into something a lot more complexe, that it's not possible to test
automatically in the CI

I think this was a little bit of too expanded generalization.
We are already testing in this PR the new renamed target image with docker build -f tools/docker/gpu_tests.Dockerfile --target gpu_tests -t tfa_gpu_tests .

I could add the same tests execution also for interactive_dev target. Or do you want to test something else in that target to be sure that we are "developer-ready"? Do you have a build task/script that you want to verify for that target?

The only things that we cannot automate in CI (at least @Chuxel have some internal secrect receipt to pilot cmd code IDE) is only to test .devcontainer/devcontainer.json cause it works directly in Vscode IDE.
But i think for this point is the same situation also for other and bigger Google project as you can see in .devcontainer folder in the official Angular github repo.

and that this is not critical to be able to work/debug addons with Visual Studio Code, I believe the best course of action is to split this functionality from TF addons.

This will not work well in a third party repository cause:

  • Currently you are not publishing a base image like this PR new base target in any Docker public registry
  • Your original and maintained Dockerfile receipt uses many external scripts files that need to be replicated/forked in any third party repository to be sure that we are aligned.

We need to be very selective with what we support. Adding dev code in this repository means that the maintainers are commiting to maintain it for years to come.

This is pure virtual. It is correct only by a theoretical point of view but it is not a real world policy. Also bigger projects with a more consolidated community history like i.e. Debian has an healthy MIA policy. Honestly I don't think that that Vscode IDE will disappear just around the corner as it is currently one of the most popular IDE.

@bhack bhack force-pushed the vscode_devcontainer branch from 062dfb3 to 2443c5e Compare March 16, 2020 19:39
@bhack
Copy link
Contributor Author

bhack commented Mar 18, 2020

My last overview is confirmed as you can see in microsoft/vscode-remote-release#2570 (comment)

@@ -9,5 +9,5 @@ if [ "$DOCKER_BUILDKIT" == "" ]; then
export DOCKER_BUILDKIT=1
fi

docker build -f tools/docker/gpu_tests.Dockerfile -t tfa_gpu_tests ./
docker build -f tools/docker/gpu_tests.Dockerfile --target gpu_tests -t tfa_gpu_tests ./
docker run --rm -t --runtime=nvidia tfa_gpu_tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabrieldemarmiesse Do you want to run this also on interactive_dev? Or do you want to docker run something different there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabrieldemarmiesse An alternative is to execute bazel build --nobuild -- //tensorflow_addons/... if it is enough for testing interactive_dev.

@seanpmorgan
Copy link
Member

Firstly, I want to thank you for working on this @bhack, we very much appreciate the effort to streamline development on TF-Addons. I'm going to address some of your points and then we can work toward a solution.

The only things that we cannot automate in CI ... is only to test .devcontainer/devcontainer.json cause it works directly in Vscode IDE.

As a team we've decided that supporting individual IDEs within our repo is outside of what we feel comfortable maintaining. https://github.com/microsoft/vscode-dev-containers/tree/master/containers is a centralized repo for these devcontainers and includes the proper CI to test the underlying functionality within VSCode. Ultimately that is what needs to work and it should be tested.

But i think for this point is the same situation also for other and bigger Google project as you can see in .devcontainer folder in the official Angular github repo.

The decision to include .devcontainer in angular was made by a paid team that decided to support it. As a small team of almost entirely volunteer maintainers I find there little benefit to comparing the commitments they've made to the commitments we should make.

We need to be very selective with what we support. Adding dev code in this repository means that the maintainers are committing to maintain it for years to come.

This is pure virtual. It is correct only by a theoretical point of view but it is not a real world policy. Also bigger projects with a more consolidated community history like i.e. Debian has an healthy MIA policy. Honestly I don't think that that Vscode IDE will disappear just around the corner as it is currently one of the most popular IDE.

I struggle to see how being selective in what we agree to maintain is a "theoretical" decision. In reality we do not have a "healthy MIA policy" enforced by a team who tracks down negligent maintainers. The only way this repository is viable is if there are maintainers who are willing to serve as a back-up should proxy-maintainers fail. That's a difficult task for a team of volunteers and as such we have a responsibility to make sure we don't over extend ourselves. We've determined that supporting individual IDE components is outside of what we'd like to support.

This will not work well in a third party repository cause:

  • Currently you are not publishing a base image like this PR new base target in any Docker public registry
  • Your original and maintained Dockerfile receipt uses many external scripts files that need to be replicated/forked in any third party repository to be sure that we are aligned.

Ultimately this seems to be the crux of the problem. There already exists a central repository for dev containers and the correct path seems to be removing barriers to get this into the proper place. I've added this topic to the monthly meeting agenda and we'll discuss the best way to publish a developer container for TF-Addons. I can imagine a few different situations where it would be beneficial to have this available (for starters if other IDEs begin copying the devcontainer idea).

Would you be willing to re-create your PR to vscode after we've determined the best way to accomplish this?

@seanpmorgan
Copy link
Member

Closing this PR as we do not plan to merge this code, but please feel free to continue discussion here.

@bhack
Copy link
Contributor Author

bhack commented Mar 26, 2020

@seanmorgan If you never have something in It Is hard to find people interested to maintain It.
It Is a little bit a chicken and egg issue.
If the core team need to backup all It Is hard to expand on.
But could be still ok if you have a growing queue of addon maintainers candidates that are just waiting to be qualified/trusted. If not It Is Just a bottleneck. Just my 2c.

Let me know when you will have a published Docker image that developer can use at #1414 cause I don't want to maintain a fork of all the current Dockerfile machinery and scripts in the Microsoft repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants