Add compat for ChainRulesCore v2#248
Conversation
|
The BlockDiagonals and SpecialFunctions tests should be fixed by these PRs:
The amount of ChainRules failures is a bit alarming 😬 Nightly failures are the same kind of inference failure that's on 1.12. Is that worth a Julia issue? |
JamesWrigley
force-pushed
the
compat
branch
2 times, most recently
from
de58f3a to
04c455a
Compare
This avoids making ChainRulesCore a dependency of the downstream package, which can cause Aqua failures.
|
I also added the cache action in 2c6181c. |
| test_to_vec((a=5, b=randn(10, 11), c=(5, 4, 3)); check_inferred = VERSION ≥ v"1.2") | ||
| else | ||
| test_to_vec((a=3 + 2im, b=randn(T, 10, 11), c=(5+im, 2-im, 1+im)); check_inferred = VERSION ≥ v"1.2") | ||
| test_to_vec((a=3 + 2im, b=randn(T, 10, 11), c=(5+im, 2-im, 1+im)); check_inferred = v"1.2" <= VERSION < v"1.12") |
There was a problem hiding this comment.
Is it clear why this is broken?
There was a problem hiding this comment.
I ran it through Claude and it said that it was something to do with struggling to infer the return of reduce(vcat) with heterogeneous tuples. But I'm not familiar enough with the code to say if that's correct 🤷
There was a problem hiding this comment.
The issue should either be fixed or marked as broken. Just disabling failing tests is not a proper solution.
There was a problem hiding this comment.
This (or rather the integration test fixes) should be done in a separate PR.
There was a problem hiding this comment.
Why? 🤔 They're in a separate commit.
There was a problem hiding this comment.
Because the CI changes don't deserve a new release, and ChainRulesCore@2 is not even released and can't be tested. The CI changes are useful right away.
There was a problem hiding this comment.
That's why I tested it manually by deving v2 :) How then do you want to handle JuliaDiff/ChainRulesCore.jl#703? Temporarily dev FiniteDifferences/StaticArrays/ChainRulesTestUtils in the ChainRulesCore tests? Otherwise CI won't pass.
There was a problem hiding this comment.
It's fine to prepare PRs but they can't be merged until a new release is available. CI fixes should not wait for it.
Generally, to test unreleased versions/branches in CI, you should use the sources feature of Pkg.
There was a problem hiding this comment.
Ok 👍 If it's alright with you I'll keep everything in these PRs for now including the version bumps, otherwise I'll have to do another round of PRs later. I'll tweak the ChainRulesCore CI to use these branches for now.
There was a problem hiding this comment.
Also sources was only added in 1.11 so 1.10 CI will fail. Is that ok?
There was a problem hiding this comment.
I still don't see a reason for adding unrelated changes that master would benefit from right now to this PR with unclear time horizon and without proper CI test of the remaining changes.
|
I think there are two main outstanding issues here:
|
Why exactly? There's a lot of PRs I'm having to keep track of and it's extra work for me.
I would suggest doing that in another PR. |
2 participants
See JuliaDiff/ChainRulesCore.jl#703. Tested manually by
deving v2 and JuliaArrays/StaticArrays.jl#1339.The
to_vec()inference test is failing on 1.12 so I disabled it.