ambiguities in included modules

[sadit]

I am trying to use Aqua in my package (SimilaritySearch). I followed the documentation for test_all and found some ambiguity errors in StatsBase. It reports and suggests some fixes. It stops the tests and reports. Everything is fine, but the possible errors are in the StatsBase package that I use, not in mine.

Is there a way to avoid checking for ambiguities in included modules? I searched the documentation and didn't find a way to skip crashes due to external packages. In particular, there is an option to avoid checking submodules; I supposed that this is not the same case.

I suppose this error will appear in other packages, but I just test with mine (I am using Julia 1.7.2, Aqua 0.5.5)

julia> using SimilaritySearch, Aqua

julia> Aqua.test_all(SimilaritySearch)
Skipping Base.active_repl
Skipping Base.active_repl_backend
Skipping Base.Filesystem.JL_O_RANDOM
Skipping Base.Filesystem.JL_O_SEQUENTIAL
Skipping Base.Filesystem.JL_O_SHORT_LIVED
Skipping Base.Filesystem.JL_O_TEMPORARY
Skipping Base.BinaryPlatforms.compiler_abi
Skipping SimilaritySearch.Performance
Skipping SimilaritySearch.VisitedVertices
Skipping SimilaritySearch.callbacks
Skipping SimilaritySearch.probe
Skipping Base.active_repl
Skipping Base.active_repl_backend
Skipping Base.Filesystem.JL_O_RANDOM
Skipping Base.Filesystem.JL_O_SEQUENTIAL
Skipping Base.Filesystem.JL_O_SHORT_LIVED
Skipping Base.Filesystem.JL_O_TEMPORARY
Skipping Base.BinaryPlatforms.compiler_abi
5 ambiguities found
Ambiguity #1
==(y::Real, x::Union{StatsBase.PValue, StatsBase.TestStat}) in StatsBase at /home/sadit/.julia/packages/StatsBase/pJqvO/src/statmodels.jl:91
==(x::AbstractIrrational, y::Real) in Base at irrationals.jl:88

Possible fix, define
  ==(::AbstractIrrational, ::Union{StatsBase.PValue, StatsBase.TestStat})

Ambiguity #2
==(x::Union{StatsBase.PValue, StatsBase.TestStat}, y::Real) in StatsBase at /home/sadit/.julia/packages/StatsBase/pJqvO/src/statmodels.jl:90
==(x::Real, y::AbstractIrrational) in Base at irrationals.jl:89

Possible fix, define
  ==(::Union{StatsBase.PValue, StatsBase.TestStat}, ::AbstractIrrational)

Ambiguity #3
StatsBase.TestStat(v) in StatsBase at /home/sadit/.julia/packages/StatsBase/pJqvO/src/statmodels.jl:80
(::Type{T})(x::AbstractChar) where T<:Union{AbstractChar, Number} in Base at char.jl:50

Possible fix, define
  StatsBase.TestStat(::AbstractChar)

Ambiguity #4
StatsBase.TestStat(v) in StatsBase at /home/sadit/.julia/packages/StatsBase/pJqvO/src/statmodels.jl:80
(::Type{T})(x::Base.TwicePrecision) where T<:Number in Base at twiceprecision.jl:255

Possible fix, define
  StatsBase.TestStat(::Base.TwicePrecision)

Ambiguity #5
StatsBase.TestStat(v) in StatsBase at /home/sadit/.julia/packages/StatsBase/pJqvO/src/statmodels.jl:80
(::Type{T})(z::Complex) where T<:Real in Base at complex.jl:44

Possible fix, define
  StatsBase.TestStat(::Complex)

Method ambiguity: Test Failed at /home/sadit/.julia/packages/Aqua/HWLbM/src/ambiguities.jl:117
  Expression: success(pipeline(cmd; stdout = stdout, stderr = stderr))
Stacktrace:
 [1] macro expansion
   @ ~/julia-1.7.2/share/julia/stdlib/v1.7/Test/src/Test.jl:445 [inlined]
 [2] _test_ambiguities(packages::Vector{Base.PkgId}; color::Nothing, exclude::Vector{Any}, detect_ambiguities_options::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Aqua ~/.julia/packages/Aqua/HWLbM/src/ambiguities.jl:117
Test Summary:    | Fail  Total
Method ambiguity |    1      1
ERROR: Some tests did not pass: 0 passed, 1 failed, 0 errored, 0 broken.

[cscherrer]

I just came here to ask about this. Ideally all upstream packages from mine could fix any ambiguities, but in practice there's a lot of overhead in trying to enforce this.

[cscherrer]

I found a hack for this. First I run with ambiguities = false, then

Aqua._test_ambiguities(
    Aqua.aspkgids(MeasureBase);
    exclude = [LogarithmicNumbers.Logarithmic]
)

In this case, LogarithmicNumbers.Logarithmic is a type from a dependency that was causing the ambiguity test to fail.

This is still not great, because,

1. It's very manual
2. Any methods I define on that type will now not be checked

[sadit]

Thanks for the info. It works.

I made some trial-and-error exploration based on it and found that the following code works.

using Aqua
Aqua.test_all(SimilaritySearch, ambiguities=false)
Aqua.test_ambiguities(SimilaritySearch)

It is a bit less manual but also could still have some issues. The first one is that, from my understanding, these calls should have identical results to the whole test

Using Aqua 0.5

[sadit]

Looking a bit more at the documentation, I think the issue comes with the default arguments to test_ambiguities. I don't know the implications

This also works

Aqua.test_ambiguities([SimilaritySearch, Core])

This fails

Aqua.test_ambiguities([SimilaritySearch, Core, Base])

[frankier]

Given how much people are extending stuff in Base, it's not surprising including Base, which seems to be the default, picks up a bunch of stuff.

[vtjnash]

It is usually a waste of performance time also to include Core and Base explicitly now, since they will exclusively add ambiguities that are not from the package itself (which is already listed)

[lucaferranti]

fwiw a few data points I encountered today:

• using Aqua 0.6 and julia 1.8.4, this issue does not seem to happen.
• I did encounter this however with julia 1.6 and Aqua 0.6. I haven't tested with 1.7, but based on the OP message, I would presume it persists there too

[fingolfin]

In addition to all that was said, I'd like to point out that test_ambiguities has a bunch of options that can be passed to it. E.g. one can use that to turn off recursive processing of sub-modules

Aqua.test_all(
    Aqua;
    ambiguities = (; recursive = false),
)

That said, the problem seems to be caused by Base (and Core) being in the default list of modules. @vtjnash suggests that's not so useful anyway. I don't know why it is being done, as I am not the original author of this package. I'd be happy to remove this, but the ramifications are not quite clear to me, I am afraid. A comment explicitly mentions this, though:

    * `test_all` runs [`test_ambiguities`](@ref) with `Core`.  This
       means method ambiguities of constructors can now be detected.
       In Aqua.jl 0.4, `test_ambiguities` was invoked with
       `[testtarget, Base]`.

[vtjnash]

that's not so useful anyway

It is actually actively counter-productive now (causing this issue), but it used to be required for the test to function at all.

[fingolfin]

@vtjnash OK... So, what changed that made it go from "used to be required for the test to function at all" to the current state? Were those changes in Aqua.jl? Or changes in Julia? Or something else?

[IanButterworth]

What's the current recommendation for testing for ambiguities in my package only?

This?

using Aqua
Aqua.test_all(MyPackage; ambiguities = (; recursive = false))

[lgoettgens]

There unfortunately currently is no way to test only your package for ambiguities. (as far as I know)

[lgoettgens]

For more details, I would ask @vtjnash to elaborate a bit.

[sstroemer]

I just stumpled on this issue trying to add Aqua to a package I'm developing, that makes use of JuMP.jl. I do not depend on (or make use of) ForwardDiff, but it's a dep brought in by MathOptInterface (a dep of JuMP). When running

@testset "Aqua.jl" begin
    Aqua.test_all(MyPackage; ambiguities=(exclude=[], broken=false, recursive=false))
end

it fails with ambiguities like

Ambiguity #1
==(x::Real, y::ForwardDiff.Dual{Ty}) where Ty @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:145
==(x::AbstractIrrational, y::Real) @ Base irrationals.jl:89

even though I set recursive=false.

Is there any way to ignore ambiguities from specific modules (similar problems occur for SentinelArrays and InlineStrings)?

---

I'm currently using

@testset "Aqua.jl" begin
    Aqua.test_ambiguities(MyPackage)
end

which works as a workaround, but I'd much rather manually exclude certain modules that I know of to be failing as soon as Base and Core are introduced to the list of modules, instead of silently missing potential problems when a new dependency is introduced to the project?

[ajwheeler]

Hi all, apologies if I'm missing a larger discussion elsewhere, but is there a case for changing the default behavior of test_all, or at least making it easier to not include Base and Core? It seems to me that most medium to large packages will run into this issue and it's not an ideal first experience of Aqua if it is primarily pointing out things you can't address.

[3f6a]

My understanding is that this issue refers to all dependencies , not just Core or Base.
That is, I would prefer Aqua to focus on reporting issues only the current package, not any of its dependencies.
In that case, can this be reopened? @lgoettgens

[lgoettgens]

As of my understanding, this should be already the case. If not, could you provide an example?