Skip to content

Reinstate iris_grib. #2810

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 26, 2017
Merged

Reinstate iris_grib. #2810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7e5f085
Initial attempts to reinstate iris_grib.
pp-mo Oct 17, 2017
5860f7f
Slightly less horrible kludge in test_grib_load_translations.
pp-mo Oct 18, 2017
f48b51a
Added iris_grib to full conda-requirements.
pp-mo Oct 18, 2017
fc0c21f
Modified to test against temporary iris-grib branch.
pp-mo Oct 24, 2017
3ed870e
Test with iris-grib latest master.
pp-mo Oct 24, 2017
28828da
Tiny fix to odd test.
pp-mo Oct 25, 2017
3df9b87
Fix pep8 checks for removed sourcefiles.
pp-mo Oct 25, 2017
fd36e9d
Adopted various suggestions from pelson's #2617
pp-mo Oct 25, 2017
7d0cced
Use deferred import of iris_grib for grib loading, consistent with sa…
pp-mo Oct 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ install:
fi
fi

# JUST FOR NOW : Install latest master version of iris-grib.
Copy link
Contributor

@djkirkham djkirkham Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't we going to have to do this every time we change the Iris API in a way that breaks the currently released iris-grib? Additionally, when that happens we need to make sure we fix iris-grib at the same time to keep the Iris tests passing. I can't help thinking we need to break the circular dependency. That would mean no Iris tests which use iris-grib

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively: Make Iris-grib no longer depend on Iris somehow.

Copy link
Member Author

@pp-mo pp-mo Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

every time we change the Iris API in a way that breaks the currently released iris-grib?

Well yes, but that shouldn't really be any more frequent than breaking changes to user code.
That is, if Iris-Grib only depended on Iris public api.
That should be perfectly practical -- and right now is nearly true -- but unfortunately still a way off.

no Iris tests which use iris-grib

I'm working on that = main reason why this PR is not yet ready.
I think we can + will still have tests that use iris-grib in an integration-type way, but we will remove anything depending on iris-grib internals. Again it's WIP.
Iris will still need to use dynamic import of iris-grib, to avoid circularity in testing.

Make Iris-grib no longer depend on Iris somehow.

Really not practical I think. But we certainly can ensure that Iris does not depend on Iris-grib.
We should ensure that :

  • Iris has no reliance on iris-grib specifics
  • iris-grib uses only public Iris api
    • .. and that includes the tests.

At this point, the hard part is still the tests :

  • some tests refer to internals of iris-grib, or gribapi : these should either be ditched or moved to iris-grib.
  • all tests in iris-grib use the Iris testing mechanics : need to ensure that is only public api
  • some Iris testing code is grib-specific : need to move to iris-grib

Copy link
Member Author

Choose a reason for hiding this comment

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

no Iris tests which use iris-grib ... = main reason why this PR is not yet ready.

Correction : that would be nice, but is not required for Iris 2.0 release, so I'm shelving it for now.
See iris-grib#88

- if [[ "$TEST_MINIMAL" != true ]]; then
INSTALL_DIR=$(pwd) ;
wget https://github.com/SciTools/iris-grib/archive/master.zip ;
Copy link
Member

Choose a reason for hiding this comment

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

These lines could be replaced with pip install git+https://github.com/SciTools/iris-grib.git@master

unzip -q master.zip ;
cd iris-grib-master ;
python setup.py install ;
cd - ;
fi

- PREFIX=$HOME/miniconda/envs/$ENV_NAME

# Output debug info
Expand Down
8 changes: 3 additions & 5 deletions INSTALL
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,9 @@ gdal 1.9.1 or later (https://pypi.python.org/pypi/GDAL/)
graphviz 2.18 or later (http://www.graphviz.org/)
Graph visualisation software.

grib-api 1.9.16 or later
(https://software.ecmwf.int/wiki/display/GRIB/Releases)
API for the encoding and decoding WMO FM-92 GRIB edition 1 and
edition 2 messages. A compression library such as Jasper is required
to read JPEG2000 compressed GRIB2 files.
iris-grib 0.11 or later
(https://github.com/scitools/iris-grib)
Iris interface to ECMWF's GRIB API

matplotlib 1.2.0 (http://matplotlib.sourceforge.net/)
Python package for 2D plotting.
Expand Down
3 changes: 3 additions & 0 deletions conda-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,6 @@ nc_time_axis
pandas
python-stratify
pyugrid

# Iris extensions (i.e. key tools that depend on Iris)
Copy link
Member Author

Choose a reason for hiding this comment

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

Defines a concept of "Iris extensions"
This relates to where we have struggled with migrating grib-code to dask, circular dependencies etc.

What we found is that these :

  • depend on Iris + import it
  • only use public Iris API
  • are not included by Iris, except dynamically to support specific functions
  • can use Iris in their tests
  • are not tested in Iris
    • except integration tests on Iris-terms results (e.g. cube properties)
    • definitely not details of the extension within Iris tests

# iris_grib
11 changes: 6 additions & 5 deletions docs/iris/src/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,12 @@
modindex_common_prefix = ['iris']

intersphinx_mapping = {
'python': ('http://docs.python.org/2.7', None),
'numpy': ('http://docs.scipy.org/doc/numpy/', None),
'scipy': ('http://docs.scipy.org/doc/scipy/reference/', None),
'matplotlib': ('http://matplotlib.org/', None),
'cartopy': ('http://scitools.org.uk/cartopy/docs/latest/', None),
'cartopy': ('http://scitools.org.uk/cartopy/docs/latest/', None),
'iris-grib': ('http://iris-grib.readthedocs.io/en/latest/', None),
'matplotlib': ('http://matplotlib.org/', None),
'numpy': ('http://docs.scipy.org/doc/numpy/', None),
'python': ('http://docs.python.org/2.7', None),
'scipy': ('http://docs.scipy.org/doc/scipy/reference/', None),
}


Expand Down
16 changes: 7 additions & 9 deletions lib/iris/fileformats/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@
UriProtocol, LeadingLine)
from . import abf
from . import um
try:
from . import grib as igrib
except ImportError:
igrib = None

from . import name
from . import netcdf
from . import nimrod
Expand Down Expand Up @@ -71,10 +66,13 @@
# GRIB files.
#
def _load_grib(*args, **kwargs):
if igrib is None:
raise RuntimeError('Unable to load GRIB file - the ECMWF '
'`gribapi` package is not installed.')
return igrib.load_cubes(*args, **kwargs)
try:
from iris_grib import load_cubes
except ImportError:
raise RuntimeError('Unable to load GRIB file - '
'"iris_grib" package is not installed.')

return load_cubes(*args, **kwargs)


# NB. Because this is such a "fuzzy" check, we give this a very low
Expand Down
Loading