search

JuliaSmoothOptimizers / AmplNLReader.jl

Julia AMPL Models Conforming to NLPModels.jl
Other
18 stars 10 forks source link

prepping for JuliaOpt #6

Closed mlubin closed 7 years ago

mlubin commented 10 years ago

Continuing the discussion from #4, this package would be a good addition for JuliaOpt. Here's a checklist of what needs to be done:

CC @IainNZ @tkelman

tkelman commented 10 years ago

I think it would be best to port the C wrappers in this package into pure Julia code with ccalls. It doesn't look like it's very long or complicated.

For Windows binaries I'd greatly prefer using WinRPM vs manually building and posting binaries somewhere. This will allow Ipopt and Bonmin binaries to easily depend on the same ampl library.

I have some initial cross-compiled binaries of the more modern AMPL/MP version of libasl in my personal project at the openSUSE build service. I had several patches to https://github.com/ampl/mp a few weeks ago to improve this, but it's still not ideal because it runs a few executables at build time. I can use wine to run them, but that's not acceptable as a permanent solution because 32-bit wine can't execute 64-bit Windows cross-compiled executables. The openSUSE build service is set up so all cross-compiled packages are noarch, and get compiled from both 32 and 64 bit build workers. I can take the approach I used for HDF5 and include locally generated versions of the output files if necessary.

@vitaut, any update on https://github.com/ampl/mp/issues/9 ? The same concern applies for arithchk as well as gen-expr-info.

mlubin commented 10 years ago

The only difficulty in porting the C wrappers to Julia will be mirroring the ASL struct, which will be a bit of a pain but still tractable. I agree that this will help with distributing (and maintaining) the code.

vitaut commented 10 years ago

I've added expr-info.cc to the repo in https://github.com/ampl/mp/commit/4c447a25a066ec637c7d0f9b82cc2d3d29ea7a4e so running gen-expr-info shouldn't be necessary unless you change the code. arith.h is a bit more tricky as it detects target floating-point format. Perhaps it can be deduced from CMAKE_SYSTEM_PROCESSOR with fallback to running arithchk for unknown processors.

dpo commented 10 years ago

What's wrong with building from source on OSX?

I initially started this project by trying to mirror the ASL data structure, but it turned out to be far easier to write C wrappers. An advantage of the C wrappers is that they can be used for other languages as well. I don't think the ASL structure will change, either at all, or so much as to break the C wrappers.

mlubin commented 10 years ago

@dpo the majority of OS X users of Julia don't have a compiler installed, and we don't require them to do so for JuliaOpt packages.

mlubin commented 10 years ago

But anyway packaging binaries on OS X is relatively easy by using the internal Homebrew.jl bottles.

tkelman commented 10 years ago

We can stick with the C wrappers if we really need to, but it makes distributing Windows binaries a good deal messier. Would need to either patch them in as additional source files and libraries in the CMakeLists.txt of ampl/mp, or make a separate download for them.

mlubin commented 10 years ago

Where's the definition of the ASL struct, by the way?

tkelman commented 10 years ago

@mlubin here, I think: https://github.com/ampl/mp/blob/03e7e60a678ad88212670962a6d6fb976526536a/src/asl/solvers/asl.h#L620-L624

It does look like the binary layout of the ASL struct has changed as recently as a few months ago, so will want to be sure all platforms are referring to the same version of ASL.

vitaut commented 10 years ago

@tkelman Actually there are several ASL structures. The one you linked to is linear. There is also a nonlinear version here: https://github.com/ampl/mp/blob/03e7e60a678ad88212670962a6d6fb976526536a/src/asl/solvers/nlp.h#L214-L219, and two more here: https://github.com/ampl/mp/blob/03e7e60a678ad88212670962a6d6fb976526536a/src/asl/solvers/psinfo.h#L285-L303. They do change from time to time.

tkelman commented 10 years ago

Thanks @vitaut. To you and @dpo, what would you think of moving the jampl.c wrapper into the upstream repository and building it as an optional solver? Maybe with a different name. Or would there already be a generic C-API solver that could provide the same functionality built-in?

mlubin commented 10 years ago

Having a simple C API that doesn't depend on accessing internal structures would also be useful for calling asl from a number of other high-level languages that have C FFIs.

vitaut commented 10 years ago

I am fine with moving the C wrapper with some minor changes to https://github.com/ampl/mp if that's OK with @dpo and if it can be distributed under our usual MIT license. It should probably be a library as it is now rather than a solver (executable). The easiest is to make it a part of asl-shared.

dpo commented 10 years ago

Out of curiosity, how does https://github.com/ampl/mp relate to www.ampl.com and the official ASL? I see the official logo on https://github.com/ampl. @vitaut Are you affiliated with AMPL LLC? Is the ASL you distribute identical to the official one?

vitaut commented 10 years ago

AMPL/MP includes the ASL identical to the official one from netlib/solvers (other directories are not included) and provides additional functionality which is described in the project's README. As for my relation to AMPL, I'm a software developer there since 2012.

mlubin commented 10 years ago

Wanted to follow up on this to see if we can make some progress ahead of INFORMS. @dpo, are you okay with relicensing the C wrapper so it can be included in ampl/mp?

tkelman commented 9 years ago

Looks like yes? d74150bb3c1540c04dbc9e9754bb1e6dd302ac1c

dpo commented 9 years ago

Sorry I've been so caught up with other stuff. I'd be happy for ampl.jl to be distributed with ampl/mp but I'd like to keep this repo as is as the main development area of Julia/AMPL. Is it possible to pull this repo as a submodule of ampl/mp?

tkelman commented 9 years ago

@dpo I think what would be best is factoring things out so any C code wrappers could be contributed to ampl/mp, and all Julia code stays here. That will make managing binaries across all platforms much easier.

dpo commented 9 years ago

@tkelman That's a good idea.

@vitaut:

  1. An easy way to install ampl.jl is with Homebrew. Would it be better to change the asl.rb Homebrew formula to pull your releases instead of Dave's who which are never versioned (and cause infinite trouble)? Homebrew bottles the ASL, and so precompiled binaries are available for OSX (and soon for Linux). On Windows, I'm not sure what to do.
  2. How should I migrate the C wrapper to ampl/mp? Submit a PR?
tkelman commented 9 years ago

Would it be better to change the asl.rb Homebrew formula to pull your releases instead of Dave's which are never versioned (and cause infinite trouble)?

+1

On Windows, I'm not sure what to do.

I got it covered. https://build.opensuse.org/package/show/home:kelman:mingw-coinor/mingw64-ampl-mp will allow us to use WinRPM.

vitaut commented 9 years ago

@dpo Thanks for relicensing the code.

  1. Would it be better to change the asl.rb Homebrew formula to pull your releases instead of Dave's who are never versioned (and cause infinite trouble)?

Sounds good. The ASL in ampl/mp repo is regularly updated, so there shouldn't be any difference apart from new the versioning.

  1. How should I migrate the C wrapper to ampl/mp? Submit a PR?

PR would be great. Alternatively, I can add this repo as an optional submodule of ampl/mp as it is done with other modules like gsl or gecode.

mlubin commented 9 years ago

I have nothing to contribute at the moment, but happy to see things moving along!

tkelman commented 9 years ago

Thanks for going through with the PR @dpo, and for merging it @vitaut. Is the code in the upstream repository usable by the Julia package yet? Assuming a near-future release gets tagged when ready with the new code in roughly the state it's in today?

Also for clarity I think this package will probably need a slightly more specific name. Maybe AmplEvaluator or something? Since in addition to this package we will also have an AmplMathProgInterface, and an AmplNlWriter (or similar), all serving slightly different purposes and with slightly different dependency directions.

dpo commented 9 years ago

@tkelman When @vitaut is ready to make a new release with the interface built into the shared lib, I'll be able to test.

We'll need the Homebrew formula for asl to be accepted in Homebrew.jl. Once that's done, writing a build.jl shouldn't be too difficult.

Regarding the name, how about AmplModel? That's already the name of the main type actually.

tkelman commented 9 years ago

Can you test on master to see if there are any issues ahead of time? (temporarily do something like https://github.com/ampl/mp/archive/$sha.tar.gz) Or does it still need work to build into the shared lib?

This package can only import and evaluate functions from a model that's been previously created elsewhere, right? So this isn't creating models straight from mathematical formulations in Julia code quite like JuMP/Convex do, it's primarily importing models that are created in AMPL? Or is there the capability here of populating an AMPL model entirely from Julia code?

dpo commented 9 years ago

The build system still needs work to build the shared lib.

No, this package doesn't create AMPL mod and dat files. It imports nl files and provides a convenient interface. Maybe AmplNlReader is better then?

mlubin commented 9 years ago

AmplNlReader (or AmplNLReader?) sounds good to me. This will fit in with the ecosystem of AMPL-related packages that will soon be published and also makes it more clear that the package is not a replacement for AMPL.

dpo commented 9 years ago

The feature/libasl branch prepares for the external library. There is a tentative build.jl that builds from source or picks up the libraries build via Homebrew. For now the OSX build fails on Travis because libasl doesn't yet include the new interface, and the Linux build fails because Ubuntu 12.04 has an outdated cmake.

dpo commented 9 years ago

I updated build.jl to build the latest libasl, but it seems it's not being detected. The odd thing is that if I brew install asl, then the version previously installed by BinDeps is detected. Anything seems wrong in build.jl?

See https://travis-ci.org/dpo/ampl.jl/jobs/56176503#L668

tkelman commented 9 years ago

Looks like it should work, not sure? Try adding a dlopen of the dylib, see if there's an error message?

The [:libasl => :libasl] will give a deprecation warning that can be avoided with the Compat.jl package via @compat @BinDeps.install Dict(:libasl => :libasl), that will let you use the deps.jl cached file to avoid requiring BinDeps at runtime - it's only necessary for Pkg.build, not using the package.

mlubin commented 9 years ago

libasl.so doesn't properly specify a dependency on libmp:

ERROR: could not load module /home/mlubin/.julia/v0.3/ampl/deps/usr/lib/libasl.so: libmp.so.1: cannot open shared object file: No such file or directory
 in dlopen at c.jl:19

also ldd -r libasl.so lists tons of missing symbols.

CC @vitaut

mlubin commented 9 years ago

Okay, there is a dependency on libmp.so.1 but without a path. So this would work if LD_LIBRARY_PATH was set, although this shouldn't be needed at all.

dpo commented 9 years ago

I removed the [:libasl => :libasl] altogether. I suppose there's no harm in also exporting libmp. And ampl.jl now includes deps.jl. Is that what you meant?

On OSX, I get

$ otool -L ~/.julia/v0.3/ampl/deps/usr/lib/libasl.dylib
/Users/dpo/.julia/v0.3/ampl/deps/usr/lib/libasl.dylib:
    @rpath/libasl.1.dylib (compatibility version 1.0.0, current version 1.4.0)
    @rpath/libmp.1.dylib (compatibility version 1.0.0, current version 1.4.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1197.1.1)
    /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 120.0.0)

Perhaps -rpath wasn't specified when building the dynamic lib?!

vitaut commented 9 years ago

@mlubin Sorry, forgot to update the so versions which could cause missing symbols if wrong version of the library is picked up. Fixed in https://github.com/ampl/mp/releases/tag/2.0.1. I'll have a look why libmp dependency is not working. Probably need to set rpath or something.

dpo commented 9 years ago

@vitaut I'm still having the same issue.

vitaut commented 9 years ago

@dpo If you mean the dependency issue, then I haven't looked into it yet. A trivial workaround is to link with both libasl and libmp.

mlubin commented 9 years ago

There could be a way to force BinDeps to load libmp before libasl, wasn't able to figure it out though.

vitaut commented 9 years ago

According to CMake RPATH handling, CMake clears the RPATH when installing targets. So libasl.so will find libmp.so if it is installed in one of the standard library directories (tested on Linux). Feel free to submit an issue or a PR to AMPL/MP if you want it to be found in non-standard locations.

tkelman commented 9 years ago

Might help if we try manually setting -DCMAKE_INSTALL_RPATH=$prefix/lib ?

mlubin commented 9 years ago

So compilation from source via BinDeps is working now. @tkelman, is asl already in opensuse?

tkelman commented 9 years ago

Yes - https://build.opensuse.org/package/show/windows:mingw:win64/mingw64-ampl-mp @vitaut you may want to have a look at a patch the opensuse guys added, I think they bumped to gcc 5.1.0 and that may have introduced a problem that they added a patch to fix. I'll try to see if I can reproduce the issue at some point.

It looks like there's a dependency now on an unregistered "NLP.jl" package, which I don't think is appropriately named, nor does it contain enough information to actually be fully useful. It looks like it's just storing counts of variables and different types of constraints, but if this package and CUTEst.jl both want to use the same data structure I think the type could probably be added to MathProgBase?

dpo commented 9 years ago

The idea is to merge whatever's in NLP.jl that isn't already in MathProgBase into MathProgBase when it's ready. Currently, NLP.jl helps me experiment with modifying optimization problems in place.

mlubin commented 9 years ago

Almost all of the information in the NLP.jl structure is available through the MathProgBase nonlinear interface, but perhaps in a less convenient form for using it to transform problems when writing a solver. I'd like to see some convergence between the two interfaces, but as a short-term solution I think it would be okay to just move the data structure into MathProgBase. This would let us move forward with registering this package and moving it into JuliaOpt, that is, if you (@dpo) believe that the package is ready.

dpo commented 9 years ago

@mlubin Sure that sounds good to me. I think ampl.jl is in good shape and I'd like to consolidate everything as well.

vitaut commented 9 years ago

@tkelman The issue with GCC 5 is fixed in https://github.com/ampl/mp/commit/40c1f7fbf4a1647c6fadb9969343a68d244b8e48, thanks!

mlubin commented 9 years ago

@dpo, ready to go ahead with this? The next steps are to rename the package to AmplNLReader (as discussed) and publish what's in the develop branch in metadata.

tkelman commented 9 years ago

And uncomment the WinRPM lines in deps/build.jl (the package on opensuse is named "ampl-mp", not "asl" though), add @windows WinRPM to REQUIRE, and turn on appveyor

dpo commented 9 years ago

Almost there. Any idea why this would fail on win32 and nowhere else? https://ci.appveyor.com/project/dpo/amplnlreader-jl/build/1.0.4/job/jrdgtr5bp8vf3b8d#L62

tkelman commented 9 years ago

Integer literals being 32 bit there, probably. Would likely also fail on 32 bit Linux but that's kinda tricky to get working on Travis.

ERROR: `convert` has no method matching convert(::Type{Array{Int64,1}}, ::UnitRange{Int32})
 in NLPModelMeta at C:\Users\appveyor\.julia\v0.3\MathProgBase\src\NLP\nlp_types.jl:108
 in AmplModel at C:\Users\appveyor\.julia\v0.3\AmplNLReader\src\AmplNLReader.jl:74