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

RFC: Refactor Plots #1598

Closed
wants to merge 2 commits into from
Closed

RFC: Refactor Plots #1598

wants to merge 2 commits into from

Conversation

daschw
Copy link
Member

@daschw daschw commented Jul 17, 2018

GR and Plotly currently work on 0.7 on master. However, when I try to use PyPlot, I get the following:

julia> using Plots
[ Info: Recompiling stale cache file /home/dani/.julia/compiled/v0.7/Plots/ld3v.ji for module Plots

julia> pyplot()
┌ Warning: `warn()` is deprecated, use `@warn` instead.
│   caller = #warn#776(::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::Function, ::String) at deprecated.jl:1030
└ @ Base ./deprecated.jl:1030
WARNING: Couldn't initialize pyplot.  (might need to install it?)
┌ Warning: `info()` is deprecated, use `@info` instead.
│   caller = add_backend(::Symbol) at backends.jl:39
└ @ Plots ~/.julia/dev/Plots/src/backends.jl:39
INFO: To do a standard install of pyplot, copy and run this:

if !Plots.is_installed("PyPlot")
    Pkg.add("PyPlot")
end
withenv("PYTHON" => "") do
    Pkg.build("PyPlot")
end

# now restart julia!


ERROR: ArgumentError: Package Plots does not have PyPlot in its dependencies:
 - If you have Plots checked out for development and have
   added PyPlot as a dependency but haven't updated your primary
   environment's manifest file, try `Pkg.resolve()`.
 - Otherwise you may need to report an issue with Plots.

Stacktrace:
 [1] require(::Module, ::Symbol) at ./loading.jl:821
 [2] top-level scope at /home/dani/.julia/dev/Plots/src/backends/pyplot.jl:80
 [3] eval at ./boot.jl:319 [inlined]
 [4] _initialize_backend(::Plots.PyPlotBackend) at /home/dani/.julia/dev/Plots/src/backends/pyplot.jl:76
 [5] backend() at /home/dani/.julia/dev/Plots/src/backends.jl:185
 [6] backend(::Symbol) at /home/dani/.julia/dev/Plots/src/backends.jl:212
 [7] pyplot() at /home/dani/.julia/dev/Plots/src/backends.jl:24
 [8] top-level scope at none:0

It seems like we would have to have PyPlot in our dependencies for this to work. The same applies for all the other backends.
Hence, I propose to finally do the reorg of Plots with backend glue packages to get rid of the optional dependencies.
With this PR Plots only natively supports GR and Plotly. For all the other backends (still to be created) glue packages have to be installed and loaded. I have already implemented a PlotsPyPlot backend package locally that I plan to upload soon.
With this branch and the backend package the workflow is like this:

(v0.7) pkg> activate .julia/dev/PlotsPyPlot

julia> using Plots
[ Info: Recompiling stale cache file /home/dani/.julia/compiled/v0.7/Plots/ld3v.ji for module Plots

julia> backend()
Plots.GRBackend()

julia> Plots.showtheme(:wong)

gr_refactor

julia> pyplot()
┌ Warning: To use the `:pyplot` backend, run `using PlotsPyPlot`.
└ @ Plots ~/.julia/dev/Plots/src/backends.jl:208
Plots.GRBackend()

julia> using PlotsPyPlot

julia> Plots.showtheme(:wong, :deuteranopic)

pyplot_refactor

You can now still switch backends with gr(), plotly() and pyplot().
using PlotsPyPlot automatically calls pyplot.

I plan to implement further backends, whenever I find time and they are ready for 0.7, but I would really appreciate some help, maybe also with updating the docs.

I also wonder, if we should drop the GR dependency again with this refactor and update the PlotsGR package. It seems kind of weird that users need to install GR if they'd only like to use PlotsPyPlot or PlotsUnicodePlots for example.

I would really appreciate your feedback on this.

cc: @mkborregaard @piever @apalugniok @jheinen @ChrisRackauckas @Evizero

@daschw
Copy link
Member Author

daschw commented Jul 17, 2018

@daschw
Copy link
Member Author

daschw commented Jul 17, 2018

Also:

Julia 0.6

julia> @time using Plots
 23.636787 seconds (6.36 M allocations: 359.742 MiB, 0.96% gc time)

Julia 0.7 Plots#master

julia> @time using Plots
  5.960243 seconds (11.26 M allocations: 621.526 MiB, 7.59% gc time)

Julia 0.7 Plots#refactor

julia> @time using Plots
  5.059459 seconds (10.87 M allocations: 588.263 MiB, 7.35% gc time)

julia> @time using PlotsPyPlot
 10.296359 seconds (25.74 M allocations: 1.257 GiB, 5.81% gc time)
@time using PlotsPyPlot
 14.948731 seconds (35.50 M allocations: 1.779 GiB, 6.45% gc time)

@apalugniok
Copy link
Member

I've been pretty busy lately but here's what I understand from this and what I think. So in order to have PyPlot running with Plots on 0.7 we'd have to list it as a dependency. If that is the case I'm definitely in favour of the refactor. What is weird is that Plotly works even though we don't list it as a dependency? Also it seems like it shaves off a decent bit from the load time, which is a nice bonus 😄 . Overall I'm in favour of these changes! Great Work @daschw !!

As for GR I do think it'd make sense to drop the dependency and update PlotsGR for consistency purposes.

@daschw
Copy link
Member Author

daschw commented Jul 18, 2018

Thanks @apalugniok ! Plotly is no julia package, just a download of a js file.

Apparently (#918 (comment)) there is another solution using Requires now, which I am currently investigating. So I will report back when I have a comparison.

@piever
Copy link
Member

piever commented Jul 19, 2018

Great work!

Most of my Julia time is spent on my GSoC now so it's hard for me to contribute. I just wanted to say that I think Requires is a good solution if it works reliably. One thing to check to make sure that it works is the setcharheight but on GR backend. Meaning:

using Plots
plot(rand(10))

on Juno causes this error and it's a good test for the reorganization to see if this gets fixed.

FWIU the advantage of Requires compared to how it is now is that backend specific code is not loaded unless necessary so this should speed things up at start up.

If instead you see that Requires solution doesn't work satisfactorily I can maybe share the backend organization of Interact, as I've been quite happy with it so far, but switching to that model would require a fair amount of work.

@jheinen
Copy link
Member

jheinen commented Jul 19, 2018

The above script works for me. To induce the world-age-problem, I had to:

using Plots
display(plot(rand(10)))

ERROR: LoadError: MethodError: no method matching setcharheight(::Float64)
The applicable method may be too new: running in world age 21864, while current world is 21865.

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

The problem with the Requires approach and PyPlot (or any other package):
With

function __init__()
    @require PyPlot=[uuid] include(joinpath(@__DIR__, "backends/pyplot.jl"))
    ...
end

if I have no using PyPlot or import PyPlot in backends/pyplot.jl, I get an UndefVarError for PyPlot-specific stuff. Furthermore, both, the backend and Plots export plot. So without importing the backend we have this warning that both export plot and uses must be specified.
On the other hand, if I add a using PyPlot or import PyPlot in the backend code (within the require block) I get the Pkg error that PyPlot is not in Plots' dependencies when precompiling.

I hope that I am missing the obvious, easy and correct implementation 😟 .

@piever
Copy link
Member

piever commented Jul 19, 2018

AFAIU, It should be the user to do that, meaning the correct usage would be:

using Plots
import PyPlot
plot(...)

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

OK, thanks @piever! I will try this later

@piever
Copy link
Member

piever commented Jul 19, 2018

That's why the user should do:

using Plots
import PyPlot
plot(...)

If PyPlot is imported (without using there's no name clash). Within the @require block you should refer to PyPlot specific things as PyPlot.fct(...).

I wonder whehter:

pyplot() = @eval import PyPlot

would also be sufficient to trigger the code in the @require block.

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

I wonder whehter:

pyplot() = @eval import PyPlot

would also be sufficient to trigger the code in the @require block.

Interesting idea, I will check this.

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

I experimented a little with two example packages PkgA and PkgB:

PkgA

Corresponds to Plots with a default backend and conditional dependency on a second backend PkgB via Requires. Both export func(x) which is string(x) for the default backend and Symbol(x) for the second backend. You should be able to switch between backends with a() and b().

module PkgA

abstract type Backend end
export BackendA, BackendB, func, a, b

struct BackendA <: Backend end
struct BackendB <: Backend end

func(x, ::Type{BackendA}) = string(x)

mutable struct CurrentBackend
    pkg
end

CB = CurrentBackend(BackendA)
func(x) = func(x, CB.pkg)

a() = CB.pkg = BackendA
function b()
    if !(:PkgB in Symbol.(collect(values(Base.loaded_modules))))
        @eval import PkgB
    end
    CB.pkg = BackendB
end

using Requires
function __init__()
    @require PkgB = "7a818804-8b6b-11e8-33ae-c961580b5757" begin
        func(x, ::Type{BackendB}) = PkgB.func(x)
    end
end

end # module

PkgB

module PkgB

export func
func(x) = Symbol(x)

end # module

Unfortunately @piever's @eval import Backend proposal does not work:

julia> using PkgA

julia> b()
ERROR: ArgumentError: Package PkgA does not have PkgB in its dependencies:
 - If you have PkgA checked out for development and have
   added PkgB as a dependency but haven't updated your primary
   environment's manifest file, try `Pkg.resolve()`.
 - Otherwise you may need to report an issue with PkgA.

Stacktrace:
 [1] require(::Module, ::Symbol) at ./loading.jl:821
 [2] eval at ./boot.jl:319 [inlined]
 [3] b() at /home/dani/.julia/dev/PkgA/src/PkgA.jl:21
 [4] top-level scope at none:0

However, it seems to wotk without the @eval, if we change b() in PkgA to

function b()
    if !(:PkgB in Symbol.(collect(values(Base.loaded_modules))))
        @warn("run `import PkgB` before running `b()`.")
    else
        CB.pkg = BackendB
    end
end

Usage

julia> using PkgA

julia> func(0)
"0"

julia> b()
┌ Warning: run `import PkgB` before running `b()`.
└ @ PkgA ~/.julia/dev/PkgA/src/PkgA.jl:21

julia> import PkgB

julia> b()
BackendB

julia> func(0)
Symbol("0")

julia> a()
BackendA

julia> func(0)
"0"

So I guess our decision between reorg and Requires boils down to whether we want the user to do using PlotsPyPlot or using Plots; import PyPlot before running pyplot()

@piever
Copy link
Member

piever commented Jul 19, 2018

As an aside, I suspect that @eval fails because by default it is evaluated in the calling module (in this case Plots). Maybe @eval(Main, import PyPlot) would work (load the package in Main module). Not sure how clean of a solution this is.

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

Nice! @eval(Main, import PkgB) works:

julia> using PkgA

julia> b()
BackendB

julia> func(0)
Symbol("0")

However, if the package PkgB is not installed this will probably error. So we would need a check if a package is installed, and I'm not sure if that's possible with the new Pkg3. Or we could use try import Backend catch add_backend_string end I'm not sure how clean this is either.

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

As an aside, I suspect that @eval fails because by default it is evaluated in the calling module (in this case Plots). Maybe @eval(Main, import PyPlot) would work (load the package in Main module). Not sure how clean of a solution this is.

So we could also keep the current structure without Requires and replace the @evals in initialize_backend in the backend code with @eval(Main, ...)?

@piever
Copy link
Member

piever commented Jul 19, 2018

I would tend to prefer the Requires solution because it avoids unnecessary code loading. As it is now, even though one may only use GR, Plots still loads all backend codes. We should probably measure it, but if Requires, by removing the need to load some of these files, speeds up start up time, we should probably go for it.

To be honest though I find the:

using Plots; import PyPlot

approach saner and cleaner. One thing we could add is to put the CB.pkg = PyPlot inside the @require block so that as soon as the user does:

using Plots; import PyPlot

the backend is set (though maybe we should put some rules, like saying that this sets the backend only the first time one of the backends is loaded).

@daschw
Copy link
Member Author

daschw commented Jul 19, 2018

See #1601 for an alternative solution using Requires including benchmarks.

@daschw daschw closed this Aug 7, 2018
@daschw daschw deleted the refactor branch October 13, 2019 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants