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

Add conda recipe #261

Closed
wants to merge 5 commits into from
Closed

Conversation

whophil
Copy link
Contributor

@whophil whophil commented Jun 6, 2021

Purpose

Add recipe to build pyoptsparse as a conda package.

The conda package supports all the standard (free) optimizers, plus IPOPT (which it introduces as a build and run-time conda dependency). This all happens under the hood, so users don't need to mess around building IPOPT from source. The particular flavor of metis/mumps dependencies you get is chosen by the maintainers of the IPOPT conda recipe, so if you need full control then you'll still want to build from source.

There was some discussion about Python wheels (#191). While I think a PyPi wheel would be still be a useful thing to have, complex dependencies involving non-Python projects quickly become untenable with wheels. It would be hard to put IPOPT in a wheel without some complicated CI work; with conda, it all comes together easily.

There is one hack in this recipe, and that is related to the dependence on the mdolab-baseclasses package. The ideal solution would be to have mdolab-baseclasses available as a conda package, and to specify that as a run dependency of this package. However, that conda package doesn't exist yet. In the recipe I have embedded the source of mdolab-baseclasses, pinned to the 1.5.2 tag. This can be fixed if/when it is available as a conda package.

EDIT:
It probably makes sense to just submit this to https://github.com/conda-forge/staged-recipes, so consider this PR a demonstration. Somebody affiliated with mdolab should probably be the maintainer of that recipe. I can provide some support if needed (I would be able to open the PRs on staged-recipes for both mdolab-baseclasses and this project).

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (packaging)

Testing

In order to build the package, run this from the top level of the repo.

conda build conda.recipe

You will need conda and conda-build installed. This will build the package and run tests.

To install the conda package, into the active conda environment:

conda install -c local pyoptsparse

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@whophil whophil requested a review from a team as a code owner June 6, 2021 15:58
@whophil whophil changed the title Feature/conda recipe upstream Add conda recipe Jun 6, 2021
@codecov
Copy link

codecov bot commented Jun 6, 2021

Codecov Report

Merging #261 (4ba79c3) into master (a959ac9) will decrease coverage by 10.89%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #261       +/-   ##
===========================================
- Coverage   83.50%   72.61%   -10.90%     
===========================================
  Files          22       22               
  Lines        3268     3268               
===========================================
- Hits         2729     2373      -356     
- Misses        539      895      +356     
Impacted Files Coverage Δ
pyoptsparse/pyoptsparse/pySNOPT/pySNOPT.py 13.86% <0.00%> (-76.24%) ⬇️
pyoptsparse/pyoptsparse/pyNLPQLP/pyNLPQLP.py 25.92% <0.00%> (-66.67%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_solution.py 57.77% <0.00%> (-42.23%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_utils.py 61.86% <0.00%> (-7.91%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_history.py 76.44% <0.00%> (-5.80%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_optimizer.py 83.29% <0.00%> (-1.15%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_optimization.py 78.01% <0.00%> (-0.82%) ⬇️
pyoptsparse/pyoptsparse/pyOpt_error.py 92.00% <0.00%> (+36.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a959ac9...4ba79c3. Read the comment docs.

@whophil whophil force-pushed the feature/conda-recipe-upstream branch from 9f8de1a to e23195b Compare June 6, 2021 16:04
@whophil whophil force-pushed the feature/conda-recipe-upstream branch from e23195b to 4ba79c3 Compare June 6, 2021 20:25
@whophil
Copy link
Contributor Author

whophil commented Jun 6, 2021

This shouldn't affect coverage, there is an old test failure that appears to be stuck to this PR because I rebased a few things.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 7, 2021

Hi, thanks for doing this! First, in terms of the conda recipe itself I have two comments:

  1. As you pointed out, mdolab-baseclasses should definitely be its own conda package and that should be easy since it's pure Python
  2. I would prefer a different way to configure setup.py to link to different libraries, but a patch is perhaps okay for now if we don't want to change the existing code. In general, I think what this illustrates is that we want a more flexible configuration option for IPOPT, such that the user is able to select the appropriate flags. Maybe this can be done via environment variables or something during building, which I think conda has the ability to set. I'll open an issue to track this but it doesn't really affect this particular work. This recipe also makes IPOPT a required dependency rather than optional, but maybe that's fine in the conda ecosystem.

Moreover, I've had some discussions with lab members regarding conda packages. Like you said, this would make the most sense to go into conda-forge, but the lab doesn't really have the resources to maintain conda packages at the moment. Would this be something that you're willing to do perhaps? If we can't find someone who's willing to maintain the conda package, then what we might do is accept this PR into another branch, and keep it there just so your work doesn't get lost, but the recipe itself won't be maintained or packaged.

@whophil
Copy link
Contributor Author

whophil commented Jun 7, 2021

It is possible to compile the IPOPT interfaces at build time, but not require the ipopt package during run-time, while constraining ipopt to a version if installed. (see https://conda-forge.org/docs/maintainer/adding_pkgs.html#constraining-packages-at-runtime). This might be the best solution here.

Regarding IPOPT's setup.py, I would prefer the patch solution for the 2.6.2 conda package so we can avoid the version bump churn. If future versions of setup.py fix this, we can certainly modify the recipe.

I am happy to submit the mdolab-baseclasses and pyoptsparse packages to conda-forge and to be listed as a maintainer. However, I can't commit to long term support and I'm really only a light user of pyoptsparse. So I'd feel better if there is at least one additional name from your group whose name I could add to the maintainers list.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 7, 2021

It is possible to compile the IPOPT interfaces at build time, but not require the ipopt package during run-time, while constraining ipopt to a version if installed. (see conda-forge.org/docs/maintainer/adding_pkgs.html#constraining-packages-at-runtime). This might be the best solution here.

This would be pretty cool. Since I'm very unfamiliar with conda, I have a few more questions (assuming we get this on conda-forge):

  • conda-forge will automatically build binaries linking to the conda-provided IPOPT package, right? The user just has to download the conda package and everything should work?
  • What about stuff like mac OS and Windows? will those be handled automatically in some way? Or maybe we can limit ourselves to just building on Linux for now.
  • I think the PR as it stands will need modifications if we are to submit to conda-forge? For example instead of specifying the source code relative to the YAML file, we have to specify some tarball from GitHub or something I guess. We can iron out the details if we wish to pursue this.

Regarding IPOPT's setup.py, I would prefer the patch solution for the 2.6.2 conda package so we can avoid the version bump churn. If future versions of setup.py fix this, we can certainly modify the recipe.

Yeah, I agree. This is a good solution for now, but I can try to add this feature to pyOptSparse in the future.

I am happy to submit the mdolab-baseclasses and pyoptsparse packages to conda-forge and to be listed as a maintainer. However, I can't commit to long term support and I'm really only a light user of pyoptsparse. So I'd feel better if there is at least one additional name from your group whose name I could add to the maintainers list.

Yeah that's totally reasonable. I think I can commit to this, and in the future hopefully others in the lab can be added too. I'm just hesitant because I (and others in the lab) have very limited experience with conda, but if this is mostly just maintaining some YAML files and the build/test process is largely automated, then I'm less worried.

@whophil
Copy link
Contributor Author

whophil commented Jun 7, 2021

I have experience with conda-packaging for internal deployment, but this would be my first time submitting to conda-forge. So prefix everything below with "AFAIK."

  • conda-forge will automatically build binaries linking to the conda-provided IPOPT package, right? The user just has to download the conda package and everything should work?

Yes, conda-forge will build the necessary interface binaries and bundle those (along with the python sources) into the conda package. When pyIPOPT is imported, it will try to import pyipoptcore which will look for the IPOPT library in the conda environment's lib folder. Conda makes sure that the library search paths are all OK.

  • What about stuff like mac OS and Windows? will those be handled automatically in some way? Or maybe we can limit ourselves to just building on Linux for now.

conda-forge supports multi-platform builds on a matrix of platforms which is defined along with the recipe. So we can have it build on whichever platforms this package supports. So there will be multiple versions of pyoptsparse-2.6.2 available on conda-forge corresponding to different platforms. The platform is just one of the matrix options - other common matrix options include BLAS implementation (openblas vs mkl)

  • I think the PR as it stands will need modifications if we are to submit to conda-forge? For example instead of specifying the source code relative to the YAML file, we have to specify some tarball from GitHub or something I guess. We can iron out the details if we wish to pursue this.

Yep, we'll need to make some minor changes. And mdolab-baseclasses will have to be added and released first, as it is a dependency of this project.

I'm just hesitant because I (and others in the lab) have very limited experience with conda, but if this is mostly just maintaining some YAML files and the build/test process is largely automated, then I'm less worried.

There shouldn't be too much babysitting or CI administration, as that's all handled by conda-forge. Hopefully it isn't too much work, and I will support the best that I can.

I'll put in the PR for mdolab-baseclasses today, and pyoptsparse whenever that one is merged in. I will list you as a maintainer on both and ping you on the PRs. Or would you prefer I use @pyoptsparse_maintainers?.

I'm happy to discuss any questions you have about those PRs, or conda packaging in general. I think it would be a huge benefit to the community if more of the mdolab tools were available on conda-forge, and this is probably a good first step.

I appreciate all the effort of you and your group. So thank you, and thanks for entertaining this!

@ewu63
Copy link
Collaborator

ewu63 commented Jun 7, 2021

There shouldn't be too much babysitting or CI administration, as that's all handled by conda-forge. Hopefully it isn't too much work, and I will support the best that I can.

I'll put in the PR for mdolab-baseclasses today, and pyoptsparse whenever that one is merged in. I will list you as a maintainer on both and ping you on the PRs. Or would you prefer I use @pyoptsparse_maintainers?.

I'm happy to discuss any questions you have about those PRs, or conda packaging in general. I think it would be a huge benefit to the community if more of the mdolab tools were available on conda-forge, and this is probably a good first step.

I appreciate all the effort of you and your group. So thank you, and thanks for entertaining this!

Sounds like a plan! I don't think the maintainer group will work since that's specific to the mdolab organization on GitHub. Please put nwu63, marcomangano and eirikurj as maintainers please.

No thank you for getting this started, it has been requested several times but we were sort of waiting for a PR since we were unfamiliar with conda ourselves. We can discuss the specifics on the PR to staged-recipes, but I imagine there would be issues with how we test our codes for example (ideally we force IPOPT tests under conda, which are optional right now). None of these are blockers however and we can get something that works first before we cover everything.

As for other mdolab codes, we had talked about that internally, but since it doesn't usually fall under our use case within the lab there has been low enthusiasm to do that. But I think pyOptSparse occupies a unique place since 1) it's used by many outside the lab, and 2) it would benefit a lot from conda. Let's start here and see where things go, it's possible that in the future we may support more packages.

@whophil
Copy link
Contributor Author

whophil commented Jun 9, 2021

The conda-forge recipe is now open here: conda-forge/staged-recipes#15239. Closing this PR!

@whophil whophil closed this Jun 9, 2021
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

2 participants