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 ChainRulesCore and DensityInterface weak dependencies #1686

Merged
merged 4 commits into from
Mar 3, 2023

Conversation

devmotion
Copy link
Member

This PR makes ChainRulesCore and DensityInterface weak dependencies on newer Julia versions that support extensions (see, e.g., https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions)).

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (0d25150) 85.81% compared to head (8ec33d1) 85.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1686   +/-   ##
=======================================
  Coverage   85.81%   85.81%           
=======================================
  Files         131      137    +6     
  Lines        8310     8311    +1     
=======================================
+ Hits         7131     7132    +1     
  Misses       1179     1179           
Impacted Files Coverage Δ
ext/DistributionsDensityInterfaceExt.jl 100.00% <ø> (ø)
src/Distributions.jl 100.00% <ø> (ø)
src/eachvariate.jl 85.71% <ø> (-4.77%) ⬇️
src/multivariate/dirichlet.jl 73.30% <ø> (-4.62%) ⬇️
src/univariate/continuous/uniform.jl 95.00% <ø> (-1.00%) ⬇️
src/univariate/discrete/negativebinomial.jl 90.74% <ø> (-2.69%) ⬇️
src/univariate/discrete/poissonbinomial.jl 93.07% <ø> (-0.63%) ⬇️
src/utils.jl 95.58% <ø> (ø)
...hainRulesCoreExt/DistributionsChainRulesCoreExt.jl 100.00% <100.00%> (ø)
ext/DistributionsChainRulesCoreExt/eachvariate.jl 100.00% <100.00%> (ø)
... and 4 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.

@devmotion
Copy link
Member Author

This PR is ready for review. The logs (https://github.com/JuliaStats/Distributions.jl/actions/runs/4272559663/jobs/7482712943) show that on Julia >= 1.9 ChainRulesCore and DensityInterface are not installed automatically anymore but the extensions are precompiled and loaded successfully once they are (implicitly) loaded in the tests.

@devmotion devmotion merged commit b9d063f into master Mar 3, 2023
@devmotion devmotion deleted the dw/weakdeps branch March 3, 2023 11:40
@oschulz
Copy link
Contributor

oschulz commented Mar 3, 2023

@devmotion saw this a bit late, but isn't making DensityInterface a weak dependency rather a complication? It has a load time of 1.4 ms ...

I had rather hoped to use more DensityInterface than less of it in the future. :-)

@devmotion
Copy link
Member Author

It's easy to make it a proper dependency again, if needed. But currently it's "just" an additional interface that we support and hence a good fit for an extension, I think - even if it is only a minor contribution to load time. As soon as you load DensityInterface in your package/script, you have access to all methods but not every user might want to work with DensityInterface (similar to how not everyone needs ChainRules support or not every user of LogExpFunctions works with InverseFunctions).

@oschulz
Copy link
Contributor

oschulz commented Mar 3, 2023

But currently it's "just" an additional interface that we support and hence a good fit for an extension

I agree, but since we we careful to make DensityInterface and InverseFunctions ultra-lightweight, what's the advantage of making them optional? I would guess that if both Distributions and DensityInterface are used then the extension package might even add another ms of load time.

I agree that we should use the new Pkg extensions extensively, but I wonder if they useful for very lightweight deps intended to be community standards.

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