You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/04/14 19:19:18 UTC
[GitHub] [incubator-mxnet] leezu opened a new pull request #18056: Fix
ci/docker_cache.py
leezu opened a new pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056
## Description ##
https://github.com/apache/incubator-mxnet/pull/17984 added a `cache_intermediate` option to `build_docker` enabling developers to speed up local docker builds during development. As no default value was provided, this broke the use of `build_docker` function in other places.
Reference: Failing `ci/jenkins/restricted-docker-cache-refresh` on master after merge: http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/restricted-docker-cache-refresh/detail/master/2789/pipeline
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408426593
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Well there has been some back and forth between refactors - maybe somebody removed it :/. Initially, I designed it that way that intermediary layers were kept since that's the whole point of the partial cache. This test validates that assumption: https://github.com/apache/incubator-mxnet/blob/master/ci/test_docker_cache.py#L184
Note that this test suite is not run in CI...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408515877
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Yep exactly. But please make sure that the tests pass before merging - otherwise we might break the partial caching.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408509652
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Good catch! I can't fully recall what was the thing around --rm=false, but I recall that I wrote tests to cover all these unknowns. Rule of thumb is that if the tests pass, we're good to go (given they use the same parameters of course).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408419434
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
One issue I noted during local development when setting this to `True`, is that my disk can fill up very quickly because Docker will not clean any of the cached layers automatically.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408419117
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
I don't mind changing it to `True`, but the existing behaviour before introducing the flag was `False`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408536446
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
This PR does not change the status quo. There is no risk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408509392
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Yeah it should be possible. It will just cause issues if the test fails since that might mean that stuff gets left dangling, but due to the mentioned lifecycle, that shouldn't have a big impact.
Feel free to enable it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408427070
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Well you can't have everything - either partial caching and manual cleanup or automatic cleanup but no partial caching. I think it is fine to expect that people use docker purge every now and then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu merged pull request #18056: Fix
ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408450992
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
Does the CI docker purge automatically? My docker cache quickly grow by 200GB after developing locally with the intermediate caching enabled. It's fine to purge time to time manually, but it's better to automate this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #18056: Fix
ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#issuecomment-613633563
Hey @leezu , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [edge, windows-cpu, centos-gpu, unix-cpu, unix-gpu, website, centos-cpu, sanity, windows-gpu, clang, miscellaneous]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408453141
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
> Note that this test suite is not run in CI...
Is it possible to run this suite in CI? If so, would you like to help enable it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408487032
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
For reference, you removed the `rm=false` yourself in f107397b753ff08bb19e7572bc1a9ebedd832f88
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408513268
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
> CI does not docker purge automatically. But since the lifecycle of unix slaves is very very short, it's not an issue
If the intermediate containers are only kept on the slave, and we only generate them in PRs that change the Dockerfile, I think we don't need to use them at all on CI. `--rm=false` then seems only useful for development.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408411038
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
This has to be true.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408509009
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
CI does not docker purge automatically. But since the lifecycle of unix slaves is very very short, it's not an issue. Also, in 99% of the cases, no new layers are built and only the cache is used, so the likelyhood of a single slave regenerating a lot of new mismatching layers is quite low. On a dev machine though, that's a different case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request
#18056: Fix ci/docker_cache.py
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18056: Fix ci/docker_cache.py
URL: https://github.com/apache/incubator-mxnet/pull/18056#discussion_r408411038
##########
File path: ci/build.py
##########
@@ -71,7 +71,7 @@ def get_docker_binary(use_nvidia_docker: bool) -> str:
def build_docker(platform: str, docker_binary: str, registry: str, num_retries: int, no_cache: bool,
- cache_intermediate: bool) -> str:
+ cache_intermediate: bool = False) -> str:
Review comment:
This has to be true. Otherwise, a single change will result in a full rebuild, drastically increasing build times.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services