-
Notifications
You must be signed in to change notification settings - Fork 13
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 netcdf rather than scipy as to_netcdf engine #235
Conversation
@spencerkclark does this look good? (assuming Appveyor passes; currently running. Travis has passed.) I'll flesh out the What's New and merge once we're green |
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.
@spencerahill thanks for putting this together! I'm not sure what is going on in the Appveyor Python 3.4 environment, but maybe removing the line I suggest will fix things. It looks like it's installing some pretty old versions of most of the libraries (possibly because of that added requirement).
ci/environment-py34.yml
Outdated
@@ -3,7 +3,6 @@ channels: | |||
- conda-forge | |||
dependencies: | |||
- python=3.4 | |||
- scipy=0.18 | |||
- mkl=2017.0.3 |
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.
Do you think we can remove this now too?
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.
Good call, let's see what that does.
(FYI I might have to run for the night before this finishes. If so I'll return to it tomorrow.)
Huh, it seems like scipy gets installed anyway in the rest of the environments, so maybe we should just keep them the way they were? I'm assuming things will still pass in that case. Note also this error in our xarray-dev build: |
Ah, so I suspect what's going on is: in our other environments, xarray is installed via conda, and it's conda recipe includes scipy. But in this case, that doesn't happen. This was a good catch -- if we're actually going to switch from scipy to netCDF, we'll have to do so in our Ultimately it's very hard to imagine a user not having scipy, so I don't mind retaining it as a dependency after all. So I just pushed a commit reverting to including it, while still keeping the As for the AppVeyor 3.4 failures, I'm not sure why there would be the day vs. nanosecond mismatch. I'm hoping they'll go away with this commit. |
Blarg. Still one AppVeyor failure, strangely in the 3.5 environment and only on the "branch" rather than "pr" tests. (Since the aospy repo is under my profile, whenever I submit a PR, the CI gets triggered twice: once for the new commits on a branch in my repo, and once for a PR. When others submit a PR, only the PR tests run). Log for that failure is here: https://ci.appveyor.com/project/spencerahill/aospy/build/1.0.390/job/n8yuaforvylotr9q Will have to leave it at that for now; have to run for the day. But we're at least getting close. |
One last thing: closing and reopening to retrigger the tests, to see if that was a one-off failure. |
|
@spencerkclark thanks for the assist Very odd. Two failures in a row on the AppVeyor "branch" tests, but not on the "pr" tests, and only in 3.5 in the first case and only in 3.4 in the second case. Although the error is the same in both cases (here and here), on Given the inconsistency, I'm going to go ahead and merge this, in hopes that this was a one-off. But I'm not entirely convinced that this isn't pointing to some occasionally fragile logic on our side. So we'll keep monitoring for similar problems. |
Oy vey, now both 3.4 and 3.5 are failing. Will need to come back around to this, have to turn to other things for now. |
Closes problem described in #232 (comment) and more properly diagnosed in remainder of that thread.