JuliaOpt / GLPKMathProgInterface.jl

DEPRECATED: Interface between the GLPK.jl wrapper and MathProgBase.jl
Other
17 stars 14 forks source link

changing MPB interface for get/setvartype #14

Closed joehuchette closed 10 years ago

joehuchette commented 10 years ago

JuliaOpt/JuMP.jl#244

carlobaldassi commented 10 years ago

Note: when the release of all this new stuff goes into metadata. the requirements of all solver interfaces versions must be updated, both for the new versions (obviously) and for the old ones (which need to be capped). I also suppose we will want to update the minor version of all interfaces.

Why not having a backward compatibility fallback, so that the chars are simply translated to symbols? (Perhaps with a warning?)

coltype = map(vartype) do c
    isa(c, Char) && warn("some deprecation warning")
    if c == 'I' || c == :Int
        return GLPK.IV
    elseif c == 'C' || c == :Cont
        return GLPK.CV
    elseif c == :Bin
        return GLPK.BV
    else
        error("invalid var type: $c")
    end
end
joehuchette commented 10 years ago

Another approach would be to ensure that setvartype!(lpm, vartype::Vector{Symbol}) is strictly typed for every solver, and provide a generic fallback setvartype(lpm::AbstractMathProgModel, vartype) at the MPB level that gives a warning and maps Chars to Symbols. That way we only need this once, here. Does that cover all the bases?

carlobaldassi commented 10 years ago

That would mostly work, and I'm in favour of stricter typing when it makes sense (like in this case), but it still introduces a subtler backwards incompatibility, in that previously you could pass an Array{Any} containing only Chars.

joehuchette commented 10 years ago

So this would break if someone had an older version of MPB, but a new version of GLPKMathProgInterface. This is a bit of a corner case, but you're right, we should add checks here as well (and for any other solver that didn't have this strictly typed before).

carlobaldassi commented 10 years ago

I think we should just update the requirements all over the board, and bump the minor version to clarify that the API has changed.

joehuchette commented 10 years ago

There's another breaking change in the pipeline (JuliaOpt/MathProgBase.jl#39), so it'd probably make sense consolidate the two changes at once.

carlobaldassi commented 10 years ago

Yes, that surely makes sense. To clarify, I think these are the steps which should be taken:

  1. in METADATA, cap all previous versions of interfaces (and JuMP) to depend on MPB less than 0.3.0-, editing all the requires files.
  2. make the interfaces (and JuMP) depend on MPB 0.3.0-dev1 in all the PRs which introduce the new functionality, editing each package's REQUIRE file.
  3. publish a 0.3.0-dev1 version of MPB. It won't be installed anywhere since all solvers and JuMP can't satisfy the dependency (except for users which have all the interface packages checked out)
  4. Smooth out problems if any, introduce other breaking changes, possibly publishing new -devX versions. During the other breaking changes, the interfaces REQUIRE files may be further updated, but the packages are not published yet.
  5. Finally publish a 0.3.0 version of MPB, and then publish all interfaces.

I think that in this way everything should work smoothly. This procedure removes the need to publish and fix all at once, keeps everything consistent, and gives the opportunity to smooth out problems with MPB during the dev phase. As a shortcut, the -dev phase can be avoided; this would leave only steps 1, 2 and 5.

We should also consider that from now on it probably makes sense to sync all interfaces to a specific API of MBP, so the new REQUIRE files should probably have something like MathProgBase 0.3.0- 0.4.0-.

joehuchette commented 10 years ago

Thanks for detailed explanation, I was a bit hazy before. I'll try to take care of this this weekend.

joehuchette commented 10 years ago

I also like your idea about requiring a particular MPB API; I'll add that as well, and bump MPB to 0.3.0

carlobaldassi commented 10 years ago

Thanks for taking care of this.

joehuchette commented 10 years ago

Thanks so much for your help with this, Carlo!