JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.71k stars 5.49k forks source link

Strange type parameterization issue introduced around 30 June (or perhaps 16 June) #12063

Closed sbromberger closed 9 years ago

sbromberger commented 9 years ago

My LightGraphs.jl code started failing tests around 6/30. Before that date, it was working. The problem is in this code:

type MinCutVisitor{T} <: AbstractMASVisitor
  graph::SimpleGraph
  parities::AbstractArray{Bool,1}
  colormap::Vector{Int}
  bestweight::T
  cutweight::T
  visited::Integer
  distmx::AbstractArray{T, 2}
  vertices::Vector{Int}
end

function MinCutVisitor{T}(graph::SimpleGraph, distmx::AbstractArray{T, 2})
  n = nv(graph)
  MinCutVisitor(
    graph,
    falses(n),
    zeros(Int,n),
    typemax(T),
    zero(T),
    zero(Int),
    distmx,
    @compat(Vector{Int}())
)
end
...
function mincut{T}(
  graph::SimpleGraph,
  distmx::AbstractArray{T, 2}
 )

  visitor = MinCutVisitor(graph, distmx)     # <------ failure is here
  colormap = zeros(Int, nv(graph))

  traverse_graph(graph, T, MaximumAdjacency(), 1, visitor, colormap)

  return( visitor.parities, visitor.bestweight)
end

with this test:

g = Graph(8)

# Original example by Stoer

wedges = [
    (1, 2, 2.),
    (1, 5, 3.),
    (2, 3, 3.),
    (2, 5, 2.),
    (2, 6, 2.),
    (3, 4, 4.),
    (3, 7, 2.),
    (4, 7, 2.),
    (4, 8, 2.),
    (5, 6, 3.),
    (6, 7, 1.),
    (7, 8, 3.) ]

m = length(wedges)
eweights = spzeros(nv(g),nv(g))

for (s, d, w) in wedges
    add_edge!(g, s, d)
    eweights[s, d] = w
    eweights[d, s] = w
end
...
parity, bestcut = mincut(g, eweights)  # <------ failure is here

with this error:

ERROR: LoadError: LoadError: MethodError: `convert` has no method matching convert(::Type{LightGraphs.MinCutVisitor{T}}, ::LightGraphs.Graph, ::BitArray{1}, ::Array{Int64,1}, ::Float64, ::Float64, ::Int64, ::Base.SparseMatrix.SparseMatrixCSC{Float64,Int64}, ::Array{Int64,1})
This may have arisen from a call to the constructor LightGraphs.MinCutVisitor{T}(...),
since type constructors fall back to convert methods.
Closest candidates are:
  LightGraphs.MinCutVisitor{T}(::Union{LightGraphs.DiGraph,LightGraphs.Graph}, ::AbstractArray{Bool,1}, ::Array{Int64,1}, ::T, ::T, ::Integer, ::AbstractArray{T,2}, ::Array{Int64,1})
  LightGraphs.MinCutVisitor{T}(::Union{LightGraphs.DiGraph,LightGraphs.Graph}, ::AbstractArray{T,2})
  call{T}(::Type{T}, ::Any)

Now, the strange part: if I change the outer constructor in one of two ways, the tests pass:

1) if I explicitly call the inner constructor with {T}, it works 2) if I get rid of the {T} parameterization in the outer constructor definition (and change AbstractArray{T,2} to AbstractArray), it works.

The first one is confusing because I don't know why I'd have to pass the type to the inner constructor.

The second one is particularly confusing since I reference T in the code of the outer constructor. Yet the tests pass.

Even more confusing: if the distmx is a sparse array of Integers (instead of Floats as in the test), it works without any modification.

In any case, more details are here: https://groups.google.com/d/msg/julia-users/uAntZ-eJ7kY/Heqq32Lhr3EJ

yuyichao commented 9 years ago

Ahh, I forgot that you've already bisected. That's about the only think I can suggest... (you can probably check if my smaller script can reproduce the same result arround the bad commit although I'm not sure if we'll learn anything new...)

I think @vtjnash and @JeffBezanson are also working on some type id issues that might be related to this so I'll just wait to see what they think. This is also enough debugging of typeinf for me today....

P.S. I have to say that you need to be quite unlucky to hit this. At least in the script I have above, even very subtle changes of the order or types makes the problem go away...

sbromberger commented 9 years ago

Pinging here - what can I do to help fix this? It's causing breakages on LightGraphs for 0.4 only.

JeffBezanson commented 9 years ago

@yuyichao Thanks for the reduced test. I managed to reduce it further to this:

f2{T}(tt, g, p, c, b, v, cu::T, d::AbstractArray{T, 2}, ve) = 1
f2(args...) = 2

f() = f2(0, 0, 0, 0, 0, 0, 0.0, spzeros(0,0), Int[])

@assert f() == 1

If you so much as breathe on this it starts working, otherwise it fails.

vtjnash commented 9 years ago

it looks like _methods doesn't know how to deal with the Vararg{Union} object created by limit_tuple_type:

julia> Base._methods(f2,Tuple{Int,Int,Int,Int,Int,Int,Float64,Vararg{Union{Base.SparseMatrix.SparseMatrixCSC{Float64,Int64},Array{Int64,1}}}},4)
1-element Array{Any,1}:
 svec(Tuple{Int64,Int64,Int64,Int64,Int64,Int64,Float64,Vararg{Union{Base.SparseMatrix.SparseMatrixCSC{Float64,Int64},Array{Int64,1}}}},svec(),f2(args...) at none:1)
vtjnash commented 9 years ago

fwiw, this was not an issue on v0.3:

julia> Base._methods(f2,(Int,Int,Int,Int,Int,Int,Float64,Union(Base.SparseMatrix.SparseMatrixCSC{Float64,Int64},Array{Int64,1})...),4)
2-element Array{Any,1}:
 ((Int64,Int64,Int64,Int64,Int64,Int64,Float64,SparseMatrixCSC{Float64,Int64},Union(Array{Int64,1},SparseMatrixCSC{Float64,Int64})),(T,Float64),f2{T}(tt,g,p,c,b,v,cu::T,d::AbstractArray{T,2},ve) at none:1)
 ((Int64,Int64,Int64,Int64,Int64,Int64,Float64,Union(Array{Int64,1},SparseMatrixCSC{Float64,Int64})...),(),f2(args...) at none:1)                                                                            

the difference I see between these is the order in which they got inserted into the Union, which might help explain the bisect result (since that commit might have changed the order of uid assignment for these two types).

ScottPJones commented 9 years ago

Why does it get that Vararg{Union{ type there, instead of Base.SparseMatrix...., which I think would work? Removing a single argument makes it work correctly.

julia> a = spzeros(0,0)
0x0 sparse matrix with 0 Float64 entries:

julia> isa(a, AbstractArray{Float64,2})
true
ScottPJones commented 9 years ago

Is this related at all to the changes that have affected inlining as well? (number of subscripts)

JeffBezanson commented 9 years ago

Ok, I think this is fixed. Plz confirm whether the original LightGraphs.jl problem is fixed.

sbromberger commented 9 years ago

@JeffBezanson THANK YOU - this appears to have fixed the failures in LightGraphs.

Just out of interest - was it the commit I found via bisect that introduced the issue? (If so, it'd be the first time bisect's actually worked for me.)

JeffBezanson commented 9 years ago

Well, the bisect result was correct, but that commit did not point to the root cause. @vtjnash 's comment nailed it: https://github.com/JuliaLang/julia/issues/12063#issuecomment-121773531 The bisected commit made a change that should have been unrelated, but happened to expose the true bug.

tkelman commented 9 years ago

So I was preparing a backport branch for a probably-final 0.3.x release and accidentally brought a few extra tests in test/core.jl because of git conflicts, and found this was actually not working correctly on release-0.3. It was prior to #7882, but not after. Does this matter?

# bad: [ff764a40cc5ff5569649ac2d28142ad0f16135de] Allow inference of super even if has TypeVars (fix #12636)
git bisect bad ff764a40cc5ff5569649ac2d28142ad0f16135de
# bad: [3e70f9a863cf1d8fdea9850e2815de25405032f3] Merge branch 'release-0.3' of github.com:JuliaLang/julia into release-0.3
git bisect bad 3e70f9a863cf1d8fdea9850e2815de25405032f3
# bad: [07b75b99a828c2d54d0522b258d7f11c778a4133] Update packages.rst
git bisect bad 07b75b99a828c2d54d0522b258d7f11c778a4133
# bad: [07b75b99a828c2d54d0522b258d7f11c778a4133] Update packages.rst
git bisect bad 07b75b99a828c2d54d0522b258d7f11c778a4133
# bad: [07b75b99a828c2d54d0522b258d7f11c778a4133] Update packages.rst
git bisect bad 07b75b99a828c2d54d0522b258d7f11c778a4133
# good: [a7c1c328746e4d20d81e72a2dffa07d18c929b36] Also make line a size_t in task.c
git bisect good a7c1c328746e4d20d81e72a2dffa07d18c929b36
# good: [12166dd41fd57b5607aab4654dbbdedc4f04153d] Use config=Dict() instead of {} for Pkg.generate. Fixes #7855
git bisect good 12166dd41fd57b5607aab4654dbbdedc4f04153d
# bad: [1b0148ed2d1676d43a91d3e77a23a18e9604db81] Revert "Merge pull request #7888 from JuliaLang/teh/qrldiv"
git bisect bad 1b0148ed2d1676d43a91d3e77a23a18e9604db81
# good: [0006d81d475fee0dfe2cbbace2b904922c71da4a] Merge pull request #7874 from StephenVavasis/patch-4
git bisect good 0006d81d475fee0dfe2cbbace2b904922c71da4a
# good: [d2ba96586634bbb84db5844e7f5360ed6d12e1e4] Merge pull request #7880 from mbauman/show-function-prec
git bisect good d2ba96586634bbb84db5844e7f5360ed6d12e1e4
# bad: [6e2dc79065f1a0f4b73117754abababec3455a29] Merge pull request #7889 from JuliaLang/in/d3
git bisect bad 6e2dc79065f1a0f4b73117754abababec3455a29
# bad: [b21c386f090c9b6b6717030b5ca907afae496d87] Merge pull request #7882 from JuliaLang/dcj/gotobug
git bisect bad b21c386f090c9b6b6717030b5ca907afae496d87
# good: [78cca5b09cc61fb52ad48c99e0bf35cd268cfc6d] Fix goto/label in macros invoked from another module.
git bisect good 78cca5b09cc61fb52ad48c99e0bf35cd268cfc6d
# first bad commit: [b21c386f090c9b6b6717030b5ca907afae496d87] Merge pull request #7882 from JuliaLang/dcj/gotobug

edit: the above bisect might be garbage, if this happens intermittently... not sure