Skip to content
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

CI: bump gcc from 4.8 to 5.x #13347

Closed
wants to merge 4 commits into from
Closed

CI: bump gcc from 4.8 to 5.x #13347

wants to merge 4 commits into from

Conversation

mckib2
Copy link
Contributor

@mckib2 mckib2 commented Jan 5, 2021

Reference issue

What does this implement/fix?

  • Azure CI wheel_optimized_gcc48 job now uses GCC 5 instead of GCC 4.8

Additional information

@mckib2
Copy link
Contributor Author

mckib2 commented Jan 5, 2021

@rgommers Are we attached to the idea of gcc-5.2 or could it be gcc-5.5? From what I can tell the apt repo only has gcc-5.5, so unless we want to build our compiler from source that's what we might have to use

@rgommers
Copy link
Member

rgommers commented Jan 5, 2021

Don't build a compiler from source I think - that usually takes forever and seems like a waste.

We need to dig up the details of what sets the lower limit we need to support. I suspect 5.5 could be fine. Or maybe my memory is deceiving me, PEP 599 for manylinux2014 still mentions GCC 4.8 - I'm really confused about where I got this idea now. manylinux2014 is based on CentOS 7 which still has gcc 4.8 as default. Need to do some digging in other issues.

@rgommers
Copy link
Member

rgommers commented Jan 5, 2021

This comment says manylinux2010 is already using a more recent GCC in the official Docker image. Maybe the PEP and the implementation diverged? @mattip @matthew-brett does one of you happen to know what's up with GCC versions here?

@rgommers rgommers added the Build issues Issues with building from source, including different choices of architecture, compilers and OS label Jan 5, 2021
@rgommers
Copy link
Member

rgommers commented Jan 5, 2021

Here's another confirmation that manylinux2010 already moved to GCC 7: #11421 (comment). It looks like the PEP is wrong and needs fixing.

I can't think about other obvious constraints to anything below GCC 7 right now, so I'd expect 5.5 to be fine.

@h-vetinari
Copy link
Member

@rgommers: I can't think about other obvious constraints to anything below GCC 7 right now, so I'd expect 5.5 to be fine.

Why not move to the maximum that all conceivable constraints allow? Assuming you haven't overlooked anything, what benefit would there be for using any version below GCC 7?

@mckib2
Copy link
Contributor Author

mckib2 commented Jan 5, 2021

So we don't want to needlessly break anyone's build, but if manylinux2010 is already on GCC7, then the world is already going to break for anyone this would concern, so I tend to agree with @h-vetinari that we should bump it from 4.8 -> 7. We still do have some builds that use GCC5, e.g. the docker build, so we might also bump those alongside

Then again, it costs us nothing to keep support for GCC5.5, so I could go either way

@rgommers
Copy link
Member

rgommers commented Jan 5, 2021

Keep in mind that we do have people building from source, and packagers for Linux distros and the like. For example, Ubuntu 16.04 LTS (Xenial) only reaches end of standard support in April 2021, and end of life in 2024 (https://wiki.ubuntu.com/Releases). Its default GCC is 5.3.1 (https://packages.ubuntu.com/search?suite=xenial&keywords=gcc)

And from memory, on AIX GCC 6 is commonly used.

We can use newer versions for wheels, but doing the minimum reasonable bump would be good. I don't necessarily care about every single use case (it's always a trade-off versus effort we need to put in), but bumping to GCC 7 is definitely going to make some people's life harder. So bumping to 5.5 as the next lowest version available in CI seems good.

@mckib2 mckib2 changed the title CI: bump gcc from 4.8 to 5.2 CI: bump gcc from 4.8 to 5.x Jan 6, 2021
@mckib2
Copy link
Contributor Author

mckib2 commented Jan 6, 2021

Besides the unfortunately named branch, everything changed to use gcc5. You've won me over, I think it's a good idea to keep as much backwards compatibility as possible while generally sticking to the roadmap

@rgommers
Copy link
Member

rgommers commented Jan 6, 2021

@mckib2 can you mention this on the scipy-dev mailing list? That's what we normally do with both new features and changes like this - it gets a lot more eyes on the change to ensure it's fine.

@h-vetinari
Copy link
Member

Playing a bit of devil's advocate here...

Keep in mind that we do have people building from source, and packagers for Linux distros and the like. For example, Ubuntu 16.04 LTS (Xenial) only reaches end of standard support in April 2021, and end of life in 2024 (https://wiki.ubuntu.com/Releases). Its default GCC is 5.3.1 (https://packages.ubuntu.com/search?suite=xenial&keywords=gcc)

Aside from being on a nearly 5 year old OS (with two LTS since), numpy is providing compatible wheels for that platform, and if someone really wants to build from source, they can easily install a newer version from an official ubuntu PPA and run update-alternatives.

And from memory, on AIX GCC 6 is commonly used.

I had no idea about this, but had a look. The GCC website points to atos, where there are gcc 9 builds available for everything including AIX 6.1 (already unsupported by IBM). As far as I can tell from these old docs (though still top hit on search engine for "aix gcc"), there's no default gcc version, and users always have to install the binaries anyway. From that POV, having to install a newer version of the compiler to build a newer numpy is hardly extraordinary, IMO.

We can use newer versions for wheels, but doing the minimum reasonable bump would be good. I don't necessarily care about every single use case (it's always a trade-off versus effort we need to put in), but bumping to GCC 7 is definitely going to make some people's life harder. So bumping to 5.5 as the next lowest version available in CI seems good.

I realize the reality of software dependencies and how there can be myriad constraints for someone to change their OS version, compiler version, etc. I'm thinking that it might be worth to formulate a more precise policy regarding compiler support (similar to NEP 29)? Because now it's really hard to tell what reasonable constraints even are, much less if the scenarios we're ready to jump through hoops for actually affect any users.

@ilayn
Copy link
Member

ilayn commented Jan 9, 2021

Because now it's really hard to tell what reasonable constraints even are, much less if the scenarios we're ready to jump through hoops for actually affect any users.

yes indeed this is basically crux of our (possibly over-)cautious stance.

@rgommers
Copy link
Member

rgommers commented Jan 9, 2021

I'm thinking that it might be worth to formulate a more precise policy regarding compiler support (similar to NEP 29)? Because now it's really hard to tell what reasonable constraints even are, much less if the scenarios we're ready to jump through hoops for actually affect any users.

I don't think it's worth the effort to do and to maintain. It's very rare we have to bump minimum compiler versions; it's been GCC 4.8 for quite a few years now. And I really don't want to spend time to think about what bumps we could get away with for more exotic constraints and compilers like XLC.

@h-vetinari
Copy link
Member

I don't think it's worth the effort to do and to maintain.

I'd say the toolchain document already does that, albeit at a more superficial level

It's very rare we have to bump minimum compiler versions; it's been GCC 4.8 for quite a few years now. And I really don't want to spend time to think about what bumps we could get away with for more exotic constraints and compilers like XLC.

I think bumping compiler versions should be part of the normal "version hygiene" as for everything else - in fact, I'd say not having bumped from 4.8 for years would IMO be an argument to upgrade, because there are maintenance and possibly performance burdens for staying on old compiler versions, and those should be weighed against the benefits of those exotic constraints.

I've been wanting to do a sequel to #13083 (not least since I followed up regarding https://bugs.python.org/issue42380 and have some pertinent things there), and I'd be willing to write up the status of those constraints, but I'd need a starting set. IMO, without making those things transparent, it's really hard to make a reasonable trade-off (and erring too cautiously has hidden costs too).

Rather than the hypothetical "bumping the compiler might break exotic use cases A/B/C", I think an argument can be made that it would be preferable to actually do a more aggressive bump for (e.g. 1.7.0), and invite affected users (if they even exist not just hypothetically!) to open an issue. Then it could still be reverted/adapted in a point release.

@ev-br
Copy link
Member

ev-br commented Jan 9, 2021

actually do a more aggressive bump for (e.g. 1.7.0), and invite affected users (if they even exist not just hypothetically!) to open an issue. Then it could still be reverted/adapted in a point release.

Which is exactly the churn and extra work Ralf looks to avoid :-).

FWIW, my +1 is to a "minimum work" path advocated by Ralf.

@mattip
Copy link
Contributor

mattip commented Jan 9, 2021

numpy is providing compatible wheels for that platform

The upcoming 1.20 release (by Feb 2021) will be the last to offer manylinux1 wheels. In the manylinux1 image the build tools come from devvtoolset-2, which provides gcc4.8. In manylinux2010 the build tools come from devtoolset-8 which I believe provides gcc8.3.

As stated above, the default compiler on CentOS 7 is gcc4.8, and CentoOS 7 will EOL only in June 2024.

So if possible I guess it would be nice to keep support for 4.8 around in order to support building on that platform, or at least be responsive to issues people submit about failures once you update to a newer version.

@h-vetinari
Copy link
Member

h-vetinari commented Jan 9, 2021

@ev-br: Which is exactly the churn and extra work Ralf looks to avoid :-).

Sure, there's no free lunch :)

But that churn is just the insurance policy if indeed someone would complain; if the bump is chosen well, then that churn hopefully isn't necessary at all, but the other incremental benefits are still unlocked.

@mattip: As stated above, the default compiler on CentOS 7 is gcc4.8, and CentoOS 7 will EOL only in June 2024.

That is well-understood, I'm just curious how large the group of users is that simultaneously:

  • are on CentOS 7 (or similarly old linux) and cannot upgrade
  • cannot upgrade their compilers (which is as simple as sudo yum install centos-release-scl; sudo yum install devtoolset-7 on CentOS)
  • cannot use conda
  • absolutely require the newest numpy

That being said, I'll drop this line of argument, since there seems to be clear consensus against my proposition.

@rgommers
Copy link
Member

rgommers commented Jan 9, 2021

As stated above, the default compiler on CentOS 7 is gcc4.8, and CentoOS 7 will EOL only in June 2024.

So if possible I guess it would be nice to keep support for 4.8 around in order to support building on that platform, or at least be responsive to issues people submit about failures once you update to a newer version.

@mattip this is what I was asking about in #13347 (comment), it's exactly the conflict between what the manylinux2014 PEP says and what the Docker image toolchain contains. I don't think they're both equally relevant. I honestly don't care about CentOS 7 more than about any other individual distro. Unless there's an issue with compiling in the official manylinux2014 way using their Docker images.

The thing that's not spelled out anywhere but may be the implication is that we have to bundle libstdc++ which we may not be doing now (?). pypa/auditwheel#125 has some discussion.

@rgommers
Copy link
Member

Given that no one seems to know the exact answer to what happens with manylinux2014, we will need to test building an actual wheel in https://github.com/MacPython/scipy-wheels and seeing if it works in a CentOS 7 Docker container. And because the wheels repo vendors gfortran 4.9 which would need updating too, that's a little involved.

We should do that first before merging this PR.

@mattip
Copy link
Contributor

mattip commented Jan 11, 2021

The manylinux2010 wheel works for numpy, as they manylinux2010 wheels should. Why do you want to use the manylinux2014 on CentOS7 x86_64 and not manylinux2010?

Edit: phrasing

$ docker run -it centos:7
...
# cat /etc/redhat-release 
CentOS Linux release 7.9.2009 (Core)
# # build python3.7
# yum install gcc openssl-devel bzip2-devel libffi-devel zlib-devel wget make
# cd /usr/src
# wget https://www.python.org/ftp/python/3.7.9/Python-3.7.9.tgz
# tar xzf Python-3.7.9.tgz
# cd Python-3.7.9
# ./configure --enable-optimizations
# make altinstall
# cd
# rm /usr/src/Python-3.7.9.tgz
# python3.7 -mpip install numpy pytest hypothesis
Collecting numpy
  Downloading numpy-1.19.5-cp37-cp37m-manylinux2010_x86_64.whl (14.8 MB)
     |████████████████████████████████| 14.8 MB 5.1 MB/s 
...
# python3.7 -c "import numpy;numpy.test()"

@rgommers
Copy link
Member

The manylinux2010 wheel works for numpy, as they manylinux2010 wheels should. Why do you want to use the manylinux2014 on CentOS7 x86_64 and not manylinux2010?

NumPy doesn't have C++ code - the rationale for this PR is wanting to use C++14, which needs GCC 5.x as a compiler. NumPy doesn't have C++ code, so that it works for NumPy doesn't say much here.

Why would we go for manylinux2010 rather than manylinux2014?

@mattip
Copy link
Contributor

mattip commented Jan 11, 2021

Why would we go for manylinux2010 rather than manylinux2014?

Sorry, I was confused. 2010->CentOS6, 2014 -> Centos7.

@mckib2
Copy link
Contributor Author

mckib2 commented Jan 18, 2021

Not a lot movement on the mailing list about this and generally positive reception here. Is the blocker here doing the test build on https://github.com/MacPython/scipy-wheels to figure out if we need to add extra compiler/linker arguments?

@rgommers
Copy link
Member

Is the blocker here doing the test build on https://github.com/MacPython/scipy-wheels to figure out if we need to add extra compiler/linker arguments?

Yes indeed.

@mckib2
Copy link
Contributor Author

mckib2 commented Jan 18, 2021

Is the blocker here doing the test build on https://github.com/MacPython/scipy-wheels to figure out if we need to add extra compiler/linker arguments?

Yes indeed.

I'm not familiar with this process but I can take a look this evening at what the process is and see if I can stumble my way through it

EDIT: see MacPython/scipy-wheels#110; took a stab at just updating commit hashes, looks like more needs to be done to get it to checkout the right branch. Someone with commit rights to scipy-wheels may need to take it out for a spin to try out TravisCI and ADS

@tylerjereddy
Copy link
Contributor

@mckib2 I commented on your PR in the wheels repo with a few pointers/thoughts.

@tylerjereddy
Copy link
Contributor

I'm assuming you're going to make some actual changes in the wheels repo PR; for example, try bumping the gcc version or whatever over there while pointing to this branch. Then if it breaks, you'll try to fix the issues by making changes to this PR/feature branch and re-triggering the wheels repo builds of this feature branch.

@h-vetinari
Copy link
Member

h-vetinari commented Nov 16, 2021

@h-vetinari: I think bumping compiler versions should be part of the normal "version hygiene" as for everything else [...]

I continue to think that this should be done, and the sooner the better. While we should not senselessly break compatibility, I also think that there should be a limit to the contortions maintainers take upon themselves for supporting the mythical "long tail" of users, that may well actually be empty (or nearly so).

For someone to be impacted by this bump, they'd have to simultaneously be:

  • on a distribution that's more than 6 years old (GCC 5.1 was released April 2015)
  • unable to install newer compilers (which most distros support easily)
  • unable to use the compatible(!) wheels provided by scipy
  • unable to use conda (which even still supports CentOS 6!)
  • absolutely required to use the newest scipy

I believe the number of users that fall into all of these categories is most likely vanishingly small. And if someone truly is affected, they could still raise an issue, and would likely receive an open ear (and reverting to GCC 4.8 could be considered). But again, we might actually be talking about an empty set of users here, and IMO we should just bump GCC for scipy 1.8 and see if anything happens.

@rgommers
Copy link
Member

I continue to think that this should be done, and the sooner the better.

Yes, this should be fine now. IIRC this PR worked, but dropping gcc 4.8 on the corresponding scipy-wheels repo PR ran into some hiccups (don't remember why). Now that we've dropped manylinux1 completely, we can definitely move on that repo. It just needs doing.

@h-vetinari
Copy link
Member

h-vetinari commented Nov 17, 2021

I've opened a PR in the wheel repo, but I'm wondering now why the gfortran-vendor (purely on osx, where we're using clang 12) has something to do with bumping the minimum compiler version here? I mean, I get that gfortran also comes from GCC, but it seems that from the POV of scipy, gcc-version-for-linux and gfortran-version-for-osx are completely uncoupled anyway?

@rgommers
Copy link
Member

I mean, I get that gfortran also comes from GCC, but it seems that from the POV of scipy, gcc-version-for-linux and gfortran-version-for-osx are completely uncoupled anyway?

Those are uncoupled, yes. The macOS x86_64 one can be updated, should work fine. The minimum gcc+gfortran version on Linux is determined by old distros & manylinux standards, that's the externally limiting factor.

@h-vetinari
Copy link
Member

The minimum gcc+gfortran version on Linux is determined by old distros & manylinux standards, that's the externally limiting factor.

I've opened a minimal PR to do remove what's left of manylinux1 on the wheel repo: MacPython/scipy-wheels#141

I think this could/should be merged independently of the gfortran effort.

@h-vetinari
Copy link
Member

Now that MacPython/scipy-wheels#141 has been merged, we can proceed here.

@mckib2, are you willing to continue on this PR? If not, I can give it a shot as well.

@h-vetinari
Copy link
Member

@rgommers, I was just writing something about GCC for #15045 (in light of this PR / #15066), and was wondering about your statement:

Now that we've dropped manylinux1 completely, we can definitely move on that repo. It just needs doing.

I wanted to write something about how the GCC minimum is affected by the oldest-supported manylinux, but actually, manylinux2010 (aside from being EOL along with CentOS 6) comes with GCC 8.3 (through the RHEL dev toolset magic ABI-compatible backports), see this discussion.

I get that the appetite for needlessly bumping the GCC minimum is low, but we should IMO at least be able to document what the current policy is driven by. (Note that in that same discussion on the manylinux-tracker, we see that manylinux_2_24 only has GCC 6.3, because there are no gcc-backports for it; that might be actually be the next well-justifiable lower bound?)

@rgommers
Copy link
Member

I get that the appetite for needlessly bumping the GCC minimum is low, but we should IMO at least be able to document what the current policy is driven by. (Note that in that same discussion on the manylinux-tracker, we see that manylinux_2_24 only has GCC 6.3, because there are no gcc-backports for it; that might be actually be the next well-justifiable lower bound?)

Yes, GCC 6.3 is needed for manylinux_2_24. And also it's the default version on AIX 7 (latest open AIX build report on this issue tracker uses GCC 6.3.0). Ubuntu 18.04 LTS is at 7.3.0. So 6.3.0 seems fine, or whatever the version below that is which can be easily installed in CI.

@h-vetinari
Copy link
Member

So 6.3.0 seems fine, or whatever the version below that is which can be easily installed in CI.

I've tried going to gcc-6 in #15066, and the CI passes without issue. The version getting installed is actually 6.5.0 - I didn't pin down to the minor level, since @mckib2 noted in this PR (cf. 5d7945a) that older versions may be source-only; a quick search indicates that this is the same for gcc 6.

I don't think the minor version between 6.3 & 6.5 is such an issue here, and I could imagine declaring 6.3 as the lower bound but running CI with 6.5 (knowing that's not ideal, but also cognizant of how realistic breaking changes are between minor versions, and the effort to close that gap). WDYT @rgommers?

@rgommers
Copy link
Member

I don't think the minor version between 6.3 & 6.5 is such an issue here, and I could imagine declaring 6.3 as the lower bound but running CI with 6.5 (knowing that's not ideal, but also cognizant of how realistic breaking changes are between minor versions, and the effort to close that gap). WDYT @rgommers?

I looked at the list of fixes in GCC 6.4 and 6.5, and that seems okay to me.

@tylerjereddy tylerjereddy added this to the 1.8.0 milestone Nov 20, 2021
@mckib2 mckib2 deleted the gcc48to52 branch December 1, 2021 05:08
@mckib2
Copy link
Contributor Author

mckib2 commented Dec 1, 2021

Now that MacPython/scipy-wheels#141 has been merged, we can proceed here.

@mckib2, are you willing to continue on this PR? If not, I can give it a shot as well.

Sorry for late response, thanks for taking it up.

It occurs to me that we can try upgrading Boost now that we have a more recent minimum compiler that can handle more modern C++. That may help get upstream fixes for Boost.Math that we've been patching manually, might help with gh-14901 and friends

@rgommers
Copy link
Member

rgommers commented Dec 1, 2021

Ah that'd be great, if those overflows would disappear it would save some headaches on macOS.

@h-vetinari
Copy link
Member

Ah that'd be great, if those overflows would disappear it would save some headaches on macOS.

It'd be very easy... we'd just have to to raise the minimum compiler requirement to GCC 7.x 😉

In any case, I opened a separate issue to discuss this, because it's way too easy to lose sight of comments in closed & older PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build issues Issues with building from source, including different choices of architecture, compilers and OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants