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

Make UnicodePlots a weak dependency (for julia > 1.9) #51

Merged
merged 6 commits into from
Feb 27, 2023

Conversation

ablaom
Copy link
Member

@ablaom ablaom commented Feb 23, 2023

Closes #49

There is an issue with @time_imports on Julia 1.9, but here is a summary benchark (no pre-compilation) for Julia Version 1.9.0-beta4:

# before this PR:
julia> @time using CategoricalDistributions
  4.774755 seconds (9.20 M allocations: 591.403 MiB, 4.50% gc time, 3.52% compilation time: 87% of which was recompilation)
# after this PR
julia> @time using CategoricalDistributions
  1.684570 seconds (2.62 M allocations: 168.023 MiB, 3.60% gc time, 9.73% compilation time: 86% of which was recompilation)

This PR will not impact load times for julia < 1.9.

I'd prefer not to add a temporary Requires.jl hack to address this.

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #51 (d75996e) into dev (5c62c4e) will increase coverage by 3.38%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev      #51      +/-   ##
==========================================
+ Coverage   92.23%   95.62%   +3.38%     
==========================================
  Files           7        8       +1     
  Lines         412      411       -1     
==========================================
+ Hits          380      393      +13     
+ Misses         32       18      -14     
Impacted Files Coverage Δ
src/CategoricalDistributions.jl 100.00% <ø> (ø)
src/methods.jl 90.00% <ø> (+9.08%) ⬆️
ext/UnivariateFiniteDisplayExt.jl 100.00% <100.00%> (ø)
src/types.jl 97.24% <0.00%> (+0.88%) ⬆️

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

@ablaom
Copy link
Member Author

ablaom commented Feb 24, 2023

@andreasnoack Would you be willing to review this?

@henry2004y
Copy link

henry2004y commented Feb 24, 2023

Hi, I have a question with the new extension workflow. In a fresh Julia v1.9-beta4, I git clone the UnicodePlots-as-weakdep branch on my laptop, and then ran the tests inside the directory via

julia> ]
(v1.9) pkg> activate .
(CategoricalDistributions) pkg> test
...
┌ Warning: 13 method ambiguities detected
└ @ Main /home/local/hongyang/tmp/test/runtests.jl:15
Test Summary: | Pass  Total  Time
utilities     |   30     30  8.3s
Test Summary: | Pass  Total   Time
types         |   23     23  20.7s
Test Summary: | Pass  Total  Time
scitypes      |    7      7  0.2s
Test Summary: | Pass  Total   Time
methods       |  115    115  14.1s
Test Summary: | Pass  Total   Time
arrays.jl     |  115    115  16.4s
Test Summary: | Pass  Total   Time
arithmetic.jl |   18     18  11.0s
     Testing CategoricalDistributions tests passed 

Then I went back to the main REPL,

(CategoricalDistributions) pkg> ^C

julia> using CategoricalDistributions

julia> using CategoricalArrays

julia> import Distributions

julia> import UnicodePlots
ERROR: ArgumentError: Package UnicodePlots [b8865327-cd53-5732-bb35-84acbb429228] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.

Stacktrace:
 [1] _require(pkg::Base.PkgId, env::String)
   @ Base ./loading.jl:1724
 [2] _require_prelocked(uuidkey::Base.PkgId, env::String)
   @ Base ./loading.jl:1610
 [3] macro expansion
   @ ./loading.jl:1598 [inlined]
 [4] macro expansion
   @ ./lock.jl:267 [inlined]
 [5] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1561

Note that I have UnicodePlots installed under the main environment, but not in the cloned CategoricalDistributions environment:

(v1.9) pkg> st
Status `/home/local/hongyang/.julia/environments/v1.9/Project.toml`
  [6e4b80f9] BenchmarkTools v1.3.2
  [32113eaa] PkgBenchmark v0.2.12
  [91a5bcdd] Plots v1.38.5
  [08abe8d2] PrettyTables v2.2.2
  [d330b81b] PyPlot v2.11.0
  [aa65fe97] SnoopCompile v2.10.2
  [e2b509da] SnoopCompileCore v2.10.0
  [b8865327] UnicodePlots v3.4.0

This workflow used to work on earlier Julia versions. Btw, with extensions UnicodePlots is no longer in the Manifest file. How should one use/develop a package with extensions in Julia 1.9+?

@andreasnoack
Copy link

I guess it makes sense. Now that UnicodePlots is an extension, it's not available from the CategrocialDistributions environment. You'd have to use an environment where UnicodePlots is explicitly made available to use the extension. I'm not completely sure, though. Maybe @KristofferC can confirm.

@KristofferC
Copy link

. How should one use/develop a package with extensions in Julia 1.9+?

You can create a new environment where you dev CategoricalDisrtibutions and add UnicodePlots. Or you can temporarily make UnicodePlots a normal dependency. Maybe there should be some global setting you could enable that would treat weak dependencies as normal dependencies.


Regarding this PR, it is a little bit unorthodox use of extensions. Typically, you would add an extension in order to specialize a function in the parent package on a type in one of the weak dependencies. So, as an example, if you have a package Determinants.jl, you can add an extension to StaticArrays that computes the determinant quickly for a StaticArray by defining det(::StaticArray) = ... in the extension.

What this PR does is something slightly different. If any package in the environment happens to load UnicodePlots, then the default show method is changed. So it means that the behavior you expose will change depending on what dependencies other packages in the environment has which is typically undesirable. For extensions, for things to not be considered type piracy, one of the arguments should be owned by one of the weak dependencies. However, there is no real "harm" in this so you can try it out and see how it works in practice.

@ablaom
Copy link
Member Author

ablaom commented Feb 25, 2023

@KristofferC Thanks for chipping in here!

However, there is no real "harm" in this so you can try it out and see how it works in practice.

I agree. If some harm comes we can always just dump the dependency altogether.

@ablaom ablaom closed this Feb 25, 2023
@ablaom ablaom reopened this Feb 25, 2023
Copy link
Member

@OkonSamuel OkonSamuel left a comment

Choose a reason for hiding this comment

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

LGTM!!!.

@ablaom ablaom merged commit 92afbd1 into dev Feb 27, 2023
@ablaom ablaom mentioned this pull request Feb 27, 2023
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.

Make UnicodePlots an optional dependency
6 participants