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

Re-enable option system keyword validation #1277

Merged
merged 18 commits into from Apr 13, 2017

Conversation

Projects
None yet
3 participants
@jlstevens
Member

jlstevens commented Apr 12, 2017

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!

@jlstevens jlstevens added the WIP label Apr 12, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 12, 2017

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

This comment has been minimized.

Member

jbednar commented Apr 12, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

@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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

@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

This comment has been minimized.

Member

philippjfr commented Apr 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

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

This comment has been minimized.

Member

jbednar commented Apr 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

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

This comment has been minimized.

Member

jbednar commented Apr 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

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

This comment has been minimized.

Member

philippjfr commented Apr 13, 2017

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

This comment has been minimized.

Member

jbednar commented Apr 13, 2017

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 jlstevens added the ready label Apr 13, 2017

@jlstevens jlstevens removed the WIP label Apr 13, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

@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:

image

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!

override_kwargs = dict(options.kwargs)
old_allowed = group_options.allowed_keywords
override_kwargs['allowed_keywords'] = options.allowed_keywords + old_allowed

This comment has been minimized.

@philippjfr

philippjfr Apr 13, 2017

Member

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

@@ -314,7 +382,7 @@ def __call__(self, allowed_keywords=None, **kwargs):
"""
Create a new Options object that inherits the parent options.
"""
allowed_keywords=self.allowed_keywords if allowed_keywords is None else allowed_keywords
allowed_keywords=self.allowed_keywords if allowed_keywords in [None,[]] else allowed_keywords

This comment has been minimized.

@jlstevens

jlstevens Apr 13, 2017

Member

One line that ruined productivity for an evening! hoorah!

@philippjfr philippjfr added the options label Apr 13, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 13, 2017

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.

@philippjfr philippjfr merged commit 8ab7e4d into master Apr 13, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 78.81%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the option_system_validation branch Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment