Add rules for LogExpFunctions

[devmotion]

This PR adds the rules for the functions in LogExpFunctions, as defined in https://github.com/JuliaStats/LogExpFunctions.jl/blob/master/src/chainrules.jl.

The motivation for adding them here is that recent versions of SpecialFunctions depend on LogExpFunctions. Hence LogExpFunctions is already an indirect dependency of DiffRules. Moreover, if AD backends such as ForwardDiff, Tracker, and ReverseDiff are fixed (see below) this PR fixes remaining test issues for these backends in DistributionsAD (see TuringLang/DistributionsAD.jl#203).

While this PR is not breaking technically, it seems it breaks almost all downstream dependencies in practice. The reason is that it seems to be common practice to iterate over DiffRules.diffrules() and define differentiation rules with @eval without checking that the module where the function is defined is available in the current module. I.e., they are all missing a check such as M in (:Base, :SpecialFunctions, :NaNMath) to ensure that they don't try to define rules for unsupported packages. I guess this means that we should update the version number to DIffRules 2.0.0 to make sure that we don't break downstream packages. When upgrading to DiffRules 2.0.0, ideally the packages fix this bug.

Additionally, one could provide something like

macro diffrules() = :(diffrules(__module__))

function diffrules(mod::Module)
    return filter(diffrules()) do (M, _, _)
        return isdefined(mod, M)
    end
end

to make it more convenient for packages to load rules for all modules that are loaded in the invoking module.

[devmotion]

Maybe I missed it but it seems there is no way to reuse the result of the primal computation in DiffRules?

[devmotion]

I added the let blocks to fix some local/global scope warnings.

[codecov-commenter]

Codecov Report

Merging #69 (69cad0a) into master (2db3a81) will increase coverage by 0.91%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage   93.75%   94.66%   +0.91%     
==========================================
  Files           2        2              
  Lines         128      150      +22     
==========================================
+ Hits          120      142      +22     
  Misses          8        8              

Impacted Files	Coverage Δ
src/api.jl	65.00% <100.00%> (+35.00%)	⬆️
src/rules.jl	99.23% <100.00%> (+0.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2db3a81...69cad0a. Read the comment docs.

[andreasnoack]

@DilumAluthge Is the IntegrationTest action unable to handle new dependencies?

[DilumAluthge]

Hmmm. I'm not very familiar with that particular job. Who wrote the original workflow file?

Maybe @ChrisRackauckas will know.

[devmotion]

No, it works just fine. It correctly shows that this PR breaks all downstream packages. As explained above, the problem is not that this change is technically breaking but that downstream packages don't use the output of diffrules() carefully enough. If I bump the version to 2.0.0 all integration tests will pass since no downstream package is compatible with this breaking release.

[andreasnoack]

I somehow misunderstood your original comment even though the issue is pretty well explained there. What about updating all the downstream packages before merging this?

[mcabbott]

Another option might be to let the caller specify which modules to cover, and set a default which matches the old behaviour: diffrules(; modules=[:Base, :NaNMath, :SpecialFunctions])

[devmotion]

I like this suggestion, I'll update the PR (I'll use a tuple as default value to save some allocations though I think).

[devmotion]

The test failure with nightly is unrelated and caused by an instability of the finite differencing method for the randomly chosen values (due to the RNG changes and the possibly different order of the rules they are not stable across different Julia versions). Maybe in a separate PR one could switch to more advanced methods eg by using FiniteDifferences.

[devmotion]

Bump 🙂

I changed this PR according to the suggestions such that it is not breaking anymore but only a deprecation warning is shown that in the next breaking release the behaviour will change and not only rules for Base, SpecialFunctions, and NaNMath but all available rules will be returned by diffrules().

[devmotion]

Bump 🙂

[andreasnoack]

Is it intended that all the workflow changes are part of this PR? I'd think that they belong in a separate PR. (Btw I don't think we should be using the git based registry).

[devmotion]

Is it intended that all the workflow changes are part of this PR?

Partly. I updated the Julia version in the docs action, updated docs/make.jl, and added an action that cleans the docs preview to ensure that the documentation is built correctly and doctests pass.

Additionally, I added the "cancel actions for old commits" feature on purpose since otherwise GH actions would be blocked by the longer running integration tests.

I'll move the TagBot + CompatHelper updates to a separate PR though, they are unrelated.

[devmotion]

I moved the CI updates to #70.

[devmotion]

@andreasnoack I forgot to remove these lines in #70, seems I copied the updated file incorrectly. schedule: is not needed anymore, so removing this trigger will reduce the amount of runs.

All other CI changes were addressed in #70.

[odow]

What about updating all the downstream packages before merging this?

This would have been my preference. Arguably the downstream packages are mis-using the API. For packages that are this deep in the dependency tree, we should try to avoid introducing deprecation warnings that users see (we don't want JuMP users seeing deprecation warnings in production or during their tests that they can't do anything about).

Another option would have been to make accessing all the rules opt-in so diffrules(all = true), and the default, diffrules(all = false) would have kept the current behavior.

[devmotion]

we should try to avoid introducing deprecation warnings that users see (we don't want JuMP users seeing deprecation warnings in production or during their tests that they can't do anything about).

Users don't see deprecation warnings since they are not enabled by default. And arguably in tests we want downstream packages to see deprecation warnings - they don't have to fix them immediately but they are notified that the API will change in the future and have time to prepare a fix.

[odow]

since they are not enabled by default.

Only in more recent versions of Julia. JuMP still supports the LTS of Julia 1.0.

in tests we want downstream packages to see deprecation warnings

We want direct dependencies to see them. But if there are lots of transitive dependencies, there are issues.

DiffRules has 17 direct dependents that might need updating (depending if they called this properly), but 853 indirect dependencies:

Most of those are due to ForwardDiff, which does cause the deprecation warning to be thrown:

[devmotion]

I can understand that you would like that deprecation warnings don't show for users, and apparently also in tests. However, I think the whole point of deprecation warnings is to clearly mark the API as deprecated and notify packages and users about this future change such that they can react accordingly (if they plan to support a future breaking release). In my opinion, deprecation warnings are no errors that have to be fixed immediately since they are no errors but warnings or notifications. It would basically be impossible for packages with many, possibly indirect, dependencies to mark something as deprecated if one had to fix all downstream packages first. And in my opinion this would also invalidate the reason for deprecation warnings. I don't think it can be expected from contributors to fix the use of diffrules in all downstream packages such as ForwardDiff, ReverseDiff, or Tracker if there is a clear way to deprecate the current (incorrect) use. (Technically, it would have been even OK to not introduce any deprecation warnings at all since the problem is mainly an incorrect use of diffrules in these downstream packages - but this would have been extremely breaking and disruptive for the whole AD ecosystem.)

[devmotion]

Only in more recent versions of Julia. JuMP still supports the LTS of Julia 1.0.

I didn't know it was only introduced in more recent versions. Of course, you can always disable them manually with --depwarn=no.

[mcabbott]

I think I missed this at the time, but why must this add a dependence on LogExpFunctions? And hence on 6 other packages.

I mean a test-time dep is fine, and a compat entry in Project.toml is fine (like SpecialFunctions) but why actually load it?

[devmotion]

Not all functions are defined in all versions of LogExpFunctions, and it is not possible to check for their existence in any other way than with idefined(LogExpFunctions, ...).

It does not affect dependencies of DiffRule since SpecialFunctions depends on LogExpFunctions and hence it's an indirect dependency anyway. Moreover, it is very lightweight and all its dependencies are very lightweight (only interfaces such as ChainRulesCore, InverseFunctions, and ChangesOfVariables). It was extracted from StatsFuns to make it so lightweight (in particular without Rmath) that it can be added to any package without major impacts on package load times and deoendencies.

[mcabbott]

and it is not possible to check for their existence in any other way than

But if LogExpFunctions follows semver, then surely we can just bound the version. Removing them is a breaking change. This is exactly how SpecialFunctions was handled.

since SpecialFunctions depends on LogExpFunctions

But this package doesn't depend on SpecialFunctions, except to bound its version. That's the pattern I'm suggesting could be copied.

This package did and (I think) still can have zero dependencies. That's the best situation, and I think we ought to have a solid reason to move away from it. The costs of doing so aren't huge, but they aren't nothing either. It seems brittle to have every little package depend on every other one, e.g. the fairly recent case where some change broke Static.jl and about a thousand downstream packages.