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

adding IPOPT recipe #1607

Merged
merged 16 commits into from Jan 22, 2017
Merged

adding IPOPT recipe #1607

merged 16 commits into from Jan 22, 2017

Conversation

pstjohn
Copy link
Contributor

@pstjohn pstjohn commented Sep 16, 2016

Borrows heavily from https://github.com/SCIP-Interfaces/conda_recipes/tree/master/ipoptlib, this library likely deserves to have its own conda-forge recipe. Currently compiling against MKL on OSX and Linux.

Will depend on

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ipopt) and found it was in an excellent condition.

@@ -0,0 +1,20 @@
#!/bin/bash

THIRDPARTY=(Metis Mumps)
Copy link
Member

Choose a reason for hiding this comment

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

Be aware that the version of Metis downloaded here has a strange license.

You'll need ./get.ASL too if you want an executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up! Do you have a suggestion for a better place to get Metis? There is a conda feedstock, but it only builds a static library. Its also a different version (5.1.0 vs 4.0.3 used from get.Metis)

Just added ASL in the most recent commit, hope that works.

Copy link
Member

Choose a reason for hiding this comment

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

Metis is optional in Ipopt. You'd need to use Mumps 5.x for it to work with Metis 5.x (which has a normal Apache license), and I don't think Ipopt has been updated yet to work with Mumps 5.x. Maybe if you build them all separately it might work as an external library instead of building the vendored copy under ThirdParty, if there aren't API changes. I think homebrew-science might be building Mumps and Ipopt that way.

Probably simplest to leave out Metis for now if you want to redistribute binaries (kind of the point of conda-forge), and revisit with a todo for building latest versions of things separately. If you're going to link against MKL then you should be able to use the MKL implementation of Pardiso for Ipopt anyway, which won't need Metis.

@pstjohn pstjohn changed the title adding IPOPT recipe [WIP] adding IPOPT recipe Sep 18, 2016
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ipopt) and found it was in an excellent condition.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ipopt) and found it was in an excellent condition.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ipopt) and found it was in an excellent condition.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ipopt) and found it was in an excellent condition.

@basnijholt
Copy link
Contributor

@Jvanschoubroeck this will ease your pain 😄

@pstjohn
Copy link
Contributor Author

pstjohn commented Oct 18, 2016

Glad to see there's interest! But this might still be a while before we merge. We'd rather not build third-party libs in each recipe, see coin-or/COIN-OR-OptimizationSuite#3.

There's also the question of whether or not we're allowed to compile against intel's MKL from #1445

@basnijholt
Copy link
Contributor

I would just go with openblas for now.

The MKL provided by anaconda does something to your code that it automatically parallelizes single core operations, which is rather inefficient and what you don't want when you parallelize your own code.

AFAIK you can't compile code without a MKL license, but when the code is already compiled (by someone with a license) you are free to spread it.

Disclaimer: I am far from an expert on any of these subjects.

@tkelman
Copy link
Member

tkelman commented Oct 18, 2016

It's not just BLAS and LAPACK that's the issue here, Ipopt can also use the Pardiso sparse linear solver in MKL which is much faster than the only redistributable option MUMPS.

@pstjohn
Copy link
Contributor Author

pstjohn commented Oct 19, 2016

But you can load a separate linear solver after the fact using shared libraries. I wonder then how much the blas/lapack you supply to configure really matter in that case.

@tkelman
Copy link
Member

tkelman commented Oct 19, 2016

Have you used the linear solver loader with MKL's Pardiso? I know it works with HSL solvers and with the non-MKL version of Pardiso but I've never heard of anyone using it with a dynamically loaded MKL.

@tkelman
Copy link
Member

tkelman commented Oct 19, 2016

Usually Ipopt is dealing with sparse problems and BLAS only gets called on small dense sub blocks so in my experience the BLAS implementation makes much less difference than the sparse solver. It'll depend on specific problem structure though.

@tkelman
Copy link
Member

tkelman commented Oct 19, 2016

Looking over https://projects.coin-or.org/Ipopt/ticket/216 where we originally implemented linking Ipopt against MKL's Pardiso, since the APIs differ between MKL and non MKL Pardiso and the switch in Ipopt is made based on a compile time flag, I doubt the linear solver loader will work. Maybe if you swap the defaults...

@pstjohn
Copy link
Contributor Author

pstjohn commented Oct 19, 2016

Gotcha, thanks for weighing in! You'd obviously know this better than me, I've never actually used IPOPT with Pardiso.

Where does that leave this recipe? Are you familiar with how COIN currently ships binary versions for IPOPT?

@tkelman
Copy link
Member

tkelman commented Oct 19, 2016

As a big monolithic installer that has all of coin-or, or via distributions that have packaged the individual components. All the openly redistributable binaries use MUMPS as far as I know. I used to maintain the Ipopt matlab interface which was a mex file, and was built in a special way to use the copy of HSL MA57 that comes with matlab. Ampl and Gams also ship their own executable or library that they build, not positive which linear solvers they include. In JuliaOpt we leverage homebrew on mac and have cross compiled binaries for windows that we download from the opensuse build service.

What's the specific question? There are lots of ways to build this for different requirements.

@pstjohn
Copy link
Contributor Author

pstjohn commented Oct 19, 2016

Cool, thanks for the background. The question was what would be the most useful to users looking for IPOPT: it seems that if someone wants to use the MKL pardiso solver, they're going to have to compile from source anyways (vs. grabbing a binary from COIN or elsewhere).

So would a conda install that links against MUMPS & openblas be the best implementation we're allowed to distribute?

@pstjohn
Copy link
Contributor Author

pstjohn commented Dec 22, 2016

CI builds are passing! 🎉
RTM on my end, let me know if I'm missing anything

@pstjohn pstjohn changed the title [WIP] adding IPOPT recipe adding IPOPT recipe Dec 22, 2016
@@ -0,0 +1,58 @@
# Note: there are many handy hints in comments in this example -- remove them when you've finalized your recipe

# Jinja variables help maintain the recipe as you'll update the version only here.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the example comments from your final recipe.


test:
files:
- Cpp_example
Copy link
Member

Choose a reason for hiding this comment

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

I would name this directory test but that is OK.

- scotch
- gcc
- blas 1.1 {{ variant }}
- openblas 0.2.18|0.2.18.*
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now at 0.2.19 I believe. Might as well update now.


extra:
recipe-maintainers:
- Peter St. John
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be your github username.

@basnijholt
Copy link
Contributor

Could this be merged? :)

@pstjohn
Copy link
Contributor Author

pstjohn commented Jan 20, 2017

I'm done with it, do you know who I should ping to get it merged? @conda-forge-admin ?

@@ -0,0 +1,87 @@
Eclipse Public License - v 1.0
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be in the recipe as license_file is able to get this from the source.

- mumps
- metis
- scotch
- gcc
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the whole compiler at run time or just libgcc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming we need the whole compiler, mainly because the test compiles a test program and makes sure the solution is optimal. I could cut these though if that's not warranted.

g++ -I$PREFIX/include/coin  -c -o cpp_example.o cpp_example.cpp
...
./cpp_example | grep -q "Optimal Solution"

Copy link
Member

Choose a reason for hiding this comment

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

Normally one adds gcc to test/requires then.

Copy link
Member

Choose a reason for hiding this comment

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

Done below.

Note: Diff comments have moved from run to build.


about:
home: https://projects.coin-or.org/Ipopt/
license: EPL
Copy link
Member

Choose a reason for hiding this comment

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

Would add 1.0 as it appears to be that version of EPL.

about:
home: https://projects.coin-or.org/Ipopt/
license: EPL
license_file: LICENSE
Copy link
Member

Choose a reason for hiding this comment

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

Seems the license file at this level is just filler.

For licensing information for files in this directory, please see the
comments in the header of each file. For licensing of files in each of
the subdirectories, please see the LICENSE file in that subdirectory.

Instead would use license_file: Ipopt/LICENSE, which appears to be the actual EPL 1.0 license.

@jakirkham
Copy link
Member

Thanks @pstjohn. Will merge when it passes.

@pstjohn
Copy link
Contributor Author

pstjohn commented Jan 20, 2017

Thanks for the help finishing this up!

@jakirkham jakirkham merged commit 450ffa8 into conda-forge:master Jan 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants