Repository navigation
Compile time of erfa.pyx is very large #3134
Description
Sorry I didn't have enough (mental) bandwidth to follow this thread as it was happening, but it seems the new generated ERFA wrappers pose a surprisingly large burden on the compiler. On a 2.8GHz machine with 16GB of RAM, running Fedora 21 and gcc 4.9, it takes 23 264 seconds.
I may try look into chunking this up into smaller compilation units.
Activity
embray commentedon Nov 24, 2014
I've been noticing this too. Would we want to break it into multiple pyx sources? Or is it possible to tell Cython to produce multiple C sources?
taldcroft commentedon Nov 24, 2014
👍 This has been bugging me too.
astrofrog commentedon Nov 24, 2014
Will breaking it up actually speed it up overall though? Or will the cumulative time actually be the same if not larger than the current time?
astrofrog commentedon Nov 24, 2014
@embray - I think that the reason we kept it in one
pyxfile was because we figured that with multiplepyxfiles you may have the same overhead multiple times so end up with a larger overall compile time. That reasoning may be wrong though.mdboom commentedon Nov 24, 2014
I've seen other cases where breaking files into multiple
.cfiles decreases overall time because many of the optimization algorithms in the compiler are greater than O(n). Can't guarantee it will help here, but it's the first thing I might try.astrofrog commentedon Nov 24, 2014
Woah, it is pretty damn slow, it takes 72 seconds to compile the C file with clang on MacOS X 10.8 :-/
mdboom commentedon Nov 24, 2014
Actually I misstated the timing, since I had a hot ccache. The actual time is 264 seconds.
astrofrog commentedon Nov 24, 2014
😲
mdboom commentedon Nov 24, 2014
In fairness, this is 11MB of generated C code ;)
astrofrog commentedon Nov 24, 2014
Just to throw an idea out there, we may want to see if it makes sense to wrap all erfa fucntions - some of them may be mainly useful internally by other erfa functions, and it may be that only a subset are actually useful for outside codes? We could consider whitelisting a subset of 'useful' functions?
mhvk commentedon Nov 24, 2014
Yes, noticed the very slow time too, but was especially bugged by the memory use (
ccwas listed as using >1 GB, which ground things pretty much to a halt on my poor laptop, which has only 2 GB of memory..).astrofrog commentedon Nov 24, 2014
(just going to cc @eteq and @jwoillez into this conversation)
eteq commentedon Nov 24, 2014
Huh, weird, when I had been doing testing I was seeing more like 60 sec on clang, and maybe 2x that on gcc, so I wasn't too worried... But maybe there was some caching that I had missed? Anyway, I agree we should fix this if it's at all possible.
It's certainly true that we are not using all of the ERFA functions. So the best solution may be to do both of the suggestions here: that is, split the autogenerated erfa.pyx into a different
.pyxfor each function, and then "opt-in" only those that we need.Splitting the autogeneration into individual files shouldn't be that much work - should I give it a try and see if it speeds up compilation?
(P.S.: even this may be "delaying the inevitable" if we decide to eventually make our erfa bindings public, though. If we do that, we of course have to have it all in. But we can deal with that later if necessary I suppose.)
14 remaining items
astrofrog commentedon Nov 25, 2014
@jwoillez - that's certainly an option (so we'd basically put the bare minimum in cython if I understand you correctly). Would it be easy to try this to see how the compilation time changes?
jwoillez commentedon Nov 25, 2014
Here you go. Now, it seems that compiling
erfaitself takes more time than its wrapper.eteq commentedon Nov 25, 2014
@mhvk - note that right now, we are not planning to provide any of erfa as part of v1.0's public API. The thought was that down the road we might, but that isn't finalized.
One option to consider for a future version is instead figuring out a way to make the generator the part of the public API that other packages use. That is, the package in question specifies which erfa functions it needs, and
astropy(orastropy_helpers) provides the machinery to compile them. Not ideal, because it's an extra step for the user, but it might be an option to consider if we really aren't willing to compile it all as part ofastropy.As for the name
astropy.erfa, the reasoning there is that we might in the future provide that, and it is needed in a variety of other packages. I suppose it could go inutilsinstead if we don't want it top-level... I vaguely remember that being the original plan and then someone saying we should change it? Anyway, I have no strong opinion on top-level vs.utils.eteq commentedon Nov 25, 2014
@mdboom and @jwoillez - the approaches you're both looking at will leave the API the same, right? So I can push forward on
coordinateswork without fear that theerfainterface will change somehow from the approaches either/both of you are working on?Also, if it's actually the case that compiling
erfaitself is the biggest problem, there's probably no solution other than to limit the functions we compile to only those that we need, right? I'm fine with that as a possible stopgap for v1.0, as long as it's reasonably easy to do (i.e., just some list of function names insetup_package.py).scottransom commentedon Nov 26, 2014
Hi all... not that my opinion matters much, but two things... 1) I really like the idea that all of erfa is exposed publicly in astropy. I think that could benefit a lot of people (and it definitely would for me). 2) Why is the compilation time such an issue? A large fraction of people would not even touch that "problem" (given packages), a quite small fraction (like me) would only run into it occasionally during updates, and only those messing with the interface at all would see a real annoyance.
astrofrog commentedon Nov 26, 2014
@scottransom - the compilation time at the moment is really huge - >200s for a single file is a pain if you happen to
git cleanthe repo. But in any case it seems like we don't need all the code in Cython at the moment to actually be in Cython, so from @jwoillez's experiments with that it seems we can reduce the compile time a lot but still exposing everything (see #3141)scottransom commentedon Nov 26, 2014
@astrofrog - yeah, when I first installed the bindings I thought something was hung on my system -- I think it was longer than 200s on my laptop! But I still don't see that as a deal breaker -- there are a couple huge C++ codebases I work with that take much longer than that to compile. It is annoying, though, for sure. Sounds great about the experiments to decrease the times, though, as I really like the idea of the two new excellent astro libraries being available together (astropy and SOFA/ERFA) for python.
taldcroft commentedon Nov 26, 2014
@scottransom - the immediate issue for developers (at least me) is when jumping around in branches and python versions the compile time matters. Last week I was working on a problem that was different between py2 and py3 and I ended up spending a lot of time waiting for compilation (which I think roughly doubled for me as a result of the new ERFA).
scottransom commentedon Nov 26, 2014
@taldcroft Wouldn't that problem be "fixed" by having a 2nd clone of the astropy repo and having one in py2 and the other in py3? Anyway, I definitely understand that it is frustrating for you guys and since you are the ones doing all the hard work, I'll shut up now. (And serious thanks for the fantastic work, BTW, to all of you... great stuff)
mhvk commentedon Nov 27, 2014
@eteq - on the location, I'm all for
astropy.erfaif we provide all oferfa, otherwise I think it is better underutil. But I hope the slow compilation is indeed being solved -- all oferfais definitely better but the compilation time is just as definitely a real pain!astrofrog commentedon Nov 27, 2014
@mhvk - @jwoillez already has a nice solution to the speed issue in #3141. With those changes the wrapper takes only a few seconds to compile.
mhvk commentedon Nov 27, 2014
Fantastic! I had noticed the PR but didn't know it was that fast!
mhvk commentedon Nov 28, 2014
Closed by #3141
mdboom commentedon Dec 1, 2014
To answer @eteq's question (which is mostly obsolete now days later): I don't intend to change the ERFA API by my Cython-ridding patch.