-
Notifications
You must be signed in to change notification settings - Fork 213
Use gcc 5.4 on xenial non-conda + use pytorch 0.4 official + fix broken python API #358
Conversation
ftynse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add an empty line in your commit message between "Fix docker image to use gcc 5.4 on xenial non-conda env" and the rest? In git, the first "paragraph" (before the empty line) is the short summary and the rest is the longer commit message that are treated differently. We generally prefer short summaries to fit into 80 characters.
| apt-get install -y --no-install-recommends libcilkrts5 gcc-$GCC_VERSION g++-$GCC_VERSION | ||
| update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-$GCC_VERSION 50 | ||
| update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-$GCC_VERSION 50 | ||
| if [[ "$GCC_VERSION" == 4.9 ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SO this installs from PPA when asked for GCC 4.9, but what is the version shipped with the system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system version 4.8.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then I don't really understand how is this supposed to make it use gcc-5.4 as the commit message says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the trusty distrib, we use gcc4.9 but default shipped is 4.8.4
for xenial, we use gcc 5.4 but default shipped by ubuntu toolchain is 5.5 as of April 24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so how do we get gcc 5.4 then? we only install from ppa if 4.9 is requested. so when 5.4 is requested -- nothing happens. I assume than the default 5.5 is used, which sort of contradicts your commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if gcc version 4.9 is requested, we get it from ppa. otherwise, don't use the toolchain and apt install it. Earlier, gcc5 was coming from ppa as well which used to be gcc5.4 before April 24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the part that is missing from the description is that xenial has 5.4 by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. I understand now what you mean. I'll edit the description. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! sorry it took so long
|
On Thu, Apr 26, 2018 at 03:13:33PM +0000, ftynse wrote:
ftynse commented on this pull request.
Could you please add an empty line in your commit message between "Fix docker image to use gcc 5.4 on xenial non-conda env" and the rest? In git, the first "paragraph" (before the empty line) is the short summary and the rest is the longer commit message that are treated differently. We generally prefer short summaries to fit into 80 characters.
Not just the one-line summary, but the entire commit message
should fit in 80 characters.
See
https://github.com/facebookresearch/TensorComprehensions/blob/master/CodingConventions.md#commits
skimo
|
95fb299 to
a9da0da
Compare
…on tests fix the aten build which fails with avx related issues on gcc5.5. Ubuntu toolchain ships gcc 5.5 by default since April 24, 2018 hence not using the toolchain for gcc 5.4 but rather what ubuntu provides. Also use the official pytorch 0.4 version and fix python tests which were broken by a recent change in PR 353
a9da0da to
8ccf885
Compare
|
thanks @ftynse for review, I just edited the description and also fixed the python API broken by a recent change. this PR should be good to go now and ready for review again :) |
|
merging this to help facilitate fixing the Jenkins CI @mingzhe09088 can you re-try the python tests again with this PR? I noticed the similar issue as you reported. It was broken in a recent change in PR 353. |
Fix docker image to use gcc 5.4 on xenial non-conda env to fix aten build which fails with avx related issues on gcc5.5. Ubuntu toolchain ships gcc 5.5 be default since April 24, 2018 hence not using the toolchain for gcc 5.4
issue filed on pytorch repo pytorch/pytorch#6992
i'll propagate this to gcc as well
partially helps #338