Skip to content
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

Allow options for non-enabled backends #6196

Merged
merged 2 commits into from
May 17, 2024

Conversation

douglas-raillard-arm
Copy link
Contributor

An attempt at fixing: #3587

This allows passing options for backends that have not been enabled, so that it becomes possible for a library to return plots that have been styled for multiple backends, leaving the choice of what backend to use to the end user.

This change seems to work, but options passed to non-enabled backends are discarded. If the user subsequently changes the backend and tries to plot the object, the styling for that backend is just the default. However, if multiple backends are already enabled when the .options() is called, everything works as expected.

Note that I'm not very familiar with the internals of the Options system of holoviews, so this PR was hacked together until it worked, rather than being really planned by knowing how it should work.

@douglas-raillard-arm
Copy link
Contributor Author

douglas-raillard-arm commented Apr 16, 2024

This was tested in jupyterlab with:

# Cell 1
import holoviews as hv
hv.extension('bokeh')
hv.extension('matplotlib')
x = hv.Curve([0,1])

# Cell 2
y = x.options(color='green', backend='matplotlib').options(color='red', backend='bokeh')
y

# Cell 3
x.options(color='red', backend='bokeh').options(color='green', backend='matplotlib')

# Cell 4
# Switch to the other backend and re-display an already-built plot. As long as the backend was enabled (even if not the one in use), this works
hv.extension('bokeh')
y

Note that the code in the original issue #3587 still fails, I'll see if I can fix that as well:

import holoviews as hv
from holoviews import opts

hv.extension('matplotlib')

hv.Curve([1,2])\
 .opts(opts.Curve(backend='matplotlib', fig_size=400)) \
 .opts(opts.Curve(backend='bokeh', height=300, width=400))

@douglas-raillard-arm
Copy link
Contributor Author

Updated PR with fixes for the original issue as well

@philippjfr
Copy link
Member

This seems like a fabulous idea if we can make it work.

@douglas-raillard-arm
Copy link
Contributor Author

Updated with PR rebased on main and fixed (and simplified) to pass the test suite.

@hoxbro hoxbro added the type: enhancement Minor feature or improvement to an existing feature label Apr 17, 2024
@TheoMathurin
Copy link
Contributor

+1 for this feature when you want to delay the choice of backend to later stages for different contexts.

@hoxbro hoxbro linked an issue Apr 17, 2024 that may be closed by this pull request
@hoxbro hoxbro requested a review from philippjfr April 17, 2024 15:35
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

This looks right to me, but the proof is really in the pudding. A few things we need to check here:

  1. Will validation of options still work after the backend is imported (I think so)
  2. What happens to invalid options that were set when validation wasn't available
  3. How do we test this? Unloading a backend is difficult, so we may have to test it with a fake custom backend.

@hoxbro hoxbro changed the title Allow options for non-enabled backends (#3587) Allow options for non-enabled backends Apr 17, 2024
@douglas-raillard-arm
Copy link
Contributor Author

Will validation of options still work after the backend is imported (I think so)

From my experiment, if you set an option for a backend that is not imported yet, the option is just discarded, e.g. if you use .options(foo=1, backend='matplotlib') when the matplotlib backend is not imported, then it's as good as doing nothing.

What happens to invalid options that were set when validation wasn't available

They are not set, they are discarded. If you enable the backend in question later on, the plot will just have the default styling, none of your previous options will have been saved.

Maybe it would be possible to save the unvalidated options, but I'm not sure how useful that would be. Our jupyter use-case is like that:

import holoviews as hv
hv.extension('bokeh')

# the_library_func() returns a plot styled for all backends,
# so the user can choose any backend and don't need to
# import backends they won't use.
plot = the_library_func()
plot

In that scenario, it's not hard for the user to just pick a backend before calling any library code.

How do we test this? Unloading a backend is difficult, so we may have to test it with a fake custom backend.

I was also thinking it would be a problem. As it stands I see two solutions:

  1. Use multiprocessing or concurrent.futures.ProcessPoolExecutor() to run the test in a fresh process. The ProcessPoolExecutor is the best option as it transparently re-raises exceptions in the calling process so very little boilerplate.
  2. A terrible hack: make a context manager that removes some modules from sys.modules, then run the code, then restores sys.modules. This should work in terms of providing a "fresh set of modules", but any object produced this way will be subtly incompatible with the ones created in normal conditions, e.g. hv.Curve instances will not pass isinstance(x, hv.Curve) outside of the context manager since they will belong to different classes, even if those classes have the exact same name and code.

I'd say that 2. is only reasonable if it saves a lot of re-import of other modules that can be kept as they are. Otherwise, 1. is much likely to work without troubles. I tried 2. with polars and it failed pretty badly, so really not the most robust approach.

@hoxbro
Copy link
Member

hoxbro commented Apr 18, 2024

For checking, I would propose having the test as strings and then running the command with a subprocess, like what I have done in this test.

@philippjfr
Copy link
Member

I would just register a new backend and test with that. We already have other tests doing that.

Maybe it would be possible to save the unvalidated options, but I'm not sure how useful that would be.

I would say that I would expect them to persist but for them to be validated and perhaps raise warnings if it detects invalid options.

@douglas-raillard-arm
Copy link
Contributor Author

What area should host that feature ? I'm not familiar with the rendering infrastructure, so it's easy to discard what cannot be validated, and it would be rather easy to hang on to the unexpanded options, but I have no idea where these options should be actually processed

douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this pull request Apr 24, 2024
BREAKING CHANGE

Remove the matplotlib options for holoviews plot as that currently
requires the matplotlib backend to be enabled just to validate the options.

If and when this gets merged, this commit can be reverted:

holoviz/holoviews#6196
github-actions bot pushed a commit to ARM-software/lisa that referenced this pull request May 7, 2024
BREAKING CHANGE

Remove the matplotlib options for holoviews plot as that currently
requires the matplotlib backend to be enabled just to validate the options.

If and when this gets merged, this commit can be reverted:

holoviz/holoviews#6196
@hoxbro hoxbro enabled auto-merge (squash) May 17, 2024 11:22
@philippjfr
Copy link
Member

I'm okay with merging this as is. We'll be doing some work on making the options system more easily testable and we will follow up by allowing the options to be transferred when the backend is eventually imported.

Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 88.38%. Comparing base (023ad71) to head (ef99063).
Report is 1 commits behind head on main.

Files Patch % Lines
holoviews/util/__init__.py 28.57% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6196      +/-   ##
==========================================
- Coverage   88.39%   88.38%   -0.01%     
==========================================
  Files         323      323              
  Lines       67609    67613       +4     
==========================================
+ Hits        59761    59763       +2     
- Misses       7848     7850       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoxbro hoxbro merged commit 8b63dc2 into holoviz:main May 17, 2024
13 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend specific options raise error when backend isn't loaded
4 participants