Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Breaking ChainRules 1.35.3 release #216

Closed
rofinn opened this issue Jun 20, 2022 · 1 comment · Fixed by #218 · May be fixed by #217
Closed

Breaking ChainRules 1.35.3 release #216

rofinn opened this issue Jun 20, 2022 · 1 comment · Fixed by #218 · May be fixed by #217

Comments

@rofinn
Copy link
Member

rofinn commented Jun 20, 2022

Looks like how cholesky factors are being handled by ChainRules is breaking our CI now.

https://github.com/invenia/Nabla.jl/runs/6959631070?check_suite_focus=true#step:6:390

Bisect results:

Commit Msg Error
c52f7bc Check that check kwarg correctly passed Failing with MethodError: no method matching cholesky(::Branch{Matrix{Float64}, Nothing})
08b0d31 Generalize diagonal cholesky to Hermitian Failing with MethodError: no method matching UpperTriangular(::ZeroTangent)
ebe03e0 Create tangent with factors Same error, but only for last two tests in test/sensitivities/linalg/factorization/cholesky.jl
cce75a0 Work with factors directly Exactly the same behaviour as before
b543b0e Rewrite getproperty rule to store factors Same errors above, but with some mul! method errors

So far I can kinda follow the ZeroTangent errors from JuliaDiff/ChainRules.jl#630, but I still need to run another bisect to figure out when we started getting the Branch method errors?
This was referenced Jun 22, 2022
Open
Open
@oxinabox
Copy link
Member

I talked to Rory about this last week in person.
Concluded that we should put in the [compat] section:
ChainRules="=1.35.2"
and just pin it to an exactly specified known working version.

Nabla is fragile to ChainRules version changes in subtle ways.
So any changes should be done with care.

As we are not using this in prod (still using the old version of Nabla)
this version of Nabla is more a proof of concept of ChainRules integration than actually needing to be fully robust.
If we did move it into production we should think about how to robustify it more.
(which might be just solving a lot of these case)

@rofinn rofinn mentioned this issue Sep 13, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@oxinabox @rofinn