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

Add GeoInterfaceMakie as a weak dependency #404

Merged
merged 13 commits into from
Dec 27, 2023
Merged

Conversation

felixcremer
Copy link
Contributor

@felixcremer felixcremer commented Nov 29, 2023

This now adds a weak dependency which depends on Makie to allow for the easy plotting of geometries with Makie.
This needs Makie 0.20 and julia 1.9.

Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

I'm overall okay with this given the discussion in JuliaGeo/GeoInterface.jl#99 (comment)

ext/ArchGDALGeoInterfaceMakie.jl Outdated Show resolved Hide resolved
ext/ArchGDALGeoInterfaceMakie.jl Outdated Show resolved Hide resolved
src/ArchGDAL.jl Outdated Show resolved Hide resolved
@yeesian yeesian requested a review from rafaqz December 1, 2023 04:37
@visr
Copy link
Collaborator

visr commented Dec 1, 2023

It'd be a pity if this doesn't work as a weak dep. I haven't looked at it yet, but it would be nice to understand first if there is a fundamental reason that it cannot work.

If rather than using the enable macro we need a different implementation of the same functionality that supports weak deps that of course would also be fine.

@yeesian yeesian requested a review from visr December 1, 2023 05:27
@felixcremer
Copy link
Contributor Author

This is the error that I get, when I try to set it up as in the first commit.
I am not sure, what is happening here, but apperantly it doesn't like the fact, that the extension depends on Makie and I am loading GeoInterfaceMakie in the extension module.
When I set the extension up to depend on GeoInterfaceMakie directly this works, but this would mean, that users have to load GeoInterfaceMakie explicitely and I would rather like to hide that from the user side.

According to this thread one should be able to load a package in the extension if we add it to the normal list of dependencies.
https://discourse.julialang.org/t/using-multiple-packages-in-extensions-for-julia-1-9/98237
Then the GeoInterfaceMakie packages will be installed but not loaded if Makie is not loaded.

julia> using GADM
       using ArchGDAL
       using GLMakie

       deu = GADM.get("DEU")
       #projdeu = ArchGDAL.reproject(only(deu.geom), EPSG(4326), ProjString(proj))
       plot(deu)
[ Info: Precompiling GADM [a8dd9ffe-31dc-4cf5-a379-ea69100a8233]
ERROR: Makie.PlotMethodError(Any, Tuple{NamedTuple{(:geom, :GID_0, :COUNTRY), Tuple{Vector{ArchGDAL.IGeometry{
Stacktrace:
  [1] _plot!(p::ERROR: MethodError: no method matching namemap(::Type{ArchGDAL.OGRwkbGeometryType})
The applicable method may be too new: running in world age 33474, while current world is 33726.

Closest candidates are:
  namemap(::Type{ArchGDAL.OGRwkbGeometryType}) (method too new to be called from this world context.)
   @ ArchGDAL Enums.jl:214
  namemap(::Type{LibGit2.Consts.GIT_CONFIG})
   @ LibGit2 Enums.jl:214
  namemap(::Type{Makie.Interpolation}) (method too new to be called from this world context.)
   @ Makie Enums.jl:214
  ...

Stacktrace:
  [1] _symbol(x::ArchGDAL.OGRwkbGeometryType)
    @ Base.Enums ./Enums.jl:37
  [2] show(io::IOContext{IOBuffer}, x::ArchGDAL.OGRwkbGeometryType)
    @ Base.Enums ./Enums.jl:45
  [3] show_typeparams(io::IOContext{IOBuffer}, env::Core.SimpleVector, orig::Core.SimpleVector, wheres::Vector{TypeVar})
    @ Base ./show.jl:707
  [4] show_datatype(io::IOContext{IOBuffer}, x::DataType, wheres::Vector{TypeVar})
    @ Base ./show.jl:1092
  [5] show_datatype
    @ ./show.jl:1058 [inlined]
  [6] _show_type(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:958
  [7] show(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:950
  [8] show_typeparams(io::IOContext{IOBuffer}, env::Core.SimpleVector, orig::Core.SimpleVector, wheres::Vector{TypeVar})
    @ Base ./show.jl:707
  [9] show_typealias(io::IOContext{IOBuffer}, name::GlobalRef, x::Type, env::Core.SimpleVector, wheres::Vector{TypeVar})
    @ Base ./show.jl:740
 [10] show_typealias(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:793
 [11] _show_type(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:955
 [12] show(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:950
 [13] show_datatype(io::IOContext{IOBuffer}, x::DataType, wheres::Vector{TypeVar})
    @ Base ./show.jl:1081
 [14] show_datatype
    @ ./show.jl:1058 [inlined]
 [15] _show_type(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:958
 [16] show(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:950
 [17] show_typeparams(io::IOContext{IOBuffer}, env::Core.SimpleVector, orig::Core.SimpleVector, wheres::Vector{TypeVar})
    @ Base ./show.jl:707
 [18] show_datatype(io::IOContext{IOBuffer}, x::DataType, wheres::Vector{TypeVar})
    @ Base ./show.jl:1092
 [19] show_datatype
    @ ./show.jl:1058 [inlined]
 [20] _show_type(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:958
--- the last 9 lines are repeated 1 more time ---
 [30] show(io::IOContext{IOBuffer}, x::Type)
    @ Base ./show.jl:950
 [31] sprint(f::Function, args::Type; context::IOContext{Base.TTY}, sizehint::Int64)
    @ Base ./strings/io.jl:112
 [32] sprint
    @ ./strings/io.jl:107 [inlined]
 [33] #print_type_bicolor#540
    @ ./show.jl:2491 [inlined]
 [34] show_tuple_as_call(io::IOContext{Base.TTY}, name::Symbol, sig::Type; demangle::Bool, kwargs::Nothing, argnames::Vector{Symbol}, qualified::Bool, hasfirst::Bool)
    @ Base ./show.jl:2472
 [35] show_tuple_as_call
    @ ./show.jl:2441 [inlined]
 [36] show_spec_linfo(io::IOContext{Base.TTY}, frame::Base.StackTraces.StackFrame)
    @ Base.StackTraces ./stacktraces.jl:244
 [37] print_stackframe(io::IOContext{Base.TTY}, i::Int64, frame::Base.StackTraces.StackFrame, n::Int64, ndigits_max::Int64, modulecolor::Symbol)
    @ Base ./errorshow.jl:730
 [38] print_stackframe(io::IOContext{Base.TTY}, i::Int64, frame::Base.StackTraces.StackFrame, n::Int64, ndigits_max::Int64, modulecolordict::IdDict{Module, Symbol}, modulecolorcycler::Base.Iterators.Stateful{Base.Iterators.Cycle{Vector{Symbol}}, Union{Nothing, Tuple{Symbol, Int64}}, Int64})
    @ Base ./errorshow.jl:695
 [39] show_full_backtrace(io::IOContext{Base.TTY}, trace::Vector{Any}; print_linebreaks::Bool)
    @ Base ./errorshow.jl:594
 [40] show_full_backtrace
    @ ./errorshow.jl:587 [inlined]
 [41] show_backtrace(io::IOContext{Base.TTY}, t::Vector{Base.StackTraces.StackFrame})
    @ Base ./errorshow.jl:791
 [42] showerror(io::IOContext{Base.TTY}, ex::Makie.PlotMethodError, bt::Vector{Base.StackTraces.StackFrame}; backtrace::Bool)
    @ Base ./errorshow.jl:90
 [43] showerror(io::IOContext{Base.TTY}, ex::Makie.PlotMethodError, bt::Vector{Base.StackTraces.StackFrame})
    @ Base ./errorshow.jl:86
 [44] display_repl_error(io::Base.TTY, stack::VSCodeServer.EvalErrorStack)
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.60.2/scripts/packages/VSCodeServer/src/repl.jl:259
 [45] evalrepl(m::Module, line::Expr, repl::REPL.LineEditREPL, main_mode::REPL.LineEdit.Prompt)
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.60.2/scripts/packages/VSCodeServer/src/repl.jl:201
 [46] top-level scope
    @ ~/.vscode/extensions/julialang.language-julia-1.60.2/scripts/packages/VSCodeServer/src/repl.jl:169
 [47] eval
    @ ./boot.jl:370 [inlined]
 [48] eval_user_input(ast::Any, backend::REPL.REPLBackend, mod::Module)
    @ REPL ~/.julia/juliaup/julia-1.9.3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/REPL.jl:153
 [49] repl_backend_loop(backend::REPL.REPLBackend, get_module::Function)
    @ REPL ~/.julia/juliaup/julia-1.9.3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/REPL.jl:249
 [50] start_repl_backend(backend::REPL.REPLBackend, consumer::Any; get_module::Function)
    @ REPL ~/.julia/juliaup/julia-1.9.3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/REPL.jl:234
 [51] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool, backend::Any)
    @ REPL ~/.julia/juliaup/julia-1.9.3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/REPL.jl:379
 [52] run_repl(repl::REPL.AbstractREPL, consumer::Any)
    @ REPL ~/.julia/juliaup/julia-1.9.3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/REPL.jl:365
 [53] (::Base.var"#1017#1019"{Bool, Bool, Bool})(REPL::Module)
    @ Base ./client.jl:421
 [54] #invokelatest#2
    @ ./essentials.jl:819 [inlined]
 [55] invokelatest
    @ ./essentials.jl:816 [inlined]
 [56] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
    @ Base ./client.jl:405
 [57] exec_options(opts::Base.JLOptions)
    @ Base ./client.jl:322

Project.toml Outdated Show resolved Hide resolved
@felixcremer
Copy link
Contributor Author

This works as a weak dependency with Makie 0.20.
This would still install GeoInterfaceMakie, because we can't specify dependencies only for extensions yet.
But it would only load GeoInterfaceMakie when Makie is loaded as well.
See JuliaLang/Pkg.jl#3641
Using a weak dependency would need at least julia 1.9.

src/geointerface.jl Outdated Show resolved Hide resolved
@felixcremer
Copy link
Contributor Author

I included your comments and made it as a weak dependency.
I would need a decision whether it is ok to drop support for Julia older than 1.9.
This also needs some tests.

Project.toml Show resolved Hide resolved
@felixcremer
Copy link
Contributor Author

The world age problems on Makie 0.19 stem from StableHashTraits see MakieOrg/Makie.jl#3420.

@yeesian
Copy link
Owner

yeesian commented Dec 11, 2023

Sorry for flipping on my stance from #404 (review). I wasn't aware of the full implications back then --

I would need a decision whether it is ok to drop support for Julia older than 1.9.

Julia v1.6 is still the current LTS, and so it feels a little premature for the benefit of making it marginally more convenient for users to plot with Makie -- but I could be convinced if @visr feels otherwise, or if we can find an approach that doesn't require dropping support for the current Julia LTS version.

This should enable to be able to ignore the weak dependency in Julia 1.8 or older.
@felixcremer
Copy link
Contributor Author

I now added Makie to extras in the Project.toml. This way the Makie weak dependency should be ignored in julia versions before 1.9 and will only be loaded in 1.9 or newer.
This way people on older julia versions would not get the Makie recipes but can still use the package as before.

yeesian
yeesian previously approved these changes Dec 14, 2023
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

@visr @rafaqz can you have a look again?

@yeesian yeesian changed the title Add GeoInterfaceMakie as a dependency Add GeoInterfaceMakie as a weak dependency Dec 14, 2023
Co-authored-by: Felix Cremer <[email protected]>
Project.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
module ArchGDALGeoInterfaceMakie
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module ArchGDALGeoInterfaceMakie
module ArchGDALMakieExt

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason is the extension actually triggers on Makie not GeoInterfaceMakie

ext/ArchGDALMakieExt.jl Outdated Show resolved Hide resolved
@rafaqz rafaqz enabled auto-merge (squash) December 27, 2023 17:35
@rafaqz rafaqz merged commit b878709 into yeesian:master Dec 27, 2023
12 of 13 checks passed
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