Iris options: NetCDF #2467

DPeterK · 2017-03-29T16:10:12Z

Adds the options module to master (cf. #2462). In particular, this PR adds an Option to control certain aspects of NetCDF file handling, specifically what to do with a custom conventions attribute on saving a cube to a NetCDF file.

In this sense this PR is proposing a very similar change to the excellent suggestion made by @morwenna-01 in #2457, but does so in the manner suggested there by @bjlittle.

bjlittle

@dkillick This is great, but I'm keen to understand how this might scale as more options are added.

That is, we know that's going to happen pretty swiftly on adoption, so there will be separate option name-spaces for netcdf and parallel (or whatever for dask) ... how are these extra options added. Can you preempt that by having a common behaviour and state that can be inherited? Or is your strategy for us to refactor/rationalise at the point when a new additional option is added?

It would also be handy to be able to do

  >>> iris.options

or

>>> print(iris.options)

and see what options are available. Or is your expectation for the user to do help(iris.options) ?

I've not specifically gone through your tests, in case there is fall out from my current comments.

bjlittle · 2017-03-30T09:01:03Z

lib/iris/options.py

+        """
+        self.__dict__['conventions_override'] = conventions_override
+
+    def __str__(self):


Use __repr__ instead, otherwise you get this behaviour ...

>>> iris.options.netcdf <iris.options.NetCDF object at 0x7fdc0b9f60d0> >>> print iris.options.netcdf NetCDF options: conventions_override=False. >>>

I didn't have much success with __repr__ either 😒

bjlittle · 2017-03-30T09:03:02Z

lib/iris/options.py

+
+    def __str__(self):
+        msg = 'NetCDF options: conventions_override={}.'
+        return msg.format(self.conventions_override)


Since there's potential for more option attributes to come, this pattern is handy ...

msg = 'NetCDF options: conventions_override={self.conventions_override!r}.' return msg.format(self)

bjlittle · 2017-03-30T09:04:22Z

lib/iris/options.py

+import warnings
+
+
+class NetCDF(object):


@dkillick Is this thread safe? We'll want that for dask usage ...

Will we though? It is interesting that dask's set_options is not thread-safe. I left this like this for precisely this reason -- so that we'd get some commenting on it.

In fact I'd go so far as to argue that we don't want options thread-safe (!) -- my reason being that if I set an option I'd like it to be available in all threads, not just the calling thread. Of course, this may indicate some mixed-up thinking about how threads work, so I'm happy to stand corrected about this one.

I guess we'll be driven by the behaviour. Let's see how it plays!

I'd say if you provide it as a context manager, then it's not a one-off switch to set-and-forget.
In that case, thread-safe is definitely preferable IMHO.

bjlittle · 2017-03-30T09:05:39Z

lib/iris/options.py

+
+    def __setattr__(self, name, value):
+        if name not in self.__dict__:
+            msg = "'Future' object has no attribute {!r}".format(name)


Cut'n paste carnage ... Future?

Whoops! Good spot 😉

bjlittle · 2017-03-30T09:10:25Z

lib/iris/options.py

+
+    @property
+    def _defaults_dict(self):
+        return {'conventions_override': False}


I see the intent, but there's a slight disconnect here, in that there is a default value in the formal argument list of the __init__.

These could contradict each other ... it would be nice to specify this in one place somehow.

True! I'll poke it and see what I can do.

bjlittle · 2017-03-30T09:12:04Z

lib/iris/options.py

+        if name not in self.__dict__:
+            msg = "'Future' object has no attribute {!r}".format(name)
+            raise AttributeError(msg)
+        if value not in [True, False]:


Are we restricting all option attributes to be only bool?

Yes, in a "maximise work not done" sense 😉 -- in this case I only need to care about bools, so I have. I do have a plan for making this more broad, but that will appear in #2462, where I need it.

pp-mo · 2017-03-30T11:10:29Z

I fell this doesn't follow the spirit of what you mentioned

the manner suggested there by @bjlittle.

I think I'd find it much tidier to have 'iris.netcdf.options' instead of 'iris.options.netcdf', because it scales nicely : In particular, the definition and usage of the options settings relating to a given area (e.g. netcdf) are kept together and contained in the relevant place, i.e. the sub-module.
However, I think it also makes very good sense to write the class code once and have that in 'iris.options'. It should be similar to the iris.Future usage, though I know we have outstanding concerns about the thread-safe usage.

I suspect we may want a top-level 'iris.options', mostly just so we can print a summary.
In that case, you could make it a dictionary which the other modules populate on install.
E.G. netcdf.py could have something like ...

from iris.options import Options, options as iris_options

options = Options(conventions_override=False, other_things=2)
iris_options['netcdf'] = options

Then you can ...

from iris.fileformats.netcdf import options as nc_options
. . .
    with nc_options.context(conventions_override=True):
       save(...)

Also print iris.options.options and/or print iris.fileformats.netcdf.options

DPeterK · 2017-03-30T11:14:08Z

@pp-mo I think it is tidier to have all of the options in one place (there's another one for controlling parallel processing options), and I intend them to inherit from a superclass Option. All of this suggests to me that we lump all the Options together into one place...

pp-mo · 2017-03-30T11:22:12Z

it is tidier to have all of the options in one place

Not if they are going to divide into separate self-contained areas of interest.
Which it already sounds like they are doing.

This is very much like iris.Future...
That is getting a bit awkward as more things are added : they don't really interact or belong together.
The difference with iris.Future is that we expect all the components to be transient
-- when the implementation matures + deprecation cycle is complete, they get removed again.
But that wouldn't happen with iris.options -- the complexity will simply increase steadily over time.

DPeterK · 2017-03-31T15:49:19Z

Following from an offline conversation with @pp-mo I've moved the Options to config.py, which we think might be the best fit given the code that already exists within Iris. It is possible that doing this has blatted some of @bjlittle's comments on these changes without addressing them... sorry!

marqh · 2017-04-07T08:45:17Z

i think i am following the intent of this

the ability to control low level options in these ways seems useful

I am wondering how these are described for users. I think there could be some be some explanatory docs in the refdocs for config to better handle this.
Perhaps a user guide section, or references from the user guide in the appropriate sections, e.g. save

I also wonder whether the sample.site.cfg should include this, and perhaps remove some of the other deprecated options whilst we are there:
https://github.com/dkillick/iris/blob/8dace2350feac49fa92461665233548e33ff9c58/lib/iris/etc/site.cfg.template

this file is a useful reference, under source control

(cherry picked from commit d910548)

DPeterK · 2017-04-10T10:36:52Z

Rebased.

bjlittle · 2017-04-10T10:54:34Z

lib/iris/config.py

+            If `False` (the default), specifies that Iris should set the
+            CF Conventions version when saving cubes as NetCDF files.
+            If `True`, specifies that the cubes being saved to NetCDF should
+            set the cf conventions version for the saved NetCDF files.


@dkillick Apologies for being picky, but is this cf conventions or CF Conventions ...

Nono, good spot! My bad, I'll change (it should be the latter).

bjlittle · 2017-04-10T10:56:10Z

lib/iris/config.py

+        * Specify, for the lifetime of the session, that we want all cubes
+          written to NetCDF to define their own CF Conventions versions::
+
+            iris.options.netcdf.conventions_override = True


@dkillick This has gone state ... should now be iris.config.netcdf.conventions_override

bjlittle · 2017-04-10T10:56:36Z

lib/iris/config.py

+        * Specify, with a context manager, that we want a cube written to
+          NetCDF to define its own CF Conventions version::
+
+            with iris.options.netcdf.context(conventions_override=True):


@dkillick Same reek of staleness here, thanks

bjlittle · 2017-04-10T11:04:28Z

lib/iris/config.py

+    def _defaults_dict(self):
+        return {'conventions_override': {'default': False,
+                                         'options': [True, False]},
+                }


@dkillick The _defaults_dict is private to the class, so is there a real need to make this a property? I can understand making this a read-only property if it was part of the public API, but it's not, so is there a need to be this restrictive or am I missing something? (most likely)

Why can't the self._defaults_dict be created and initialised just the once in the __init__ ?

bjlittle · 2017-04-10T11:10:41Z

lib/iris/config.py

+    def __setattr__(self, name, value):
+        if name not in self.__dict__:
+            # Can't add new names.
+            msg = "'Option' object has no attribute {!r}".format(name)


@dkillick This isn't an Option instance ... or are you thinking in the generic sense? If so, would it be better to say something along the lines of "{!r} option has no attribute {!r}.".format(type(self).__name__, name) ...

bjlittle · 2017-04-10T11:16:25Z

lib/iris/config.py

+            # anything goes.
+            if value not in self._defaults_dict[name]['options']:
+                good_value = self._defaults_dict[name]['default']
+                wmsg = ('Attempting to set bad value {!r} for attribute {!r}. '


Minor suggestion, bad -> invalid

Do we want to raise a warning here, which may be silently ignored and use the default, or raise a hard exception ...

bjlittle · 2017-04-10T12:39:33Z

@dkillick You have quite a few test failures.

Without delving too deeply, I think that the failures might be related to the fact that the options are not thread safe? In that, a test is setting the conventions_override for the purpose of what it's trying to test, but this is a global setting that is interfering with other concurrent running tests ...

I'll leave you to dig and confirm whether this is the case or not ...

bjlittle · 2017-04-10T12:52:14Z

@dkillick We still need to consider how this approach is going to scale ... at the moment we will be adding all of the options into iris.config ... I think we should consider whether we can easily separate our concerns, or accept that iris.config may bloat quite quickly ...

Also, as a user of iris I want an easy way to check what possible options are available to me and what specific options are set for a group i.e. what NetCDF options are set?, what regrid options are set?, what are all the options groups? Do you have any thoughts on this ...

bjlittle · 2017-04-11T07:57:10Z

lib/iris/fileformats/netcdf.py


-        conventions = CF_CONVENTIONS_VERSION
+        if iris.config.netcdf.conventions_override:
+            conventions = cube.attributes['Conventions']


@dkillick The cube.attributes may not necessarily have any Conventions to use ...

I guess if that is the case, it seems reasonable to use the default CF_CONVENTIONS_VERSION instead rather than raise an exception ...

bjlittle · 2017-04-11T08:00:23Z

lib/iris/tests/unit/config/test_NetCDF.py

+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with Iris.  If not, see <http://www.gnu.org/licenses/>.
+"""Unit tests for the `iris.options.Paralle` class."""


@dkillick cut'n paste ...

And a typo!

DPeterK · 2017-04-11T09:40:32Z

We still need to consider how this approach is going to scale

At the moment iris.config is still small and, I think, the best place to have options like these. If it ends up getting big in the future then maybe we can think about shuffling things around then. It may also be clearer how/if to when we have a lot more examples of options like these.

DPeterK · 2017-04-11T10:05:25Z

@bjlittle we have a green light!! 🚦

bjlittle · 2017-04-11T14:43:27Z

@dkillick Awesome, thanks! 👍

DPeterK added the Status: Work in Progress label Mar 29, 2017

bjlittle requested changes Mar 30, 2017

View reviewed changes

bjlittle self-assigned this Mar 30, 2017

pp-mo mentioned this pull request Apr 3, 2017

Iris options (dask parallel processing) #2462

Closed

marqh mentioned this pull request Apr 7, 2017

ENH: Infer suitable non optional valid_range #2099

Closed

DPeterK added 5 commits April 10, 2017 11:36

Add options.py

fa8843f

(cherry picked from commit d910548)

Add NetCDF controlling Option

d957f99

Add and modify tests

6c75714

Corrections for non-functional tests

016f829

Move to config.py

af96e74

DPeterK force-pushed the options branch from 8dace23 to af96e74 Compare April 10, 2017 10:36

bjlittle reviewed Apr 10, 2017

View reviewed changes

Remove old options import

6f35125

bjlittle reviewed Apr 10, 2017

View reviewed changes

Review actions

6d2b735

DPeterK force-pushed the options branch from dd56137 to 6d2b735 Compare April 10, 2017 11:35

bjlittle reviewed Apr 11, 2017

View reviewed changes

Use mock in tests

03a8fb5

DPeterK force-pushed the options branch from b5267c6 to 03a8fb5 Compare April 11, 2017 12:30

bjlittle approved these changes Apr 11, 2017

View reviewed changes

bjlittle merged commit 538eed4 into SciTools:master Apr 11, 2017

bjlittle removed the Status: Work in Progress label Apr 11, 2017

DPeterK deleted the options branch April 11, 2017 14:44

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

bjlittle mentioned this pull request Apr 13, 2017

Allow user to explicitly set the attribute "Conventions" #2457

Closed

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

DPeterK mentioned this pull request Jul 20, 2017

Sharedata #2691

Closed

Iris options: NetCDF #2467

Iris options: NetCDF #2467

Uh oh!

Conversation

DPeterK commented Mar 29, 2017

Uh oh!

bjlittle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DPeterK commented Mar 30, 2017

Uh oh!

pp-mo commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DPeterK commented Mar 31, 2017

Uh oh!

marqh commented Apr 7, 2017

Uh oh!

DPeterK commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle commented Apr 10, 2017

Uh oh!

bjlittle commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjlittle Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DPeterK commented Apr 11, 2017

Uh oh!

pp-mo commented Mar 30, 2017 •

edited

Loading

pp-mo commented Mar 30, 2017 •

edited

Loading

bjlittle Apr 10, 2017 •

edited

Loading

bjlittle commented Apr 10, 2017 •

edited

Loading

bjlittle Apr 11, 2017 •

edited

Loading