Re-enable option system keyword validation

[jlstevens]

This PR attempts to re-enable option system keyword validation, addressing issue #1101.

Before HoloViews supported multiple backends, we put a lot of effort into keyword validation. However, once we had multiple backends, it became problematic to have keywords in the opts magic applicable to some other backend other than the current one. As a result, validation was disabled entirely.

One issue is that the new form of validation needs to work across backends. I think the sensible thing is to validate against the loaded backends as you don't want to import all possible backends to see what keywords might make sense.

The next issue is more fundamental - our original validation system was based around a custom OptionError exception that knew about appropriate keywords. The idea was if something was invalid, raise an exception and present it to the user appropriately.

The issue now is that exceptions disrupt the code and each OptionError was specified per backend. What we want now is to figure out keywords that make sense for none of the loaded backends. In other words you need to take a union, and normally if you raise an exception you will only get one exception at a time.

The solution shown here is to allow StoreOptions to record the exception objects that are currently being silently ignored. This lets me collect all the invalid option reports together - if a particular key is reported as invalid for all loaded backends, I assume it really is invalid and reraise an OptionError with it, using the union of all the allowed_keywords across backends. I hope that makes sense!

One positive thing about this approach is that practically everything is implemented as classmethods on StoreOptions with almost zero changes to the Options and OptionTrees which are pretty hard to understand as it is.

That said, I'm not entirely sure this approach is 100% correct as it doesn't look at which element complained or what type of option complained (i.e plot or style). This is probably fixable by declaring the Options with a bit more (optional!) information such as the element name and backend. This shouldn't be too bad as the plotting code doesn't actually raise OptionError, instead they declare their allowable plot and style keywords which are used to build Option instances in the Store.register classmethod. As everything is declared consistently in one place, we should be able to supply Options and OptionErrors with the appropriate metadata to do the correct union operation.

Hope that makes sense!

[philippjfr]

The approach taken here seems reasonable and I'll be glad to see some validation again. The only thing I'd request is that at minimum we show the repr of the object in addition to the warning message.

[jbednar]

That all sounds good. After some thought, validating against only the loaded backends does seem to be the right thing to do. That way, if someone changes a notebook from mpl to bokeh, they'll immediately be warned about the options that no longer apply. And if they want to make a notebook that works for both backends, they have a way to declare that options for both will be appearing.

The only valid use case I can see that's not covered is if someone wants to write a notebook valid for multiple backends, but only ever use one backend at any one time (i.e., no per-cell backend switching), and doesn't want the other backends not in use to be loaded at all. But that seems like a rare case and a reasonable limitation, while trying to separate out the validation from the other backend code seems like it would be infeasible, so I agree with this approach.

[jlstevens]

@jbednar I agree with your analysis.

... doesn't want the other backends not in use to be loaded at all.

That does seem to be an odd thing to do. Why would you write a notebook for multiple backends but insist only one is imported at a time?

... trying to separate out the validation from the other backend code seems like it would be infeasible, so I agree with this approach.

The plotting backends define the plotting classes which declare their allowed keywords i.e allowed plot and style options (as parameters and a class attribute). In theory you could make a 'mock' backend with stub plotting classes that only have parameters and this attribute, allowing you ro declare what holoviews supports for a given backend without actually importing it.

It would be fair bit of effort (for little to no benefit) but it would be possible to declare everything needed for keyword validation/opts magic completion without importing anything external. My point is that the options system isn't inherently tied to anyone else's code so I'm not sure why anyone would want to do this!

[jlstevens]

@philippjfr Out of curiosity, couldn't this work already?

Trying to import the bokeh backend could still register all the plotting classes and style options even if bokeh isn't available. If we wanted, we could get that option tree even if things would then break were you to actually try to use those plotting classes. Again, just curious - I see no real reason to do this!

[philippjfr]

Trying to import the bokeh backend could still register all the plotting classes and style options even if bokeh isn't available. If we wanted, we could get that option tree even if things would then break were you to actually try to use those plotting classes. Again, just curious - I see no real reason to do this!

Seems like it would be a right pain wrapping all the bokeh imports.

[jlstevens]

Seems like it would be a right pain wrapping all the bokeh imports.

Agreed. I'm saying if we wanted to this, it wouldn't be an insane amount of work, just annoying. Thankfully we aren't doing this - I'm just wondering about the space of possibilities. :-)

[jlstevens]

Note that this PR will rely on a fix in #1285 - currently if you try to load only the bokeh backend, the matplotlib backend is also loaded, breaking Store.loaded_backends() which I am relying on.

[jbednar]

Why would you write a notebook for multiple backends but insist only one is imported at a time?

Well, the same reason one would write code that works with both Python 2 and 3. Just because we write our code to work with both, we don't then insist that people have both 2 and 3 installed to use it; they just use the code in whatever context makes sense for them. Similarly, one might plausibly want to write a notebook that supports both mpl and Bokeh, distributing it for various people to consume according to their own preference, without requiring everyone to have both backends installed.

Seems like it would be a right pain wrapping all the bokeh imports.

Agreed; I'm glad it could be done if it somehow became a priority, but there's no need to do that work in the foreseeable future.

[jlstevens]

What I might be able to do right away is this:

1. Set the Options.skip_invalid class parameter default back from True to False. This should be possible now the validation is backend aware. I think this should be the default anyway.
2. Make the validation use this flag i.e disable validation if Options.skip_invalid is True.

Then we can accommodate the scenario you mentioned by simply requiring Options.skip_invalid=True in the notebook.

[jbednar]

Set the Options.skip_invalid class parameter default back from True to False.

Assuming False means "warn when invalid options are detected" (not "abort when invalid options are detected), then yes, I think the default should clearly be False.

Then we can accommodate the scenario you mentioned by simply requiring Options.skip_invalid=True in the notebook.

Right. I think that's all true, though it means giving up on option validation entirely in that case, so probably what the developer should do then is to leave the option False during development, loading both backends but allowing options to be validated, then distribute with the option True to suppress warnings on user systems. Seems like a reasonable compromise.

[jlstevens]

Assuming False means "warn when invalid options are detected" (not "abort when invalid options are detected), then yes, I think the default should clearly be False.

That currently isn't the case but I don't see why I can't change it to make it the way you describe - currently it raises an OptionError exception instead of issuing a warning.

[jlstevens]

Actually I think this is an issue in the implementation, not the parameters:

    skip_invalid = param.Boolean(default=True, doc="""
       Whether all Options instances should skip invalid keywords or
       raise and exception. May only be specified at the class level.""")

    warn_on_skip = param.Boolean(default=True, doc="""
       Whether all Options instances should generate warnings when
       skipping over invalid keywords or not. May only be specified at
       the class level.""")

I suggest we just set the default of skip_invalid to False and the warn_on_skip=False could mean 'raise an exception instead of a warning' as opposed to 'do nothing'. If you want it to be silent, set skip_invalid to True - 'skip' means no warnings or exceptions.

Make sense?

Edit 1: The docstrings would need to be updated...
Edit 2: I suppose the names wouldn't be quite right warn_on_skip would then just be warn?

[philippjfr]

I suggest we just set the default of skip_invalid to False and the warn_on_skip=False could mean 'raise an exception instead of a warning' as opposed to 'do nothing'. If you want it to be silent, set skip_invalid to True - 'skip' means no warnings or exceptions.

Yes that makes sense to me.

[jbednar]

Yes, sounds good. Regarding the name change, I think the possible combinations here should act like the following when an invalid option is encountered:

• skip==False, warn==False : Raise exception
• skip==False, warn==True : Print warning
• skip==True, warn==False : Do nothing
• skip==True, warn==True : Do nothing

If that's true, then the names should be skip_if_invalid (default False) and warn_if_invalid (default True), with if_ optional. But the logic problem could be avoided altogether if you use a keyword option instead (policy_for_invalid_options='skip','warn','exception', or if that's too long, maybe invalid='skip','warn','exception').

[jlstevens]

@philippjfr I think this PR is ready for review.

It didn't turn out as ugly as I had anticipated and the new Keywords class does seem to make sense after all - it ended up simplifying a few things.

Here is an example of what happens:

In the first case, bgcolors doesn't exist for either backends (bokeh or matplotlib). In the second case, the bokeh backend is active but doesn't complain about aspect which isn't a bokeh option because it is recognized by matplotlib. If only the bokeh backend was loaded, there would be a similar warning.

We can worry about how we want Options.skip_invalid and Options.warn (a renamed parameter) in a separate PR. For now, I would love to see this merged!

[philippjfr]

Three lines that ruined both our productivity for several hours, yay!

[jlstevens]

One line that ruined productivity for an evening! hoorah!

[philippjfr]

Very glad that we're finally reenabling validation. Thanks for taking the hit on this, hunting down that bug was more than enough for me. Going to merge.