Refactor CI #406
Refactor CI #406
Conversation
|
Bijectors.jl documentation for PR #406 is available at: |
|
Well look at that those green ticks, not had that for a while on Bijectors. |
There was a problem hiding this comment.
Subsumed into CI.yml.
docs/make.jl
Outdated
| # TODO: Remove this | ||
| warnonly=true, |
There was a problem hiding this comment.
This is a replacement for strict=false which no longer works with Documenter.jl v1.0. It converts all docs errors into warnings. This is not good, but I didn't want fix this in this PR as well, it should be a separate PR.
There was a problem hiding this comment.
how difficult a fix is it? As much as possible I think we should avoid merging broken things to main, it feels distinctly wrong unless it's an urgent fix.
There was a problem hiding this comment.
#412 fixed this so I can actually remove it!
| test_frule(Bijectors.find_alpha, x, y, z) | ||
| test_rrule(Bijectors.find_alpha, x, y, z) | ||
|
|
||
| if @isdefined Mooncake |
There was a problem hiding this comment.
moved to test/ad/mooncake.jl
| @test _xs == xs | ||
| end | ||
|
|
||
| function test_bijector_parameter_gradient(b::Bijectors.Transform, x, y=b(x)) |
There was a problem hiding this comment.
this function was not being used anywhere so can be deleted
There was a problem hiding this comment.
this file is no longer needed, the test_ad function is directly defined in test/runtests.jl
docs/make.jl
Outdated
| # TODO: Remove this | ||
| warnonly=true, |
There was a problem hiding this comment.
how difficult a fix is it? As much as possible I think we should avoid merging broken things to main, it feels distinctly wrong unless it's an urgent fix.
There was a problem hiding this comment.
Is there a better way to do this? The whole enzyme stuff seems very messy still.
There was a problem hiding this comment.
Could you explain what you mean by messy / what could be done to make it less so? I think it seems as clean as it can be already.
|
|
||
| x = rand(dist) | ||
| y = b(x) | ||
| @testset "VecCholeskyBijector: $backend_name" for (backend_name, adtype) in TEST_ADTYPES |
There was a problem hiding this comment.
Could you explain TEST_ADTYPES array to me in more detail? Is this the best way to do things with this fixed array?
There was a problem hiding this comment.
Pretty much. The idea is that Bijectors provides these functions f and you want to make sure that it can be differentiated with all adtypes of interest. The same sort of loop is used elsewhere see e.g.
There was a problem hiding this comment.
... and each adtype corresponds to an AD backend of interest (plus some configuration options e.g. whether to use forward- or reverse-mode).
test/ad/flows.jl
Outdated
| end | ||
| test_ad(randn(11)) do θ | ||
| if ENZYME_FWD_AND_1p11 | ||
| @test_throws Enzyme.Compiler.EnzymeInternalError test_ad(f, adtype, randn(7)) |
There was a problem hiding this comment.
Not sure I'm linking right, should the finv and ginv be used or is this correctly referenced for what you want?
|
These Enzyme tests are so frustratingly flaky, I had marked all the failures but between the time I made this PR and now there are different failures popping up. |
This PR reworks most of Bijectors' CI setup to be more manageable.
Use DifferentiationInterface instead of directly calling
ForwardDiff.gradient, etc.Test all AD backends in a single CI job. The tests don't take very long at all so this is not a problem in terms of time. It also allows us to restructure the
test_adfunction so that we can more surgically disable / enable tests. The previous one was very awkward to work with.Disable / enable failing Enzyme tests as needed to make CI pass without the use of
continue-on-error. Where possible we use@test_throwsinstead of just skipping the test as that will let us know if and when a test begins to workSeparate doctests into a standalone CI job that only runs on 1.11. Doctest output can depend on Julia version so it can be very fragile running doctests on multiple different CI setups.
Cache the Julia environment in CI. Saves time.
Add a couple of CI runners on Windows.
Closes #373
Closes #347 (not directly, but I think the intent behind that was really 'clean up the Enzyme tests', and this PR does so)