ChainRulesCore.jl icon indicating copy to clipboard operation
ChainRulesCore.jl copied to clipboard

How to @opt_out of rules for methods in packages that don't use CRC

Open willtebbutt opened this issue 4 years ago • 5 comments

Problem Statement

PDMats.jl doesn't utilise CRC, however, it has a decent chunk of code that needs to be opted out of. For example, this diag rule probably isn't what you want in order to differentiate this method of diag. Rather you should just differentiate through the method.

We're starting to use PDMats.jl in the JuliaGPs ecosystem and I realised that I have no idea how to opt out of stuff when we inevitably hit something we need to opt out of without convincing the maintainers of PDMats.jl to use CRC, and to write a load of additional code opting out.

An example of this kind of problem came up in Zygote recently: https://github.com/FluxML/Zygote.jl/issues/1106

Question

Do we have any options other than

  1. convincing PDMats maintainers to care about CRC, or
  2. putting opt-out code in the JuliaGPs ecosystem?

To my mind, neither of the above options are good. 2 is quite clearly bad (type piracy), and 1 also feels bad because we would just be adding the CRC dep in order to opt out of stuff (if they had written code which wasn't trivially differentiable, it would be a different matter).

edit: typo

willtebbutt avatar Oct 28 '21 14:10 willtebbutt

Just do 1. CRC is light-weight. We put a lot of effort into cutting it down to be a lightweight interface.

You can't really avoid that in julia. You need to depend on packages. At least until we fix https://github.com/JuliaLang/Pkg.jl/issues/1285


The ship has sailed for CR v1.0 in terms of we decided that general rules and then more specific opting-out was the thing we would do. It was a hard-choice, and we are not revisiting it here.

oxinabox avatar Oct 28 '21 15:10 oxinabox

It was a hard-choice, and we are not revisiting it here.

I'm not suggesting to revisit it here, I was just trying to see if I've missed an option.

Just do 1.

I have no idea if that's feasible. I don't own PDMats.

willtebbutt avatar Oct 28 '21 16:10 willtebbutt

Was going to reference Distributions / DistributionsAD as another model. But I see that the basic package now depends on CRC https://github.com/JuliaStats/Distributions.jl/pull/1390, moving toward option 1.

mcabbott avatar Oct 28 '21 16:10 mcabbott

Yeah, I think DistributionsAD is actually a good example for why ideally this should be part of PDMats but not be defined in a separate package. Hopefully DistributionsAD will be gone one day. It was useful and still is but it's just not feasible in the long run to commit so much type piracy and depend on so many internal things of other packages. It is broken all the time.

devmotion avatar Oct 28 '21 18:10 devmotion

I think I can review (and probably merge) a PR in PDMats if you want to prepare one @willtebbutt.

devmotion avatar Oct 28 '21 18:10 devmotion