Skip to content

Making run_docker.sh faster. #870

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
gabrieldemarmiesse opened this issue Jan 13, 2020 · 5 comments
Closed

Making run_docker.sh faster. #870

gabrieldemarmiesse opened this issue Jan 13, 2020 · 5 comments
Labels
test-cases Related to Addons tests

Comments

@gabrieldemarmiesse
Copy link
Member

Describe the feature and the current behavior/state.

Using docker to make the life of contributors easier is a very good idea. But the current implementation has a drawback (that we can fix).

Currently running any command with run_docker.sh installs dependencies even if the docker image is already present locally. For example every time I call:

bash tools/run_docker.sh -c 'make unit-test'

It downloads and install tensorflow, wich takes several minutes. Same for bash tools/run_docker.sh -c 'make code-format' which installs clang format every time.

What I propose is to make a dockerfile for each of those commands and install the necessary dependencies in it. Therefore, running bash tools/run_docker.sh -c 'make code-format' would takes only a few seconds the second time and we would avoid wasting bandwidth.

Relevant information

  • Are you willing to contribute it (yes/no): maybe
  • Are you willing to maintain it going forward? (yes/no): maybe
  • Is there already an implementation in another framework? (if so, where): In keras-tuner

Who will benefit with this feature?

Contributors

Any other info.

@seanpmorgan
Copy link
Member

So typically I would recommend that a developer use the docker image in interactive mode for repeated testing:
https://github.com/tensorflow/addons/blob/master/CONTRIBUTING.md#run-manually

We have yet to provide a docker container for Addons (requires hosting, and taking on responsibility for it -- but that's managable). One issue I see is that on the master branch the dependency tf-nightly is updated daily. This would require pulling a new latest image each day.

@tensorflow/sig-addons-maintainers thoughts?

@seanpmorgan seanpmorgan added test-cases Related to Addons tests and removed test-cases Related to Addons tests labels Jan 14, 2020
@WindQAQ
Copy link
Member

WindQAQ commented Jan 14, 2020

So typically I would recommend that a developer use the docker image in interactive mode for repeated testing:
https://github.com/tensorflow/addons/blob/master/CONTRIBUTING.md#run-manually

I also use docker in this way.

We have yet to provide a docker container for Addons (requires hosting, and taking on responsibility for it -- but that's managable). One issue I see is that on the master branch the dependency tf-nightly is updated daily. This would require pulling a new latest image each day.

@tensorflow/sig-addons-maintainers thoughts?

This would be problem. Just wonder that compared with re-installing tensorflow, would pulling a new commit from dockerhub be faster/smaller?

@gabrieldemarmiesse
Copy link
Member Author

gabrieldemarmiesse commented Jan 16, 2020

We have yet to provide a docker container for Addons (requires hosting, and taking on responsibility for it -- but that's managable).

I'm not sure it's necessary. A well written Dockerfile is a reproducible docker build command. Contributors can just build the image the first time they use it.

One issue I see is that on the master branch the dependency tf-nightly is updated daily. This would require pulling a new latest image each day.

That's exactly why I'm not so fond of using tf-nightly to run our tests. It's not even reproducible from day to day. We can't also use git bisect because of that, and git bisect is a fantastic tool to pinpoint the origin of a breakage.

@gabrieldemarmiesse
Copy link
Member Author

See #1022 and #1021 where there is no need to use the interactive mode to have fast formatting and sanity check.

@gabrieldemarmiesse
Copy link
Member Author

See also #1030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-cases Related to Addons tests
Projects
None yet
Development

No branches or pull requests

3 participants