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

Updates for Julia v0.7/v1.0 Compatibility #14

Merged
merged 9 commits into from
Oct 21, 2018
Merged

Updates for Julia v0.7/v1.0 Compatibility #14

merged 9 commits into from
Oct 21, 2018

Conversation

ccoffrin
Copy link
Collaborator

Most updates are fairly strait forward.

The only non-trivial one is findnz. It seems the return type semantics have changed which Compat not a viable option for this function. My solution was to implement the function in this package using the recommended v1.0 revision and only add this function when running v0.7 or higher.

@chriscoey
Copy link
Member

am I alone in thinking v0.6 support should be dropped?

@ccoffrin
Copy link
Collaborator Author

For the LANL packages I am endeavoring to make at least one version of every thing that simultaneously supports v0.6/v0.7/v1.0. When that is done, everyone at LANL will have a path to update to v1.0 and we will drop v0.6 support in the subsequent version.

@chriscoey
Copy link
Member

I'm more of a just rip the band-aid off kinda guy

@ccoffrin
Copy link
Collaborator Author

Right now MI* support is so spotty in v0.7, ripping of the band-aid would probably kill LANL. :-)

@chriscoey
Copy link
Member

chriscoey commented Oct 20, 2018

Right, but older releases will continue to support 0.6
Edit: major swypo

@ccoffrin
Copy link
Collaborator Author

You have much more faith in the Julia packager manager than I.

@ccoffrin
Copy link
Collaborator Author

So far it's been really helpful for me that the solvers, e.g. SCS are working in all three. It helps track down if new errors are due to the solver version update or the Julia version update.

@chriscoey
Copy link
Member

I struggle to understand this. Julia 0.6 and oldpkg keep packages under .julia/v0.6, whereas Julia 0.7+ and pkg keep packages in a different location.

I am prepared to go and update Pajarito soon but I think it's much easier and cleaner to just tag master with the right Julia version bounds and then start making 0.7+ only compatible changes and then tag again with updated version bounds.

@chriscoey
Copy link
Member

If the upgrade is purely changing syntax for 0.7 then any regressions are due to a Julia version update.

There isn't really anything algorithmic that will change in Pavito and Pajarito.

@ccoffrin
Copy link
Collaborator Author

If you would like a tutorial in Julia v0.7 package development, I would be happy to provide one. It is very different from Julia v0.6, but it is possible to support both simultaneously from one repo.

As long as,

  1. your tests are comprehensive
  2. there are no unfinished new features in branches
  3. all of your up-steam dependencies support v0.7
    Then a hard jump from v0.6 to v0.7 seems reasonable.

Basically all of the packages I work on fail point 2 (incomplete migrations to MOI) and 3 (limited MI solvers in v0.7)

I will note that using Compat, I found it pretty easy to support v0.6 using v0.7 features.

@chriscoey
Copy link
Member

Ok. I suppose part of my reluctance to support both on master stems from (a) desire for simplicity of the code and (b) belief that we should not make it easy for people to just stay on 0.6 once the package supports 1.0.

Anyway, so this suggests a division of labor. It sounds like updating Pajarito for MOI is a much bigger job than updating for 1.0. If you would care to do the 1.0 update (keeping v0.6 support if you need) then I'll subsequently update for MOI. That will essentially be a rewrite of half of Pajarito's codebase. Pavito should be easier because the NLP interface is mostly unchanged.

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

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

Thanks for updating! A few comments.

.travis.yml Outdated
julia:
- 0.6
- 0.7
# - nightly
- 1.0
- nightly
Copy link
Member

Choose a reason for hiding this comment

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

Please drop nightly. It slows CI times for all JuliaOpt packages because of the shared queues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,17 @@
name = "ConicBenchmarkUtilities"
Copy link
Member

Choose a reason for hiding this comment

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

Last time I checked project.toml files and manifest.toml files are not needed for registered packages and can confuse the package manager (JuliaRegistries/General#22). Is there more recent guidance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would seem that I do not know enough about the new package manager to understand how to use it without toml files. For example, how does one run test inside the Pkg REPL? If there is a tutorial on how to do this I can look into dropping the toml files.

For what it is worth, when setting up the toml files I use the same UUID as the General registry. So I don't think there will be any confusion.

using SparseArrays
using LinearAlgebra
# this is required because findall return type changed in v0.7
function SparseArrays.findnz(A::AbstractMatrix)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to update the code to work with the new return types so that later we can just delete the 0.6 compatibility blocks and have clean 0.7/1.0 code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed this would be preferable. I tried to do it that way for about an hour, but in the end I think it is impossible or requires a much deeper knowledge of sparse matrices in Julia than I have.

The root of the challenge is that the semantics of findall have also changed in v0.7. For the tests in this package, the v0.6 findall returns a list of integers (not clear how ints map back to matrix cells) and the v0.7 findall returns a list of Cartesian data types. So the workaround code proposed for v0.7 is incompatible with v0.6. Using Compat on findall to bring the v0.7 semantics into v0.6 is also no successful.

test/runtests.jl Outdated
@@ -25,17 +36,20 @@ using ConicBenchmarkUtilities
x_sol = MathProgBase.getsolution(m)
objval = MathProgBase.getobjval(m)

#=
# drop Convex.jl
Copy link
Member

Choose a reason for hiding this comment

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

Better to just delete the code instead of leaving a commented block that's unlikely to be revived. The code is stored in the git history if we need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@ccoffrin
Copy link
Collaborator Author

@chriscoey, I'll make you a counter offer.

You write some new tests for ConicNonlinearBridge, so that the test code coverage is above 80% and I will make Pavito and Pajarito work with JuMP v0.18 in Julia v0.6/0.7/1.0. After that is done, migrations to MOI and JuMP v0.19 can be discussed. :-)

@chriscoey
Copy link
Member

Ok deal. Let me know when to start on tests!

@ccoffrin
Copy link
Collaborator Author

Add JuMP-based tests to https://github.com/mlubin/ConicNonlinearBridge.jl ASAP. Once that done, I should be able to update it in short order.

@ccoffrin
Copy link
Collaborator Author

I need those tests before I can start migrating Pavito.

@chriscoey
Copy link
Member

old JuMP/MPB doesn't really support cone modeling, so I used MPB directly. see PR at mlubin/ConicNonlinearBridge.jl#10

@mlubin mlubin merged commit 8380b72 into JuliaOpt:master Oct 21, 2018
@mlubin
Copy link
Member

mlubin commented Oct 21, 2018

@ccoffrin I moved the repo to JuliaOpt and gave you access. Tag when ready.

@ccoffrin
Copy link
Collaborator Author

ccoffrin commented Oct 21, 2018

Thanks!!! I am gonna play around with dropping the toml files today. I'll tag after I have some more clarity on that point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants