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

ml-matches: update and improve ambiguity computation #36962

Merged
merged 2 commits into from
Sep 2, 2020
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 7, 2020

This uses the much-enhanced ambiguity computing algorithm in ml-matches, and implements this all better. The failures here are actually all type-intersection problems (#36951), as far as I can tell.

@vtjnash vtjnash requested a review from JeffBezanson August 7, 2020 20:05
@vtjnash vtjnash force-pushed the jn/new_ambig branch 2 times, most recently from e556167 to 7e55d58 Compare August 24, 2020 20:36
ambig = Set{Any}(((m1.sig, m2.sig) for (m1, m2) in ambig))
expect = []

Sys.iswindows() || push!(expect, (Tuple{typeof(\),Factorization{T},Union{Array{Complex{T},1}, Array{Complex{T},2}}} where T<:Union{Float32, Float64}, Tuple{typeof(\),LQ{TA,S} where S<:AbstractMatrix{TA},StridedVecOrMat{TB}} where TB where TA))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one seems possibly more real though, as the intersection can be either:

julia> t = typeintersect(Tuple{typeof(\),Factorization{T},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}, Tuple{typeof(\),LQ{TA,S} where S<:AbstractMatrix{TA},StridedVecOrMat{TB}} where TB where TA)
Tuple{typeof(\),
LQ{T,S} where S<:AbstractMatrix{T},
Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}

or

julia> t = typeintersect(Tuple{typeof(\),LQ{TA,S} where S<:AbstractMatrix{TA},StridedVecOrMat{TB}} where TB where TA, Tuple{typeof(\),Factorization{T},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64})
Tuple{typeof(\),
Union{LQ{TA,S} where S<:AbstractMatrix{TA}, LQ{TA1,S} where S<:AbstractMatrix{TA1}},
Union{Array{Complex{TA},1}, Array{Complex{TA1},2}}} where TA1<:Union{Float32, Float64} where TA<:Union{Float32, Float64}

which end up matching slightly different sets of methods:

1-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(\),Union{LQ{T,S} where S<:AbstractMatrix{T}, LQ{T,S} where S<:AbstractMatrix{T}},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}, svec(T<:Union{Float32, Float64}), \(F::LQ{T,S} where S<:AbstractMatrix{T}, B::Union{Array{Complex{T},1}, Array{Complex{T},2}}) where T<:Union{Float32, Float64} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:322, true)

vs

2-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(\),LQ{T,S} where S<:AbstractMatrix{T},Union{Array{Complex{T},1}, Array{Complex{T},2}}} where T<:Union{Float32, Float64}, svec(T<:Union{Float32, Float64}), \(F::LQ{T,S} where S<:AbstractMatrix{T}, B::Union{Array{Complex{T},1}, Array{Complex{T},2}}) where T<:Union{Float32, Float64} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:322, false)
 Core.MethodMatch(Tuple{typeof(\),Union{LQ{TA,S} where S<:AbstractMatrix{TA}, LQ{TA1,S} where S<:AbstractMatrix{TA1}},Union{Array{Complex{TA},1}, Array{Complex{TA1},2}}} where TA1<:Union{Float32, Float64} where TA<:Union{Float32, Float64}, svec(TA<:Union{Float32, Float64}, TB<:(Complex{TA} where TA<:Union{Float32, Float64})), \(A::LQ{TA,S} where S<:AbstractMatrix{TA}, B::StridedVecOrMat{TB}) where {TA, TB} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:310, true)

@vtjnash vtjnash force-pushed the jn/new_ambig branch 2 times, most recently from b70bcce to d83a051 Compare August 28, 2020 18:25
more symmetry in use of type-intersection
@vtjnash vtjnash merged commit 21ff6ee into master Sep 2, 2020
@vtjnash vtjnash deleted the jn/new_ambig branch September 2, 2020 04:52
@timholy
Copy link
Member

timholy commented Sep 2, 2020

Sorry, I should have looked at this earlier. While you've carefully carved out exemptions for ambiguity tests that run within Julia itself, what should packages do? I have a lot of packages where the tests include detect_ambiguities, and as of today we're seeing new failures. For example, https://travis-ci.org/github/JuliaDebug/JuliaInterpreter.jl/jobs/723397378.

I'm not arguing that the new algorithm is wrong (I haven't checked), but just wondering whether there's a plan for moving forward.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 2, 2020

There’s no reason anymore to include Base and Core in your list. List just the modules you care about.

@timholy timholy added the needs news A NEWS entry is required for this change label Sep 2, 2020
@timholy
Copy link
Member

timholy commented Sep 2, 2020

Is that true even if we add methods to functions owned by Base?

I'll mimic the style of tests here. I am adding a "needs news" tag.

Comment on lines +159 to +162
# this is basically just a list of everything for which typeintersection is too conservative,
# and ends up matching some methods that don't actually have an interesting intersection
expect = []
push!(expect, (Tuple{typeof(convert),Type{T},Any} where T>:Union{Missing, Nothing}, Tuple{typeof(convert),Type{T},T} where T>:Nothing))
Copy link
Member

@tkf tkf Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to understand this list of methods enumerated in the expect vector. Since this PR is an improvement, they are previously false-negative and now true-positive, right? If so, is it correct to assume that I should be able to invoke a method error with some combination? However, I couldn't get the run-time ambiguity error with a few examples for this first item of expect. What am I doing wrong here?

julia> VERSION
v"1.6.0-DEV.819"

julia> convert(Union{Missing, Nothing}, missing)
missing

julia> convert(Union{Missing, Nothing}, nothing)

julia> convert(Union{Missing, Nothing, Int}, missing)
missing

julia> convert(Union{Missing, Nothing, Int}, nothing)

julia> convert(Union{Missing, Nothing, Int}, 1)
1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily—the type system might claim that such an ambiguity exists, while no counterfactual actually does. I posted a link above that references this particular failure in the type system (#36951).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the API of detect_ambiguities? I've been assuming that it returns a non-empty vector if and only if there exist concrete argument types that would cause a method error. Could you document the exact guarantee? For example, it would be helpful to document that the users should expect false positive or negative.

If providing a guarantee is not possible or not appropriate (e.g., to allow the type system to evolve), can we demote detect_ambiguities (and detect_unbound_args?) to experimental API and explicitly mention as such.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from problems with the type system, that is what this did before and is still what it does. Hence the new bug report number also. There was a bug in v1.0 where someone got a conditional backwards and accidentally additionally filtered out any ambiguity resulting from a failure of type-intersection. Hence this PR. That's now fixed.

Copy link
Member

@tkf tkf Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what this did before and is still what it does

What is that? Could you explain what API detect_ambiguities provides now? Could you clarify what kind of "bound" users can assume with respect to the run-time behavior (method error)? No false positive (which is apparently not the case from your previous comment?)? No false negative (i.e., if detect_ambiguities returns an empty vector, there will be no method error; given there is no bug, of course)? Both kinds of failures are possible (i.e., it's just a hint)?

@tkf
Copy link
Member

tkf commented Sep 3, 2020

I noted above #36962 (comment), I think we need more documentation. So, I'm also adding needs docs

I'm also adding breaking because:

  1. It breaks existing user code bases.
  2. It removes a keyword argument.
  3. If we retrofit a more specific API definition by introducing new documentation, it could be different from the users' mental models. It is not ideal to change the behavior of a 4-year-old API (be8679e) and claim a change in observable behavior as a bug fix or improvement when there is no documentation to ensure that the old and new behaviors are compatible in some ways.

@tkf tkf added breaking This change will break code needs docs Documentation for this change is required labels Sep 3, 2020
@JeffBezanson
Copy link
Member

JeffBezanson commented Sep 5, 2020

This seems to cause some significant latency regressions.
Current master:

Base  ───────  20.674068 seconds
Stdlibs: ────  31.106509 seconds 60.0718%

julia> @time using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
 94.738177 seconds (8.06 M allocations: 545.291 MiB, 0.12% gc time)

julia> @time display(plot(rand(10)))
  7.265292 seconds (10.40 M allocations: 601.608 MiB, 3.82% gc time)

julia> @time using Plots
  4.513663 seconds (7.97 M allocations: 539.723 MiB, 2.35% gc time)

with this reverted:

Base  ───────  20.185031 seconds
Stdlibs: ────  29.825570 seconds 59.6366%

julia> @time using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
 85.283090 seconds (6.21 M allocations: 443.919 MiB, 0.12% gc time)

julia> @time display(plot(rand(10)))
  6.373428 seconds (8.95 M allocations: 514.353 MiB, 4.12% gc time)

julia> @time using Plots
  3.221717 seconds (6.12 M allocations: 438.344 MiB, 3.30% gc time)

So I don't think this change is worth it.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2020

Yeah, I don’t think inference should be asking us to do the more expensive query (eliminate ambiguous methods) that this makes even worse. Need to first get data on what patterns this is affecting. IIUC, this is revealing problems with type-intersection that are stealing away a lot of performance by making it slightly worse in those cases.

mergify bot pushed a commit to JuliaTesting/Aqua.jl that referenced this pull request Sep 6, 2020
 (#32)

* Do not hard-code default options for `Test.detect_ambiguities`
* Ignore ambiguities from Base for now
tkf added a commit to JuliaFolds/BangBang.jl that referenced this pull request Sep 6, 2020
Using commit:
Workaround over-sensitive ambiguity detection from JuliaLang/julia#36962 (#32)
JuliaTesting/Aqua.jl@fe03e3f
tkf added a commit to JuliaFolds/Transducers.jl that referenced this pull request Sep 6, 2020
Using commit:
Workaround over-sensitive ambiguity detection from JuliaLang/julia#36962 (#32)
JuliaTesting/Aqua.jl@fe03e3f
mergify bot pushed a commit to JuliaFolds/Transducers.jl that referenced this pull request Sep 6, 2020
 (#419)

* Update to JuliaTesting/Aqua.jl#32
* Don't call test_ambiguities with Base in julia >= 1.6.0-DEV.816
timholy added a commit to JuliaDebug/LoweredCodeUtils.jl that referenced this pull request Sep 7, 2020
timholy added a commit to JuliaDebug/LoweredCodeUtils.jl that referenced this pull request Sep 7, 2020
@timholy
Copy link
Member

timholy commented Sep 9, 2020

I'm not entirely sure from @vtjnash's response above what the future is likely to look like. Will this be reverted? Or will this be retained but fixed to address the performance issues Jeff noticed? The reason I ask is that tons of packages are facing ambiguity-related test failures, and I'm unsure whether to migrate to the new style (the right strategy if this is being kept) or keep the old (if this is being reverted).

@timholy
Copy link
Member

timholy commented Sep 9, 2020

Ah, I see from #37458 (comment) that this seems likely to be reverted.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 9, 2020

It should probably be surgically disabled again (as this code has been), rather than undoing the bug fixes entirely, but I can't get to that till next week.

JeffBezanson pushed a commit that referenced this pull request Sep 9, 2020
vtjnash added a commit that referenced this pull request Sep 16, 2020
vtjnash added a commit that referenced this pull request Sep 16, 2020
Removes a part of #36962 that was the source of major ire by several
users: This restores some abort code paths that I had put in place to
cause isambiguous to return incorrect answers in this particular case.
Even though the underlying algorithm can now deal with these cases
correctly, users were not pleased that the algorithm was now handling
these cases correctly and attacked with gusto.

Additionally, it fixes a performance bug in ml-matches, where I'd
accidentally set two flags in the wrong direction, which were still
conservatively correct, but prevented us from using the fast path.
vtjnash added a commit to vtjnash/Aqua.jl that referenced this pull request Oct 15, 2024
Reporting ambiguities that cannot be fixed by the user since they are
not part of the testtarget can be harmful to the user experience since
they will never be useful, since at least JuliaLang/julia#36962.

Fixes JuliaTesting#77
lgoettgens added a commit to JuliaTesting/Aqua.jl that referenced this pull request Oct 15, 2024
Reporting ambiguities that cannot be fixed by the user since they are
not part of the testtarget can be harmful to the user experience since
they will never be useful, since at least JuliaLang/julia#36962.

---------

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs docs Documentation for this change is required needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants