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

Use external ci-helpers #392

Merged
merged 26 commits into from Jan 5, 2016
Merged

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Nov 30, 2015

Unfortunately I couldn't manage to pass the sherpa build to ci-helpers as it always ended in conflicts just as @cdeil predicted.
As a working solution it gets installed after the ci-helpers script finishes, directly from .travis.yml.

It still has issues with the older numpy-s, I'll come back to it.

This PR also updates to use np 1.10 and astropy 1.1, and adds extra tests for np 1.9 and astropy lts.

Closes #391

@cdeil cdeil added this to the whishlist milestone Nov 30, 2015
@cdeil cdeil self-assigned this Nov 30, 2015
@cdeil
Copy link
Contributor

cdeil commented Nov 30, 2015

@bsipocz
Copy link
Member Author

bsipocz commented Nov 30, 2015

It's just a side effect of the package version conflicts I've mentioned
before. I'll come back to this later this week.
On Nov 30, 2015 6:55 AM, "Christoph Deil" notifications@github.com wrote:

@bsipocz https://github.com/bsipocz - Thank you!

There's two builds where Cython is missing:


Reply to this email directly or view it on GitHub
#392 (comment).

@cdeil
Copy link
Contributor

cdeil commented Dec 10, 2015

Now we're getting this error in Gammapy on a different PR (#396):

ValueError: numpy.dtype has the wrong size, try recompiling

https://travis-ci.org/gammapy/gammapy/jobs/96029239#L1210

@mwcraig - Did something change in conda or the Astropy channel builds?
@bsipocz - Is this something you can / want to address in this PR, or should I start a separate one?

@bsipocz
Copy link
Member Author

bsipocz commented Dec 10, 2015

I can look at it tomorrow, and will try to fix it here. I'll let you know
if I hopelessly stuck somewhere
On Dec 10, 2015 5:17 PM, "Christoph Deil" notifications@github.com wrote:

Now we're getting this error in Gammapy on a different PR (#396
#396):

ValueError: numpy.dtype has the wrong size, try recompiling

https://travis-ci.org/gammapy/gammapy/jobs/96029239#L1210

@mwcraig https://github.com/mwcraig - Did something change in conda or
the Astropy channel builds?
@bsipocz https://github.com/bsipocz - Is this something you can / want
to address in this PR, or should I start a separate one?


Reply to this email directly or view it on GitHub
#392 (comment).

@cdeil cdeil mentioned this pull request Dec 10, 2015
4 tasks
@mwcraig
Copy link
Contributor

mwcraig commented Dec 10, 2015

Took a quick look (it was that or grade :)). One of your requirements is pulling in numpy 1.8 (sherpa), others are using 1.9 and others 1.10. conda does not update already-installed packages if you change the version of numpy installed.

The easiest way to ensure the version you want is to include numpy=$YOUR_NUMPY_VERSIN_HERE in all of the conda installs that you do.

@cdeil
Copy link
Contributor

cdeil commented Dec 10, 2015

@mwcraig - Thanks for having a look and advising!

I think we don't really need to pin the numpy version. Astropy and Sherpa should work with numpy 1.8 or 1.9 or 1.10, no? I think it's at least worth a try to "unpin" Numpy in some of the builds and see if they pass, i.e. the different Numpy versions are compatible enough for our needs.

@bsipocz - Let me know if you don't get to it tomorrow or get stuck. I have some time on Saturday and can try to help finish up this PR and fix the new numpy version issue.

@mwcraig
Copy link
Contributor

mwcraig commented Dec 11, 2015

@cdeil @bsipocz -- sorry, I think I wasn't very clear. Some of your dependencies already have their numpy dependencies pinned. For example, sherpa is built against numpy 1.8 (its build string is np17py27_0 (last digit may be one), so when sherpa is installed conda also installs the corresponding version of numpy.

The problem is that the first set of numpy dependent packages currently being installed is compiled against numpy 1.9.

The next set probably just asks for the most recent numpy (or maybe astropy) and gets packages built agains numpy 1.10 and numpy 1.10 instead of 1.9.

Finally, sherpa gets installed, and changes numpy to 1.8.

So in the end you have a mix of binary packages, some built against numpy 1.9, some against 1.10, and one against 1.8, and the final version of numpy installed is 1.8.

You might be able to fix this just by reinstalling numpy 1.10 at the very end since both the numpy ABI and API are supposed to be forward-compatible.

The alternative is to pin numpy everywhere to make sure everything gets installed with the same numpy.

@cdeil
Copy link
Contributor

cdeil commented Dec 29, 2015

@bsipocz - Currently this PR fails on travis-ci like this:
https://travis-ci.org/gammapy/gammapy/jobs/96866514#L1430

I can't reproduce the issue locally, but I think it's something that was fixed in scipy.interpolate.RegularGridInterpolator in scipy 0.15 (scipy/scipy#3707) and that build is using scipy 0.14, and 0.14 is what we currently list for Gammapy in setup.py:
https://github.com/gammapy/gammapy/blob/master/setup.py#L154

@bsipocz - Could you please bump the scipy version number required and used on travis-ci to 0.15 and see if that resolves the issue?

@bsipocz
Copy link
Member Author

bsipocz commented Jan 2, 2016

@cdeil - There is no scipy >=0.15 build available in conda with np1.8. Would you prefer to have a build against np1.8 with
a) only the core dependencies (and sherpa) installed,
b) or pip installing scipy,
c) or installing scipy 0.15/0.16 it from another conda channel,
d) or maybe another hack?

Option a) seems to be good enough, but I don't know much about gammapy, so I may miss something.

@cdeil
Copy link
Contributor

cdeil commented Jan 2, 2016

I'm not sure what your options imply for our test coverage, i.e. what the final build matrix would look like.
Maybe go with option a) and once all tests pass I'll have a look at the various builds and think about what's covered and what isn't (further additions probably left to future PRs).

The most important build for us is one that does include scipy and sherpa, because we use scipy for interpolation and sherpa for fitting in our main analysis routines. If none of the builds contains both scipy and sherpa, that's not good. (we had this before, so it should be possible, even if tedious with having to pin numpy version or run install commands in the right order).

@bsipocz
Copy link
Member Author

bsipocz commented Jan 2, 2016

OK, I'll rebase then to get rid of the noise commit here, and put back the full matrix.

@bsipocz bsipocz mentioned this pull request Jan 2, 2016
@bsipocz
Copy link
Member Author

bsipocz commented Jan 3, 2016

@cdeil - I've disabled enable_deprecations_as_exceptions() as an exception was raised in sherpa that I couldn't get rid of. It can be turned back on once the astropy issue is solved astropy/astropy#4430

@bsipocz bsipocz changed the title WIP Use external ci-helpers Use external ci-helpers Jan 3, 2016
@bsipocz
Copy link
Member Author

bsipocz commented Jan 3, 2016

@cdeil - This is ready for review.

@@ -7,6 +7,8 @@
from astropy.coordinates import Angle
from astropy.io import fits
from astropy.wcs import WCS
# Remove this when/if https://github.com/astropy/astropy/issues/4429 is fixed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add TODO: here so that we can find this later and clean it up.

@cdeil
Copy link
Contributor

cdeil commented Jan 5, 2016

@bsipocz - Thank you! Happy new year!

I mentioned two minor things inline. Otherwise this looks good to merge to me!

There's no reason to keep the bundled .continuous-integration version in master, right?
Can you remove it here or should I do this after merging in master?

@bsipocz
Copy link
Member Author

bsipocz commented Jan 5, 2016

@cdeil - Happy new year to you, too!

I've added the TODOs and removed the ci stuff (the curse of the hidden directories, I didn't notice it).

cdeil added a commit that referenced this pull request Jan 5, 2016
@cdeil cdeil merged commit 9aecf5d into gammapy:master Jan 5, 2016
@cdeil
Copy link
Contributor

cdeil commented Jan 5, 2016

@bsipocz - Thank you!

I know that figuring out build / test issues as you did in this PR is difficult and time consuming.
I've added you to the Gammapy contributor list in @6a44079.

@cdeil cdeil modified the milestones: 0.4, whishlist Jan 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants