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/22 19:27:23 UTC
[GitHub] [incubator-mxnet] leezu opened a new pull request #18138: CI: Consolidate Ubuntu Dockerfiles
leezu opened a new pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138
## Description ##
Follow-up https://github.com/apache/incubator-mxnet/pull/18115 for Ubuntu CPU and GPU CI images.
See #18115.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413415773
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
This is to not invalidate docker cache at COPY if only ignored files changed. As we only COPY stuff from ci/docker folder, only that older should be considered.
I'm not sure what use case you are referring to
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413424549
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
I'm not sure if you're trying to solve a non-existent problem. Cache does not get invalidated because a file in the context changed. You could add your entire hard drive as the context and the cache would work fine as long as that particular file you're referencing stays the same.
The reason you do not want to add the root as context is simply for efficiency reason - it's multiple gigabyte in size. The current setup already takes care of that by specific the subfolder as context and everything else was already handled.
Since we're not changing docker mechanics here, the context and dockerignore parts shouldn't be changed either.
Yeah, let's investigate that separately. At the moment I don't think that the problem you're describing actually exists - or I might have misunderstood 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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413517317
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
> Cache does not get invalidated because a file in the context changed.
This assumption appears wrong: https://stackoverflow.com/a/48553814/2560672
Supposedly all non-ignored files are included in the computation of the hash used to lookup the cache, when using COPY. That's why avoiding COPY results in more cache hits.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413517317
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
> Cache does not get invalidated because a file in the context changed.
This assumption is wrong. Take a look at https://stackoverflow.com/a/48553814/2560672
All non-ignored files are included in the computation of the hash used to lookup the cache
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413400508
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
Could you elaborate? I'm afraid users who actually want to do add the mxnet folder might run into issues it we define this. Can't we just set that path as context root?
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413422963
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
There may be different points of reference. When moving the `.dockeignore` to the `ci` folder, cache get's invalidated due to file changes (eg. inside `.git`).
Ie. once the dockerfile is moved, the following cache invalidation occurs
```
Step 5/15 : RUN cd /usr/local/src && git clone --recursive https://github.com/ccache/ccache.git && cd ccache && git checkout v3.7.9 && ./autogen.sh && ./configure --disable-man && make -j$(nproc) && make install && cd /usr/local/src && rm -rf ccache
---> Using cache
---> 63a51833e720
Step 6/15 : COPY install/requirements /work/
---> 95beeff2e471
```
In any case, I'll remove the `.dockerignore` file as we're getting rid of `COPY` for the time-intensive parts of the build. The addition of `.dockerignore` can be investigated in a separate PR as there's already too many changes in this PR.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413415773
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
This is not to invalidate docker cache if any of the ignored files changes. I'm not sure what use case you are referring to
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#issuecomment-617983119
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**: [windows-gpu, website, edge, windows-cpu, centos-gpu, centos-cpu, unix-gpu, sanity, miscellaneous, unix-cpu, clang]
***
_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
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413626098
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
I think you are misunderstanding that answer. That statement is true if you'd use COPY for an entire folder. In that case, everything in that folder and below is part of the hash. But it does not mean that the entire context is used to calculate the hash.
You can validate that assumption by extending the docker cache tests. Introduce another test which adds or changes a random file in the context and make another rebuild. If the rebuild invalidated the cache although the files used by the add/copy command are unaltered, then you are right. Otherwise, my assumption is correct.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413422963
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
There may be different points of reference. When moving the `.dockeignore` to the `ci` folder, cache get's invalidated due to file changes (eg. inside `.git`).
Ie. once the dockerfile is moved, the following cache invalidation occurs
```
Step 5/15 : RUN cd /usr/local/src && git clone --recursive https://github.com/ccache/ccache.git && cd ccache && git checkout v3.7.9 && ./autogen.sh && ./configure --disable-man && make -j$(nproc) && make install && cd /usr/local/src && rm -rf ccache
---> Using cache
---> 63a51833e720
Step 6/15 : COPY install/requirements /work/
---> 95beeff2e471
```
In any case, I'll remove the `.dockerignore` file as we're getting rid of `COPY` for the time-intensive parts of the build. The addition of `.dockerignore` can be investigated in a separate PR
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413420016
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
No, CI scripts always assume the mentioned subfolder as context. Check the existing dockerfiles - the paths are not relative to root.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413517317
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
> Cache does not get invalidated because a file in the context changed.
This assumption is wrong. Take a look at https://stackoverflow.com/a/48553814/2560672
All non-ignored files are included in the computation of the hash used to lookup the cache, when using COPY. That's why avoiding COPY results in more cache hits.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413416455
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
Can you just switch the context to the folder you'd like to watch - docker/CI. It seems like you specified the root folder as context and thus it will always observe everything while it actually shouldn't.
I'm referring to the case where users actually want to add the entire mxnet folder to their dockerfile.
----------------------------------------------------------------
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
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18138: CI: Consolidate Ubuntu Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18138:
URL: https://github.com/apache/incubator-mxnet/pull/18138#discussion_r413417330
##########
File path: .dockerignore
##########
@@ -0,0 +1,4 @@
+# Exclude everything
+*
+# but the docker/ci folder
+!docker/ci/*
Review comment:
Ci scripts are run from the root folder. if we change the CI scripts, which we can do in the future, we can adapt the dockerignore? I don't mind removing dockerignore again from this PR given that there appear to be usecases negatively impacted by the addition?
----------------------------------------------------------------
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