Error on loading graph (Slice)

[dstarkenburg]

Hello!

Im trying to parse one of the files I have been working on adding compatibility for, and I am getting a strange error on Slice.

using Flux
using ONNX
using Umlaut
using ProtoBuf

function onnxtoumlaut(file_name::String, input_array::Array)
    onnxfile = ONNX.load(file_name, input_array)
    return onnxfile
end

function onnxtojuliafunc(file::String, input_array::Array)
    onnxfile = onnxtoumlaut(file, input_array)
    final_function= compile(onnxfile)
    return final_function
end

my_func = onnxtojuliafunc("14_ieee_ml4acopf.onnx", ones(Float, 22, 1))

this code throws this error

and here is the view from Roder's netron app

here is the ONNX file https://github.com/AI4OPT/ml4acopf_benchmark/tree/main/onnx

am I missing something about the shapes of my dummy input or has Slice not been implemented for the latest version of ONNX?

[dfdx]

Can you please run the following and then repeat the experiment?

@eval function ONNX.onnx_slice(
        data::AbstractArray,
        starts::VecOrMat{Int}, ends::VecOrMat{Int},
        axes::Vector{Int}=Int[], steps::Vector{Int}=Int[])

    @show starts
    @show ends
    @show axes
    @show steps

    axes = isempty(axes) ? collect(0:ndims(data)-1) : axes
    steps = isempty(steps) ? [1 for i=1:ndims(data)] : steps
    @assert all(starts .>= 0) "Negative indices are not supported yet"
    @assert all(ends .>= 0) "Negative indices are not supported yet"
    # construct ranges, adjusting starts to 1-based indexing
    ranges = [s+1 : st : e for (s, st, e) in zip(starts, steps, ends)]
    # reversed, 1-based dimensions
    dims = ndims(data) .- axes
    # dimension => range mapping
    d2r = Dict(zip(dims, ranges))
    I = [get(d2r, i, (:)) for i=1:ndims(data)]
    return data[I...]
end

[dstarkenburg]

EDIT: 9223372036854775807 is the max value for a 64-Bit signed integer. I dont think my ONNX file has any negative indices but let me know if this could be a cause.

[dfdx]

It turns out, Slice has a pretty complicated rules for preprocessing of the indexes, including clamping of the ends array:

The clamping for the adjusted ends[i] depends on the sign of steps[i] and must accommodate copying 0 through dims[axes[i]] elements, so for positive stepping ends[axes[i]] is clamped to [0, dims[axes[i]]], while for negative stepping it is clamped to [-1, dims[axes[i]]-1].

A quick fix would be to add clamping to out implementation too:

dims = size(data)                                                                       # <-- added
axes = isempty(axes) ? collect(0:ndims(data)-1) : axes
steps = isempty(steps) ? [1 for i=1:ndims(data)] : steps
ends = [clamp(e, 0, dims[axes[i]]) for (i, e) in enumerate(ends)]    # <-- added
...

Using this fix, I was able to get further, but loading still fails on another node:

┌ Error: Error while loading node ("/ConstantOfShape_output_0",) = ConstantOfShape("/Constant_38_output_0",)
│     value => TensorProto([1], 7, nothing, Float32[], Int32[], Vector{UInt8}[], Int64[], "", "", UInt8[0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], StringStringEntryProto[], Main.var"TensorProto.DataLocation".DEFAULT, Float64[], UInt64[])
└ @ Main ~/work/ONNX.jl/src/load.jl:125
ERROR: MethodError: no method matching iterate(::TensorProto)
The function `iterate` exists, but no method is defined for this combination of argument types.

If you have an intuition about this new error, feel free to go forward with a solution. Otherwise I'll be able to take a deeper look at this graph in ~1 week.

[dstarkenburg]

Let me implement the fix and see what I can do about ConstantOfShape. Since that was one I wrote recently, I likely didn't read a constraint that is necessary for this, i.e. dumping the value of an array and not itterating over the Tensor itself.

Ill let you know if I find anything!

[dstarkenburg]

Using this fix, I was able to get further, but loading still fails on another node:

So, I tried to implement the fix as described above and now it says this when testing the package

Did you remove/edit/change any other lines of code besides those two?

[dfdx]

Ah, right, for Julia we need to clamp to 1 from below, not to 0. I.e.

# current
ends = [clamp(e, 0, dims[axes[i]]) for (i, e) in enumerate(ends)]
# correct
ends = [clamp(e, 1, dims[axes[i]]) for (i, e) in enumerate(ends)]

[dstarkenburg]

Ah, right, for Julia we need to clamp to 1 from below, not to 0. I.e.

After trying this, I still get the same error. Perhaps the redefinition of "dims" is messing something up? Also the axes are [0, 1] in save-load, do we need to edit that to be julia friendly?

[dfdx]

Redefining dims as a local variable shouldn't affect the global scope, but your guess about axes seems to be correct: dims is indexed as 1:ndims (Julia style), so the axes, which are now in C style, should be updated to 1:ndims too. Let's try:

axes = isempty(axes) ? collect(1:ndims(data)) : axes

[dstarkenburg]

After trying that I still get the same error. By removing the two added lines let the tests pass again so it must be something in the added code. I cannot tell where we are indexing using 0 since none of the iterables start at zero anymore.

[dfdx]

Nailed it. Try this one:

function onnx_slice(
        data::AbstractArray,
        starts::VecOrMat{Int}, ends::VecOrMat{Int},
        axes::Vector{Int}=Int[], steps::Vector{Int}=Int[])
    shape = size(data)
    starts = starts .+ 1
    axes = isempty(axes) ? collect(1:ndims(data)) : axes .+ 1
    dims = ndims(data) .- axes .+ 1    # 1-based, column-major version of axes
    steps = isempty(steps) ? [1 for i=1:ndims(data)] : steps
    # TODO: check on real graphs with too high indexes if the logic of axes/dims reversal is correct
    # there's a chance that we should actually clamp by shape[axes[i]] instead of shape[dims[i]], i.e.:
    # ends = [clamp(e, 1, shape[axes[i]]) for (i, e) in enumerate(ends)]
    ends = [clamp(e, 1, shape[dims[i]]) for (i, e) in enumerate(ends)]
    @assert all(starts .>= 1) "Negative indices are not supported yet"
    @assert all(ends .>= 1) "Negative indices are not supported yet"
    # construct ranges, adjusting starts to 1-based indexing
    ranges = [s : st : e for (s, st, e) in zip(starts, steps, ends)]
    # dimension => range mapping
    d2r = Dict(zip(dims, ranges))
    I = [get(d2r, i, (:)) for i=1:ndims(data)]
    return data[I...]
end

The problem was that the first part of the code (before the line "# construct ranges, adjusting starts to 1-based indexing") operated on zero-based indexes, and in the line for clamping we tried to use them for indexing Julia array, which of course failed. So I rewrote code a bit to make indexes 1-based from the very beginning. I'm still not sure about the order of axes/dims in clamping though (recall that axes in Julia arrays go in reversed order) - at this point the indexing logic becomes too involved to analyze it without real-life examples.