Added .options method for simplified option setting #2306
Conversation
philippjfr
added
type: feature
philippjfr
force-pushed
the
simplified_option_setting
branch
from
cf5eae6
to
77c94c2
Compare
|
I'm very happy with this PR now, the .options method is really convenient and because it has to actually resolve the difference between different kinds of options its also much stricter than other approaches, which should result in a better user experience. The main question I have now where to document it (I want a section in both the getting started and user guide) and what our guidance on using it is. Should we use it throughout the docs? @jbednar @jlstevens |
|
Ready to review. |
|
I think we should use this approach consistently throughout the docs, certainly in all places currently using .opts, but also replacing magics in many cases. |
| @@ -105,6 +105,75 @@ def __call__(self, *args, **params): | |||
| return StoreOptions.set_options(obj, options) | |||
|
|
|||
|
|
|||
| @classmethod | |||
| def expand_options(cls, options, backend=None): | |||
There was a problem hiding this comment.
If there are any well-defined units of code in this rather long method, I would like them pulled out as private helper classmethods. The warning generation bit can be split out this way I think and there might be other sensible units to pull out.
| @@ -1326,13 +1326,13 @@ def tree_to_dict(cls, tree): | |||
| return specs | |||
|
|
|||
| @classmethod | |||
| def propagate_ids(cls, obj, match_id, new_id, applied_keys): | |||
| def propagate_ids(cls, obj, match_id, new_id, applied_keys, backend=None): | |||
There was a problem hiding this comment.
Glad to see this generalized to support the backend argument.
| self.map(lambda x: setattr(x, 'id', None)) | ||
| elif clone: | ||
| obj = self.map(lambda x: x.clone(id=x.id)) | ||
| StoreOptions.set_options(obj, options, backend=backend, **kwargs) |
There was a problem hiding this comment.
I think obj=self is really confusing here as it is not accessed until the line:
StoreOptions.set_options(obj, options, backend=backend, **kwargs)Which can now be expressed in a way I find much more intuitive, removing the need for obj=self entirely:
StoreOptions.set_options(obj if clone else self, options, backend=backend, **kwargs)There was a problem hiding this comment.
I'll also note that although support for the clone argument is reasonable, I would have preferred to see it in a separate PR.
There was a problem hiding this comment.
I think obj=self is really confusing here as it is not accessed until the line:
Not quite sure what you mean, it's overridden in the conditional and used twice. I'd have to do this instead:
StoreOptions.set_options(obj if clone else self, options, backend=backend, **kwargs)
return obj if clone else self
|
I haven't looked at the changes to the docs (i.e notebooks) just yet but I have looked at the code and made a few comments. It mostly looks fine and I am happy the bulk of the new functionality is implemented as a classmethod on |
holoviews/util/__init__.py
Outdated
| if backend is None: | ||
| # Check option is invalid for all backends | ||
| found = [] | ||
| for lb in [b for b in loaded_backends if b != 'backend']: |
There was a problem hiding this comment.
We spotted this bug together - it should be backend (a variable) not 'backend' a string. Is there a unit test that would have caught this bug? The tests did go green....
There was a problem hiding this comment.
Just went through this, it's not tested because this is just an optimization to ensure it doesn't check the same backend twice.
philippjfr
force-pushed
the
simplified_option_setting
branch
3 times, most recently
from
046107b
to
10d7b96
Compare
|
Ready to review again. |
|
Happy to review once the merge conflicts are resolved. I doubt I'll have many new comments to add once that is done. |
philippjfr
force-pushed
the
simplified_option_setting
branch
from
10d7b96
to
ea7df5b
Compare
| """ | ||
| Generates an error message for an invalid option suggesting | ||
| similar options through fuzzy matching. | ||
| """ |
There was a problem hiding this comment.
I should have raised this earlier. Isn't all this logic what OptionError is for? The logic is certainly closely related so maybe OptionError should be enhanced or extended somehow...
There was a problem hiding this comment.
I did look into that but I can be much more eager about the validation here because the user can declare an explicit backend and because it has to resolve each option into the appropriate group. This also means I can supply a more specific error message. I think we should look at unifying the validation, especially once the magic allows declaring an explicit backend.
What I would like to know is why the OptionError handling is so horrendously complex. Why does it have to record errors? It seems like that code would be hugely simplified by a utility that checks whether an option is invalid across all backends, so you can eagerly raise as soon as you encounter an unsupported option rather than processing all the options and checking validity at the end and having 3 methods to record options, a context manager to control the option validation, a bunch of state on the OptsMagic etc.
Separately I'm still confused in what cases the existing code actually validates anything, it seems like .opts still has zero validation, since this raises no error:
hv.Curve([]).opts(plot=dict(some_nonexistent_option=3))
There was a problem hiding this comment.
What I would like to know is why the OptionError handling is so horrendously complex. Why does it have to record errors? It seems like that code would be hugely simplified by a utility that checks whether an option is invalid across all backends, so you can eagerly raise as soon as you encounter an unsupported option rather than processing all the options and checking validity at the end.
The main reason it is this way is because the errors should be derived from the allowed_keywords specified in the Options. It is true that these allowed keywords are set from the plot and style option declarations which is what your proposed utility would rely on directly. I'm certainly not opposed to simplifying the code with a more direct check but the current design allows more flexibility by having a level of indirection: plot and style option declarations -> allowed keywords on options and errors-> validation.
As I agree we rarely use the extra flexibility that this design affords us, I would be happy to see a PR that simplifies things to plot and style option declarations -> validation. My worry is that such a PR might have to touch a lot more than you might expect (e.g how the errors are processed and printed by the %opts magic(s)).
There was a problem hiding this comment.
Right, I definitely want to tackle and simplify that code because I'm having a really tough time following it and I think I'll much prefer an approach that eagerly errors when it hits an unsupported option. But as you say it touches a lot of hairy code so I think I'd like to do that separately.
Currently I'm leaning toward merging this PR and getting a dev release out and then to follow up with a PR to try to unify the option validation. What do you think?
There was a problem hiding this comment.
I'll much prefer an approach that eagerly errors when it hits an unsupported option.
That is exactly how it worked when we only ever had to worry about a single backend at a time. The complexity was introduced when we decided to merge the allowed keywords across backends which was implemented in such a manner as to extend the old system (e.g where the magics used the OptionError message to generate a pretty printed HTML suggestion).
Currently I'm leaning toward merging this PR and getting a dev release out and then to follow up with a PR to try to unify the option validation. What do you think?
Makes sense. I don't think of it as 'unifying' the option validation though (it is already unified, which is what makes it ugly). That PR would be more of a refactor/redesign by validating without looking at the allowed keywords for Options.
I suppose one thing the current design lets you do is to declare options for elements/plots that don't exist, using allowed_keywords to do validation. The current design keeps the option system decoupled from elements/plots with allow_keywords acting as the link between them (set somewhere on Store iirc). This (in theory) could let you use the option system in a completely different project/library.
In practice, this decoupling doesn't seem to do much useful (unless you can imagine using the options system for something without needing a corresponding element/plot). In other words, I think there is a sound design underlying the way it is right now but 1) it doesn't add much of value in practice 2) it is becomes complicated once you start taking the union between backends. For this reason, I would be happy to see things simplified, even if it means coupling the options system closer to the elements and plot classes.
There was a problem hiding this comment.
Don't think we necessarily need to couple the two closer together (although I haven't considered all the implications). I just feel like it comes down to the way you iterate over the backends. Currently it seems like looping over the backends is the outer loop, which means you always iterate over all backends and can't raise until you've processed them all. The approach I've taken is to validate the current (or explicitly selected) backend first, and if an option is invalid for that backend it falls back to checking whether the option is valid for other backends. This means that as soon as an invalid option is encountered it can raise an error (or warning) rather than having to process everything first and only asserting whether an option was invalid at the end.
My only question then is about the OptsMagic, is there any reason why that can't raise eagerly or does it somehow have to catch errors and reraise them in a special way?
There was a problem hiding this comment.
does it somehow have to catch errors and reraise them in a special way?
That is exactly what it does.
There was a problem hiding this comment.
Okay, I can still do that easily enough.
There was a problem hiding this comment.
Though I will note it does it using the validation_error_message method which might mean there is indeed one single place where your approach could fit in nicely. Of course, there may still be other consequences I haven't thought about...e.g how will normal error validation work if you work with option trees directly?
There was a problem hiding this comment.
When you work with an OptionTree directly the allowed_keywords are well defined and no cross-backend validation is needed so that should be fine. I'll open a PR branching off this one to see what it could look like.
|
Other than our ongoing discussion (which will become another PR), I am happy to merge once all the tests are green (though I think another PR that is about to be merged will touch the test data). |
|
Making good progress on getting the validation unified so +1 on merging asap. This PR shouldn't touch test data so I'll try to get it passing without waiting on the other PR. |
|
Also finding a number of bugs in the existing option validation, so refactoring this is definitely a good idea. How do you feel about dropping the skip_invalid option? I feel like it should either warn or error, although I'm happy to be persuaded otherwise until we have a way of specifying an explicit backend in the opts magic. |
Fine by me. If I remember correctly, that was the first approach we had when we started supporting multiple backends. I do think it is less relevant now. |
philippjfr
force-pushed
the
simplified_option_setting
branch
from
ea7df5b
to
a99d4a1
Compare
|
Ready to merge now, refactoring will happen in #2354. |
|
As the tests are passing, I'll go ahead and merge. |
Adds a .options method to all objects making it simpler to set options in a flattened way. Two formats are supported:
and the more explicit:
which allows usage on complex nested objects.