Turing.jl icon indicating copy to clipboard operation
Turing.jl copied to clipboard

Enable Aqua.test_ambiguities

Open mhauru opened this issue 1 year ago • 3 comments

In #2257 I disabled method ambiguity checking, but @devmotion suggested enabling them with

@testset "Aqua" begin
    # Test ambiguities separately without Base and Core
    # Ref: https://github.com/JuliaTesting/Aqua.jl/issues/77
    Aqua.test_all(Turing; ambiguities = false)
    Aqua.test_ambiguities(Turing)
end

We should do that and see if we can fix the 6 ambiguities that it warns about. I'm happy to do this, but off next week, so will take a moment.

mhauru avatar Jun 07 '24 16:06 mhauru

For context, here are the method ambiguities: https://github.com/TuringLang/Turing.jl/actions/runs/10161073643/job/28098796083?pr=2290#step:8:305

They arise from three functions:

  1. tilde_assume; this should have been fixed in https://github.com/TuringLang/DynamicPPL.jl/pull/636 and https://github.com/TuringLang/Turing.jl/pull/2299

  2. bundle_samples; these are fixed in https://github.com/TuringLang/Turing.jl/pull/2304

  3. get:

https://github.com/TuringLang/Turing.jl/blob/07cc40beb0c6caa60e945e204f0fbc88cd3d4362/src/optimisation/Optimisation.jl#L278-L286

I ran the optimisation test suite (julia --project=. -e 'import Pkg; Pkg.test(; test_args=ARGS)' -- optim) and this method is only ever called with either Tuple{Symbol} or Vector{Symbol}.

Restricting the second argument to these would make sense as a fix for the method ambiguity, and in all likelihood would capture reasonable use cases, but would technically be a breaking change.

penelopeysm avatar Aug 13 '24 23:08 penelopeysm

Would it help if we defined something like

function Base.get(m::ModeResult, var_symbols::AbstactLens)

? Not sure what the method should do. I guess it could call get(m, tuple(var_symbols...)) in case some lens type is iterable, or alternatively just error straight away.

mhauru avatar Aug 14 '24 08:08 mhauru

Having mulled it over a bit, I think my preferred option would just be to swallow the breaking change and bump the minor version, as otherwise we will end up playing whack-a-mole with any other dependencies that might choose to define Base.get(x, y::WhateverType).

If we think it's too trivial to release 0.34 over this, we could wait until there are other breaking changes to be made and then bundle them in together. (Most mature packages do that anyway.) And use github milestones ;)

penelopeysm avatar Aug 14 '24 10:08 penelopeysm