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

select Plots backend via Preferences (or env variable) #4566

Merged
merged 39 commits into from
Dec 4, 2022

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Nov 28, 2022

Fix #4513.
Supersedes #4517 (borrows the added test).

This PR:

  • allows to select the default backend using an environment variable e.g. PLOTS_DEFAULT_BACKEND='unicodeplots' or make it persistent via Preferences.jl, e.g. Plots.set_default_backend!(:unicodeplots), all case-insensitive;
  • allows precompilation for the selected backend, thus improving ttfp for non GR backends;
  • avoid potential GR runtime problems (low-level c, since GR is conditionally loaded);
  • still allows to swap backends at runtime with Requires (making this PR non-breaking);
  • add Plots.diagnostics function.

We cannot remove the hard dependency on GR in Project.toml since it would be breaking, but it would be trivial to move GR from [deps] to [extras] in Plots 2.0.
One would then have to type Pkg.add(["Plots", "<mandatory chosen backend>"]) to use Plots, which I think is acceptable to ask and has the benefit of reducing Plots dependencies imprint (you won't have to download GR (and thus artifacts) if you chose to stick to another backend).

Ping @BeastyBlacksmith and @mkitti for discussion / review of the implementation.

@t-bltg t-bltg added enhancement improving existing functionality ttfp labels Nov 28, 2022
@t-bltg t-bltg changed the title select backend via Preferences select Plots backend via Preferences (or env variable) Nov 28, 2022
@t-bltg t-bltg force-pushed the prefs branch 2 times, most recently from b03ec13 to 86a1062 Compare November 28, 2022 23:43
Copy link
Contributor

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

In the special case of GR, should we do import GR within the top-level of Plots if it is the default backend?

Perhaps we should consider a more radical redesign in the near future?

  1. Each of the pieces of backend glue code could be independent subpackages. GRPlotsBackend, PyPlotPlotsBackend, etc.
  2. Most of the code currently in Plots.jl gets moved to a package named AbstractPlots.jl. All the backend packages depend on AbstractPlots.jl.
  3. Plots.jl becomes a small package which just depends on GRPlotsBackend.jl by default. Via Preferences.jl it can optionally load any of the other backend packages and act as a proxy for that package.

The advantage of this is that each backend now can be precompiled and has its own compiled cache. Also the project dependency tree becomes a bit clearer. One could get all the features of Plots by just doing using SomePlotsBackend.jl directly.

For loading with Requires.jl can then just do @eval using GRPlotsBackend.jl similar to what @timholy recommended here:
https://discourse.julialang.org/t/categoricalarrays-forces-openspecfun-jll-recompilation/87759/18?u=mkitti

The impact here for this PR is still acknowledging that the GR backend is a special case for the current architecture. That is Plots.jl has declared a dependency on GR.jl. One consequence of that special case is that we load GR.jl within Plots.jl rather than Main. Eventually we move that responsibility to GRPlotsBackend.jl.

@t-bltg
Copy link
Member Author

t-bltg commented Nov 29, 2022

In the special case of GR, should we do import GR within the top-level of Plots if it is the default backend?

This PR removes that, in order for users not wanting to use the default backend to not load GR. With all the issues encountered in GR recently (especially on windows), I think this is worth not loading GR at all if we can.
With this PR, one can use:

$ JULIA_PKG_PRECOMPILE_AUTO=0 julia -e 'import Plots; Plots.set_default_backend!(:unicodeplots)'
$ julia  # restart, to show persistent mode
julia> using Plots
julia> plot(1:2)  # uses `UnicodePlots`
[...]
julia> using Libdl
julia> any(map(x -> occursin("libGR", x), Libdl.dllist()))  # check that we didn't load `GR`
false
julia> exit(0)
$ JULIA_PKG_PRECOMPILE_AUTO=0 julia -e 'import Plots; Plots.set_default_backend!()'  # clear `Preferences` key

without ever loading GR, and making the UnicodePlots backend persistent.

I must still investigate why the downstream test are failing, but Plots test suite is green locally. ==> fixed

Thanks for the valuable comments here.

The advantage of this is that each backend now can be precompiled and has its own compiled cache.

I haven't though of that, and its a clear advantage. However, usually, the common scenario is sticking to a backend (everyone has its preferred backend, and usually only use a single one). So in practice, precompilation for a single backend might be sufficient to cover 90% of the use cases of Plots.

For loading with Requires.jl can then just do @eval using GRPlotsBackend.jl

Wouldn't the load time be increased, since you parse to evaluate the common Plots code in each backend ? I also want to get rid of the @eval Main ... currently used in Plots, this is hacky, and should be remove asap.

Most of the code currently in Plots.jl gets moved to a package named AbstractPlots.jl

This is a good idea for the long term, we'd have to export a lot of symbols from this module. This also enhances composability.

Overall I think this is the plan for the long term (let's open an issue about this specific re-design ? EDIT: #4567), but for the moment I'd like to be the least disruptive and try this out. I'm sure we're going to We might hit bugs and minimizing the changes minimizes the chances of hitting some.

@t-bltg t-bltg marked this pull request as draft November 29, 2022 11:21
@t-bltg t-bltg force-pushed the prefs branch 2 times, most recently from e76bee6 to 3f28b5b Compare November 29, 2022 12:01
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 90.76% // Head: 90.64% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (288256c) compared to base (5df0c5d).
Patch coverage: 65.25% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4566      +/-   ##
==========================================
- Coverage   90.76%   90.64%   -0.12%     
==========================================
  Files          41       41              
  Lines        8488     8756     +268     
==========================================
+ Hits         7704     7937     +233     
- Misses        784      819      +35     
Impacted Files Coverage Δ
src/Plots.jl 88.88% <ø> (+4.67%) ⬆️
src/backends/gr.jl 91.65% <ø> (ø)
src/backends/pgfplotsx.jl 88.88% <ø> (-0.13%) ⬇️
src/utils.jl 92.49% <50.00%> (-0.12%) ⬇️
src/init.jl 73.33% <53.84%> (-26.67%) ⬇️
src/backends.jl 81.50% <68.23%> (-13.40%) ⬇️
src/backends/pythonplot.jl 88.57% <100.00%> (-0.02%) ⬇️
src/examples.jl 99.20% <100.00%> (+1.61%) ⬆️
src/axes.jl 89.95% <0.00%> (-0.66%) ⬇️
src/consts.jl 100.00% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@t-bltg t-bltg force-pushed the prefs branch 17 times, most recently from c84c7ff to a7bbc94 Compare November 29, 2022 14:25
@t-bltg t-bltg force-pushed the prefs branch 4 times, most recently from d294a09 to 5f59f88 Compare December 4, 2022 12:15
@t-bltg t-bltg force-pushed the prefs branch 4 times, most recently from fcf0152 to 136064a Compare December 4, 2022 15:56
@t-bltg
Copy link
Member Author

t-bltg commented Dec 4, 2022

Thanks for the comments and the help here.

@mkitti
Copy link
Contributor

mkitti commented Dec 4, 2022

My pleasure. So are we dropping pyplot support?

That is breaking, no?

@t-bltg
Copy link
Member Author

t-bltg commented Dec 4, 2022

No, it will still supported for plots 1.x, but no more development and bug fixes. Seems acceptable no ? Maintaining both is a lot of work ...

Edit: I plan to make a discourse announcement for Plots 1.37.0 explaining all this.

@KristofferC
Copy link
Contributor

KristofferC commented Jan 31, 2023

This seems to have broken e.g. PGFPlotsX:

julia> pgfplotsx(size=(900,900), legend=true,grid = false,framestyle =:box,xlim = (1.1,5.3),ylim = (-0.1,3.0),xminorticks=2,yminorticks=2)
┌ Warning: Error requiring `PGFPlotsX` from `Plots`
│   exception =
│    LoadError: UndefVarError: LaTeXString not defined
│    Stacktrace:
│      [1] top-level scope
│        @ ~/.julia/packages/Plots/modjX/src/backends/pgfplotsx.jl:1027
│      [2] include(mod::Module, _path::RelocatableFolders.Path)
│        @ Base ./Base.jl:419
│      [3] include(x::RelocatableFolders.Path)
│        @ Plots ~/.julia/packages/Plots/modjX/src/Plots.jl:1
│      [4] top-level scope
│        @ ~/.julia/packages/Plots/modjX/src/backends.jl:101

Reported at KristofferC/PGFPlotsX.jl#308

@mkitti
Copy link
Contributor

mkitti commented Jan 31, 2023

Apparently, this line is not effective for some reason?

import LaTeXStrings: LaTeXString

@BeastyBlacksmith
Copy link
Member

I can't reproduce that. Which version is that?

@KristofferC
Copy link
Contributor

The following is the repro:

using Plots
using Plots.PlotMeasures
using LaTeXStrings
using PGFPlotsX
using DelimitedFiles

pgfplotsx(size=(900,900), legend=true,grid = false,framestyle =:box,xlim = (1.1,5.3),ylim = (-0.1,3.0),xminorticks=2,yminorticks=2)

Plots 1.38.3

@BeastyBlacksmith
Copy link
Member

Hmm.. so the issue seems to be loading PGFPlotsX before calling pgfplotsx (which is not neccessary, but should work anyways)

@mkitti
Copy link
Contributor

mkitti commented Jan 31, 2023

Removing using PGFPlotsX succeeds for me.

using Plots
using Plots.PlotMeasures
using LaTeXStrings
using DelimitedFiles

pgfplotsx(size=(900,900), legend=true,grid = false,framestyle =:box,xlim = (1.1,5.3),ylim = (-0.1,3.0),xminorticks=2,yminorticks=2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improving existing functionality ttfp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Use Preferences.jl to configure default loaded backend
4 participants