Skip to content
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

Enable Aqua.test_ambiguities #2261

Closed
mhauru opened this issue Jun 7, 2024 · 3 comments · Fixed by #2304
Closed

Enable Aqua.test_ambiguities #2261

mhauru opened this issue Jun 7, 2024 · 3 comments · Fixed by #2304
Assignees

Comments

@mhauru
Copy link
Member

mhauru commented Jun 7, 2024

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.

@penelopeysm
Copy link
Member

penelopeysm commented Aug 13, 2024

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 Add AbstractRNG types to fix method ambiguities in Turing DynamicPPL.jl#636 and Resolve ADTypeCheckContext method ambiguity #2299

  2. bundle_samples; these are fixed in Fix remaining method ambiguities #2304

  3. get:

"""
Base.get(m::ModeResult, var_symbol::Symbol)
Base.get(m::ModeResult, var_symbols)
Return the values of all the variables with the symbol(s) `var_symbol` in the mode result
`m`. The return value is a `NamedTuple` with `var_symbols` as the key(s). The second
argument should be either a `Symbol` or an iterator of `Symbol`s.
"""
function Base.get(m::ModeResult, var_symbols)

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.

@mhauru
Copy link
Member Author

mhauru commented Aug 14, 2024

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.

@penelopeysm
Copy link
Member

penelopeysm commented Aug 14, 2024

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 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants