[WIP] Changes to make tests pass when using conda forge channel #2440

djkirkham · 2017-03-16T14:24:37Z

This PR contains a set of changes to make sure the tests pass when installing the environment from the conda-forge channel, with a view to ultimately using that environment for the Travis automated tests.

Due to some issues with cartopy/matplotlib detailed in SciTools/cartopy#848, matplotlib is pinned to 1.*. (The package spec matplotlib<2 picks up 2.0.0rc2 which is inexplicably in the conda-forge channel, so I've pinned it with <1.9.)

In addition to some system and example tests which are still failing, some integration tests will fail using conda-forge until SciTools/iris-grib#75 is merged and a release has been made.

marqh · 2017-03-16T14:33:40Z

I think that there are a range of reasons why this may well be a sensible approach for us to take. I am keen to hear words of caution and tactical ways to protect ourselves were we to adopt this approach

djkirkham · 2017-03-20T17:08:41Z

imagehash changes: SciTools/test-iris-imagehash#11

marqh · 2017-03-21T10:25:31Z

i am in favour of these changes in principal. I haven't found the time to finish my detail review yet

i'd like to encourage comments for key stakeholders about the approach of moving CI testing to conda-forge, which is the purpose of this work

are there voices in favour?
voices with concern?
issues to address?

thoughts please

djkirkham · 2017-03-21T10:47:29Z

The main issue as I see it is that conda forge changes so quickly that we might struggle to keep up with the changes. Additionally, it's pretty gauling if tests start failing because something on the channel changed. This is still an issue with the scitools channel of course, but at least we're in control of that.

QuLogic · 2017-03-21T20:29:56Z

That seems like an advantage to me; having everything "work" because scitools is using a bunch of outdated packages is not really "working".

DPeterK · 2017-03-22T09:19:36Z

I agree with @djkirkham - we should not have to constantly be caught up to the latest everything. Doing so has the danger of introducing a very high workload to Iris devs that would come at the expense of the cessation of new features...

having everything "work" because scitools is using a bunch of outdated packages is not really "working"

Two things here:

if all the tests pass, then to the best of our knowledge everything is working. The definition of working is that things are not breaking all the time; it is not that we are using the latest everything.
some packages that are pinned a few point versions behind the latest release is not "outdated".

I agree that allowing ourselves to get far behind the latest version of packages is a very bad thing. It is similarly bad, however, to be constantly trying to chase the latest everything, and I'm concerned that conda-forge just moves a little too fast and too unpredictably to be really reliable to base Iris on -- at least, not without some well thought through pinning of some packages.

ajdawson · 2017-03-22T09:30:01Z

This is still an issue with the scitools channel of course, but at least we're in control of that.

You'd have quite a lot of control over the conda-forge channel too, don't forget that.

I think it is an advantage to keep up with the conda-forge channel, because that is the one most users are working with, and if iris works there then it is working for a lot of people.

djkirkham · 2017-03-22T14:16:30Z

The key issues seem to be these:

Using the latest conda-forge packages exposes us to test failures which may be out of our control and would slow the introduction of new features
Pinning the libraries or using the out-of-date packages on Scitools means that we're not supporting packages which are actually being used by users, and every so often there is a massive amount of work to bring things up-to-date

In an offline discussion earlier, the idea emerged to pin key libraries for the tests, but have a process in place to run the tests on the latest conda-forge libraries and notify us of any failures. That way new code could still easily be merged, and we'd be able to keep abreast of changes in conda-forge.

Thoughts?

marqh · 2017-03-22T15:43:56Z

@djkirkham a useful summary, thank you

My present view is that I/we can quickly 'patch' a test failure by pinning a dependency; this gives me/us time to implement an improvement and to then remove the pin

the idea emerged to pin key libraries for the tests, but have a process in place to run the tests on the latest conda-forge libraries and notify us of any failures.

I think this may be more conservative than is required. I would feel comfortable having the tests with no pins (or as few as possible) and to add a pin if a failure mode rears its head. Perhaps a developer rule that doing this should include raising a bug ticket and hoping to fix it in the future

Thus, I am advocating a reactive approach.
I think this could work for us

pp-mo · 2017-03-23T15:55:28Z

lib/iris/coords.py

-                    bounds = bounds[keys + (Ellipsis, )]
+                    # Bounds will generally have an extra dimension compared
+                    # to points, so add an Ellipsis at the end, unless there
+                    # is already on, as numpy does not support double Ellipsis.


Not quite clear: should that be "unless there already is one" ?

Yes, I have corrected it.

DPeterK

We need to fix the netcdftime screwup properly, not patch around it.

DPeterK · 2017-03-30T13:55:50Z

lib/iris/tests/test_pandas.py

-                          netcdftime.datetime(2000, 7, 21, 4, 48),
-                          netcdftime.datetime(2000, 11, 1, 7, 12),
-                          netcdftime.datetime(2001, 2, 11, 9, 36)]
+        if netCDF4.__version__ > '1.2.4':


DPeterK · 2017-03-30T13:57:18Z

lib/iris/tests/test_pandas.py

@@ -236,8 +244,12 @@ def test_time_360(self):
        time_coord = DimCoord([100.1, 200.2], long_name="time",
                              units=time_unit)
        cube.add_dim_coord(time_coord, 0)
-        expected_index = [netcdftime.datetime(2000, 4, 11, 2, 24),
-                          netcdftime.datetime(2000, 7, 21, 4, 48)]
+        if netCDF4.__version__ > '1.2.4':


Again 😱

Why do we not have a test that picks this up properly rather than having two completely unrelated tests that have two different codepaths based on the version of an unrelated package?

I feel pretty bad about this approach, but it represents another step in a long line of trying to work with netcdf4python

i think that this is the best of a bad bunch

there is also a particular issue with conda-forge builds of netcdf, not all versions are available as proper builds

further work is required on this, which needs to be planned
#2322

dkillick and I have agreed to push this out to another activity, not to have it as a blocker for this PR

bjlittle · 2017-03-30T14:53:14Z

lib/iris/tests/idiff.py

+                                                                e.message))
+                        continue
+                    else:
+                        raise


@djkirkham I think that this should be raise e to propagate the exception, surely?

No, raise on its own will keep the original stack trace. raise e would start a new stack trace from here. Minor detail, but it gives an extra bit of debugging information.

Yup, nice. But could you add a comment just to make that subtle point crystal clear to those that follow ... thanks!

bjlittle · 2017-03-30T14:53:53Z

lib/iris/tests/integration/plot/test_netcdftime.py

@@ -26,12 +26,15 @@
 # importing anything else
 import iris.tests as tests

+import numpy as np
+import netcdftime


@djkirkham Fix the sort order of the imports please

bjlittle · 2017-03-31T08:48:26Z

lib/iris/tests/unit/coord_categorisation/test_add_categorised_coord.py

@@ -99,7 +99,6 @@ def setUp(self):
            'proleptic_gregorian': np.array(list(range(360, 367)) +
                                            list(range(1, 4))),
            'noleap': np.array(list(range(359, 366)) + list(range(1, 4))),
-            'julian': np.array(list(range(360, 367)) + list(range(1, 4))),


@marqh ( @djkirkham ) Any idea why this has been removed?

Also, the from cf_units import CALENDARS as calendars is now no longer used.

There is also a disconnect between the calendars in calendars and the calendars that this test is covering ... might want to join that up somehow ...

@bjlittle This is because netcdftime julian datetime objects don't give the correct day of year in the latest version; they always give a value of 1.

In fact, some of the other calendar types have the same issue if they are constructed directly:

>>> import netcdftime >>> ut = netcdftime.utime('days since 1980-12-25', calendar='365_day') >>> date = ut.num2date(0) >>> date2 = netcdftime.DatetimeNoLeap(1980, 12, 25) >>> print repr(date) netcdftime._netcdftime.DatetimeNoLeap(1980, 12, 25, 0, 0, 0, 0, 2, 359) >>> print date.dayofyr 359 >>> print date.timetuple() (1980, 12, 25, 0, 0, 0, 2, 359, -1) >>> print repr(date2) netcdftime._netcdftime.DatetimeNoLeap(1980, 12, 25, 0, 0, 0, 0, -1, 1) >>> print date2.dayofyr 1 >>> print date2.timetuple() (1980, 12, 25, 0, 0, 0, -1, 1, -1)

All of our datetime objects are constructed using a utime object (via cf_units) so we avoid this issue with the other calendars.

I think it's probably worth just commenting out the offending line in our tests and having a comment explaining why. Either that, or pin netcdf4.

bjlittle · 2017-03-31T12:21:29Z

@djkirkham Where the heck is the travis-ci output ❓ ... 😕

cpelley · 2017-04-04T08:10:35Z

lib/iris/tests/unit/coord_categorisation/test_add_categorised_coord.py

@@ -116,6 +116,11 @@ def make_cube(self, calendar):

    def test_calendars(self):
        for calendar in calendars:
+            # Skip the Julian calendar due to
+            # https://github.com/Unidata/netcdftime/issues/13
+            # Remove this if block once the issue is resolved.


Is this issue present as a result in changing versions of netcdftime?

If so, would pinning the version of netcdftime be the preferred option until Unidata/cftime#13 is resolved?

A parallel in our own project is where we pin the numpy version to v1.11 until an iris issue is resolved:
#2425

QuLogic added the Status: Work in Progress label Mar 16, 2017

djkirkham mentioned this pull request Mar 16, 2017

adopt conda-forge #2441

Merged

marqh mentioned this pull request Mar 16, 2017

adopt conda-forge SciTools/cartopy#870

Merged

marqh assigned marqh and bjlittle Mar 16, 2017

djkirkham mentioned this pull request Mar 20, 2017

Iris conda forge SciTools/test-iris-imagehash#11

Merged

djkirkham force-pushed the totest branch from 4484c26 to 67eb24b Compare March 21, 2017 10:12

pp-mo reviewed Mar 23, 2017

View reviewed changes

DPeterK previously requested changes Mar 30, 2017

View reviewed changes

bjlittle reviewed Mar 30, 2017

View reviewed changes

bjlittle reviewed Mar 31, 2017

View reviewed changes

djkirkham force-pushed the totest branch from 71190aa to 45a9325 Compare March 31, 2017 11:34

Ensure tests work when installing from conda-forge channel

dee2d90

djkirkham force-pushed the totest branch from 45a9325 to dee2d90 Compare March 31, 2017 12:00

Update coords.py

5c97da0

marqh mentioned this pull request Apr 3, 2017

Ensure tests work when installing from conda-forge channel #2474

Closed

pp-mo merged commit f933382 into SciTools:master Apr 3, 2017

pp-mo removed the Status: Work in Progress label Apr 3, 2017

pp-mo pushed a commit that referenced this pull request Apr 3, 2017

Tests now all work when installing from conda-forge channel (#2440)

6973b56

cpelley reviewed Apr 4, 2017

View reviewed changes

marqh mentioned this pull request Apr 4, 2017

BUG: Iris coords bounds indexing broken with multiple ellipsis #2425

Closed

QuLogic added this to the v2.0 milestone Apr 4, 2017

QuLogic modified the milestones: v1.13.0, v2.0 May 17, 2017

bjlittle added a commit to bjlittle/iris that referenced this pull request Jun 5, 2017

Align TestCubeCollapsed CML based on SciTools#2440

bbf0941

bjlittle added a commit to bjlittle/iris that referenced this pull request Jun 5, 2017

Align TestAnalysisWeights CML based on SciTools#2440

821e325

djkirkham mentioned this pull request Oct 19, 2017

Iris compatibility with latest netcdf4 #2322

Closed

djkirkham deleted the totest branch October 26, 2017 13:02

ehogan mentioned this pull request Nov 3, 2017

Problem with netcdftime? #2907

Closed

rcomer mentioned this pull request May 5, 2022

Cell comparison: remove ancient netcdftime/cftime workarounds #4729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Changes to make tests pass when using conda forge channel #2440

[WIP] Changes to make tests pass when using conda forge channel #2440

djkirkham commented Mar 16, 2017

marqh commented Mar 16, 2017

djkirkham commented Mar 20, 2017

marqh commented Mar 21, 2017

djkirkham commented Mar 21, 2017

QuLogic commented Mar 21, 2017

DPeterK commented Mar 22, 2017

ajdawson commented Mar 22, 2017

djkirkham commented Mar 22, 2017

marqh commented Mar 22, 2017

pp-mo Mar 23, 2017

djkirkham Mar 23, 2017

DPeterK left a comment

DPeterK Mar 30, 2017

DPeterK Mar 30, 2017

marqh Mar 30, 2017 •

edited

Loading

bjlittle Mar 30, 2017

djkirkham Mar 30, 2017

bjlittle Mar 30, 2017

bjlittle Mar 30, 2017

bjlittle Mar 31, 2017

djkirkham Mar 31, 2017 •

edited

Loading

bjlittle commented Mar 31, 2017 •

edited

Loading

cpelley Apr 4, 2017

cpelley Apr 4, 2017

[WIP] Changes to make tests pass when using conda forge channel #2440

[WIP] Changes to make tests pass when using conda forge channel #2440

Conversation

djkirkham commented Mar 16, 2017

marqh commented Mar 16, 2017

djkirkham commented Mar 20, 2017

marqh commented Mar 21, 2017

djkirkham commented Mar 21, 2017

QuLogic commented Mar 21, 2017

DPeterK commented Mar 22, 2017

ajdawson commented Mar 22, 2017

djkirkham commented Mar 22, 2017

marqh commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DPeterK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marqh Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djkirkham Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

bjlittle commented Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marqh Mar 30, 2017 •

edited

Loading

djkirkham Mar 31, 2017 •

edited

Loading

bjlittle commented Mar 31, 2017 •

edited

Loading