-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Upgrade Travis to Xenial and floating tf-nightly #3070
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
Conversation
|
We usually keep this floating, but have pinned it (in #2947) due to |
|
@wchargin Thanks for the background info. I can see the exact double-free and size-corruption errors in the test logs of this PR: https://api.travis-ci.org/v3/job/628578238/log.txt I've pinged the owner of the tensorflow issue again. This needs to be fixed soon, or it'll block the debugger work and the next release. |
|
@mihaimaruseac Trying out |
|
For motivation of the upgrade to xenial, see tensorflow/tensorflow#34427 (comment) |
|
@mihaimaruseac It worked! Thanks!! |
wchargin
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.
Interesting. The last times that we tried to upgrade to Xenial (#2293,
#2654), we gave up because we were hitting Heisenbugs that we couldn’t
reproduce locally. The test experiencing those Heisenbugs is currently
disabled due to #2987, which isn’t obviously related to the Xenial-only
errors (SIGSEGVs vs. non-responsive HTTP routes). So I suppose that this
patch is fine with me because it’s not a regression; it merely changes
the root cause of the already-broken test.
|
(Also, the platform-specific segfaults aren’t exactly encouraging… it’s |
|
@wchargin I'm not sure I agree with #3070 (comment) , considering the fact that the official System Requirements doc states that Ubuntu 16.04+ is required: https://www.tensorflow.org/install/pip cc @mihaimaruseac for context and any opinions on whether that related tensorflow issue should be closed or not. |
|
Hmm, fair. I guess it could be a legitimate fix for an ABI change, then. |
|
We switched to being maylinux2010 compliant and that required a modern toolchain which is not provided by Ubuntu14. Furthermore, Ubuntu14 eol'ed this summer, so we can no longer support it as no patches would come to it and the toolchains. |
|
If Ubuntu16 fixes this, it could be that the change that exposed the heisenbug causing the segfaults to show up is the commit cherry-picked by tensorflow/tensorflow#34764 Unfortunately, we cannot roll that back as it would break all manylinux2010 builds. So I think it's better to close the TF issue if it's caused by old toolchain |
|
@mihaimaruseac @wchargin Thanks for the info. Given the documented system requirements of tensorflow, I think it's better to run tensorboard's CI on xenial. So I'm merging the PR now, there is another PR (#3051) of which the CI won't pass without this. @nickfelt If you have further comments, please let me know and I'll be happy to follow up. |
Motivation for features / changes
Technical description of changes