-
Notifications
You must be signed in to change notification settings - Fork 62
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
Remove major invalidations from == #524
Conversation
Codecov Report
@@ Coverage Diff @@
## main #524 +/- ##
==========================================
- Coverage 93.03% 93.02% -0.02%
==========================================
Files 15 15
Lines 862 860 -2
==========================================
- Hits 802 800 -2
Misses 60 60
Continue to review full report at Codecov.
|
This sounds great. Are we at the stage with these tools where they can be added to the tests, to flag something which would introduce major issues? (Also, maybe we should add CRTestUtils to the downstream tests of this package?) |
This is probably not going to break anything real, even the tests should be using the function from CRTU. Tim Holy and I did look at this at JuliaCon, and we concluded then that these invalidations were few and were low impact because of what was hit. Before we merge this can we
Then:
|
bump |
See #523 . Seems to only be used for tests according to the PR but it's a major compile time problem, so it doesn't make sense to keep them. This can be marked breaking if it breaks downstream, but it's rather easy to update downstream AD packages for this so the value proposition is pretty clear.
1b91cb3
to
4807f40
Compare
Look, doing this benchmark to really show the timing isn't something anyone has invested time to do in the last 4 months, but pretty much every compile time study flags this as a problem. So at this point, I think we should checkmark it at least as something to fix an easy false-positive if it did contribute nothing to times. Locally, ChainRules tests ended up failing for an odd reason which I couldn't actually recreate in the REPL using Test, ChainRulesCore, ChainRulesTestUtils
using ChainRules
using FiniteDifferences
using LinearAlgebra
using LinearAlgebra.BLAS
using LinearAlgebra: dot
using Random
using SparseArrays
using StaticArrays
using Statistics
using Test
T = Float64
test_frule(identity, randn(T))
test_frule(identity, randn(T, 4))
test_frule(identity, Tuple(randn(T, 3)))
test_rrule(identity, randn(T))
test_rrule(identity, randn(T, 4))
test_rrule(identity, Tuple(randn(T, 3)))
T = ComplexF64
test_frule(identity, randn(T))
test_frule(identity, randn(T, 4))
test_frule(identity, Tuple(randn(T, 3)))
test_rrule(identity, randn(T))
test_rrule(identity, randn(T, 4))
test_rrule(identity, Tuple(randn(T, 3))) So @oxinabox I think this is ready to just rip off the bandaid. What's going on in the ChainRules tests can be figured out separately, but I don't think that should block this. |
Downstream ErrorIt is definitely this PR causing ChainRules.jl to break. I don't think we can merge this, and thus lose all ability to run CI on ChainRules.jl til we debug that. The error is: MethodError: test_approx(::Tangent{Tuple{Float64, Float64, Float64}, Tuple{Float64, Float64, Float64}}, ::Thunk{ChainRulesTestUtils.var"#53#57"{Tangent{Tuple{Float64, Float64, Float64}, Tuple{Float64, Float64, Float64}}}}, ::String) is ambiguous. Candidates:
test_approx(actual, expected::AbstractThunk, msg; kwargs...) in ChainRulesTestUtils at /home/oxinabox/.julia/packages/ChainRulesTestUtils/fCvaU/src/check_result.jl:28
test_approx(actual::Tangent{P, T}, expected, msg; kwargs...) where {T, P} in ChainRulesTestUtils at /home/oxinabox/.julia/packages/ChainRulesTestUtils/fCvaU/src/check_result.jl:108
Possible fix, define
test_approx(::Tangent{P, T}, ::AbstractThunk, ::Any) where {T, P} This is I think caused by this change in this PR. test_approx(actual::Tangent, expected::AbstractThunk, msg::Any) = test_approx(actual, unthunk(expected), msg)- Which would unblock this PR. benchmarks:Benchmarking this took me like a minute of actual work and a couple of minutes of waiting for DifferentialEquations to finish precompiling. Special FunctionsCRC@1.13.0julia> @time using SpecialFunctions;
0.376541 seconds (718.99 k allocations: 44.210 MiB, 80.86% compilation time) CRC#invalidationsjulia> @time using SpecialFunctions;
0.366335 seconds (719.00 k allocations: 44.213 MiB, 2.08% gc time, 77.52% compilation time) DifferentialEquationsCRC@1.13.0julia> @time using DifferentialEquations
7.408940 seconds (19.70 M allocations: 1.483 GiB, 3.67% gc time, 14.54% compilation time) CRC#invalidationsjulia> @time using DifferentialEquations
5.513351 seconds (13.71 M allocations: 1.117 GiB, 5.45% gc time, 9.62% compilation time) I have about 30 minutes left to me today, so I will see about making the change to unblock this in CRTU |
Diffractor failure is on master #552 (comment) and unrelated. Merging. |
See #523 . Seems to only be used for tests according to the PR but it's a major compile time problem, so it doesn't make sense to keep them. This can be marked breaking if it breaks downstream, but it's rather easy to update downstream AD packages for this so the value proposition is pretty clear.