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

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

Open
willtebbutt opened this issue Oct 28, 2021 · 5 comments
Open

Comments

@willtebbutt
Copy link
Member

willtebbutt commented Oct 28, 2021

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: FluxML/Zygote.jl#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

@oxinabox
Copy link
Member

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 JuliaLang/Pkg.jl#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.

@willtebbutt
Copy link
Member Author

willtebbutt commented Oct 28, 2021

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.

@mcabbott
Copy link
Member

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

@devmotion
Copy link
Member

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
Copy link
Member

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

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

No branches or pull requests

4 participants