Add recipe for OpenMM #9102

jaimergp · 2019-08-09T10:51:42Z

Trying to build OpenMM in conda-forge with GPU support, following examples at #8637, #8638.

Checklist

License file is packaged (see here for an example)
Source is from official source
Package does not vend other packages
Build number is 0
A tarball (url) rather than a repo (e.g. git_url) is used in your
recipe (see
here
for more details)
GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

conda-forge-linter · 2019-08-09T10:52:21Z

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/nvcc, recipes/openmm) and found some lint.

Here's what I've got...

For recipes/openmm:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [41]

conda-forge-linter · 2019-08-09T12:16:27Z

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/nvcc, recipes/openmm) and found it was in an excellent condition.

jakirkham · 2019-08-12T13:52:09Z

Thanks for working on this @jaimergp! I'm trying to sort out how we generally handle variants for GPU package builds in issue ( conda-forge/conda-forge-pinning-feedstock#237 ), which should help us get this and other GPU packages in.

jaimergp · 2019-08-12T14:06:39Z

@jakirkham

Thanks for the tip! Following the issue now!

conda-forge-linter · 2019-08-12T16:09:21Z

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/nvcc, recipes/openmm) and found some lint.

Here's what I've got...

For recipes/openmm:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [34, 35, 40, 41, 45, 55]

conda-forge-linter · 2019-08-12T16:22:29Z

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/nvcc, recipes/openmm) and found it was in an excellent condition.

conda-forge-linter · 2019-08-19T11:22:34Z

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/nvcc, recipes/openmm) and found some lint.

Here's what I've got...

For recipes/openmm:

The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
The recipe could do with some maintainers listed in the extra/recipe-maintainers section.
Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [17, 18]
There are too few lines. There should be one empty line at the end of the file.
The extra section contained an unexpected subsection name. upload is not a valid subsection name.
The extra section contained an unexpected subsection name. force_upload is not a valid subsection name.

isuruf · 2019-10-02T16:29:29Z

Can you remove unnecessary changes like adding Nvidia channel? And also remove disabling error_overlinking

conda-forge.yml

jakirkham · 2019-10-02T16:52:47Z

.circleci/run_docker_build.sh

@@ -3,7 +3,7 @@
 # NOTE: This script has been adapted from content generated by github.com/conda-forge/conda-smithy

 REPO_ROOT=$(cd "$(dirname "$0")/.."; pwd;)
-IMAGE_NAME="condaforge/linux-anvil-comp7"
+IMAGE_NAME="condaforge/linux-anvil-cuda:10.0"


So this is fine for testing purposes, but we will need to revert this before merging.

You need to keep this, remove skipping, make tests pass. Once that is reviewed, we can revert this change and merge.

Since you are using 10.0 image, you should skip other versions in staged-recipes and only enable them in the feedstock.

.circleci/conda_build_config.yaml

jakirkham · 2019-10-02T16:54:46Z

recipes/openmm/conda_build_config.yaml

+cuda_compiler:
+- nvcc
+cuda_compiler_version:
+- 10.0


Do you support other CUDA versions? Or only 10.0?

Also do you want to have a CPU only build? Or are you only interested in building with GPU support?

We will add more CUDA versions, yes. I think we can do it like the ucx recipe, right?

OpenMM will just ignore CUDA/OpenCL if the platform is not compatible, so we don't need CPU-only packages, I'd say. We might be interested in building variants without the explicit cudatoolkit runtime dependency though, so people can use their own CUDA installation without downloading a several hundreds MB package. How are you handling this scenario right now?

By adding a None version and a selector to disable the CUDA compiler in that case.

Edit: Should add this is accompanied by the regular conda-forge Docker image.

But we still want to build the CUDA libraries in our package. We just want to ignore cudatoolkit for one of the variants so the user provides their own CUDA install (like in a cluster).

Ah. I'm not sure that we have a good way to solve this ATM. What is gained by using an existing CUDA install vs. using the cudatoolkit package?

I'd say there are two main reasons:

Avoiding the large cudatoolkit download.

Allowing HPC sysadmins to provide their own CUDA environment.

Maybe @peastman wants to add something else.

That sounds right.

Please raise an issue on the nvcc issue tracker. We can discuss this further there.

Done! conda-forge/nvcc-feedstock#14

jakirkham · 2019-10-02T16:55:12Z

recipes/openmm/conda_build_config.yaml

+cuda_compiler_version:
+- 10.0
+channel_targets:
+- conda-forge testing


What is this for?

We don't want packages in conda-forge/main without manually testing them with an actual GPU. Given the current OpenMM docs, conda-forge has precedence over omnia (where openmm resides now), and any new users that follow those instructions would obtain an untested (potentially broken) package.

Why does conda-forge have precedence over omnia? Would figure it would be the other way around?

A while back (a couple years ago now) when we were planning all this out, we wanted to eventually move every package over from omnia to conda-forge, so us omnia devs could eventually stop having to maintain a separate conda-based build system ourselves. The hard part of that was going to be resolving all the dependencies, which we had to maintain/copy a large number of them over to the omnia channel.

To prepare for this migration, we decided that conda-forge would be the de-facto dependency source, and we only maintained on omnia the minimum set of packages we had to. This basically meant that the packages which were were all OpenMM dependent at some level. The goal was, once OpenMM came to conda-forge, it would be very easy for all the other packages to migrate over as well since all they would have to do (in theory) was create a staged-recipe and the dependencies would basically already be resolved since we were relying on conda-forge as the higher priority channel anyways.

jakirkham · 2019-10-02T17:08:58Z

Thanks for continuing to push this forward, @jaimergp. Added a couple comments to guide the next steps here.

recipes/openmm/meta.yaml

jakirkham · 2019-10-03T16:10:06Z

recipes/openmm/meta.yaml

+  number: {{ build }}
+  skip: True  # [py2k or win]
+  missing_dso_whitelist:
+  - "*/libcuda.*"  # [linux]


Why is this needed?

Because we've enabled error_overlinking on staged-recipes and since /usr/local/lib/libcuda.so is outside of prefix, conda-build errors. Correct way to fix this would be to generate the stub library libcuda.so ourselves and make that a CDT.

Can you please raise an issue about this somewhere so we can follow-up? Maybe the nvcc feedstock or the webpage repo?

This is now being tracked in issue ( conda-forge/nvcc-feedstock#12 ).

jakirkham · 2019-10-03T16:12:58Z

recipes/openmm/meta.yaml

+    - numpy
+    - cython
+    # needed for Python wrappers
+    - doxygen 1.8.14


Why is this version needed?

@peastman: Do you recall which Doxygen versions work correctly with the OpenMM Swig system?

I don't know if this was in response to a particular problem. In general, every time we change versions of Doxygen or SWIG there's a good chance something will break. So when possible it's best to pin the build tools to particular versions we know work. We also should be pinning SWIG, since we know there's a bug in recent versions that breaks the OpenMM documentation (sphinx-doc/sphinx#6565).

Latest doxygen is v1.8.16, which breaks the build. 1.8.14 works correctly, that's why it's pinned. Maybe we can relax the pinning a bit (<=?). Regarding SWIG, we are not building the docs in this recipe, so we are not observing that bug, I'd say.

It would be better to be as permissive as we can, @peastman, since we could cause package conflicts if we pin to a single version.

For runtime dependencies we should be permissive. Build time dependencies don't cause problems for users, and we know from past experience that changes to the build tools have a very high probability of breaking things.

recipes/openmm/meta.yaml

jakirkham · 2019-10-03T16:25:34Z

recipes/openmm/meta.yaml

+  recipe-maintainers:
+    - jchodera
+    - jaimergp
+    - peastman


@jchodera @peastman , are you ok being maintainers on openmm?

jakirkham · 2019-10-03T16:27:52Z

Had a couple questions about the recipe generally, but I think this is largely looking good at this point. Thanks for your efforts and patience @jaimergp as we figure this out together. 🙂

Co-Authored-By: jakirkham <[email protected]>

jaimergp · 2019-10-04T08:58:36Z

I think I applied all suggested changes! Thanks a lot to @jakirkham and @isuruf for their valuable feedback!

Tests are passing and once we get the feedstock we will be able to test the packages locally.

isuruf · 2019-10-06T20:47:03Z

Looks good to me. Please remove https://github.com/conda-forge/staged-recipes/pull/9102/files#diff-ee85f7cbba6676fce7d05c46b9baff16R6 and we can merge this.

jaimergp · 2019-10-06T20:54:55Z

Thank you very much @isuruf! Suggestion applied!

isuruf · 2019-10-06T20:58:09Z

recipes/openmm/conda_build_config.yaml

+docker_image:
+# - condaforge/linux-anvil-cuda:9.2
+- condaforge/linux-anvil-cuda:10.0
+# - condaforge/linux-anvil-cuda:10.1


You can uncomment this in the feedstock. If you want to add more CUDA packages through staged-recipes, we'd appreciate some help in getting staged-recipes to play nicely with CUDA packages.

Have been meaning to raise an issue about this. Submitted issue ( #9773 ) for discussion and tracking work on this task.

Following issue!

jakirkham · 2019-10-17T18:43:20Z

@jaimergp, when you have a chance, I'd be interested in hearing about your experience adding a GPU-enabled package to conda-forge. Am planning to write up documentation on GPU builds ( conda-forge/conda-forge.github.io#901 ) and your feedback on what parts were tricky as well as what worked well would help shape this documentation. 🙂

jchodera and others added 2 commits June 15, 2019 12:27

Add OpenMM draft recipe

1172d13

Add experimental CUDA support for conda-forge

31aa301

jaimergp added 3 commits August 9, 2019 14:02

OpenMM was not being built

4930425

Match nvcc and cuda versions

ad1cdb9

build_all will take care of building nvcc first

9acf8fb

jaimergp added 4 commits August 9, 2019 15:17

Sync OpenMM build script with that in omnia-md/conda-recipes-gpu

f85d4a3

Provide clang for Linux

03980c9

Don't use clang on Linux

e4b1e9f

Don't pass gcc-toolchain to GXX

51012e0

jaimergp added 3 commits August 12, 2019 16:47

Manually specify CUDA_LIBRARY_DIR

3408f32

2nd workaround

4452fce

Try to use clang again

90e57e5

Satisfy linter and add channel_targets

e335cf8

jaimergp added 6 commits August 12, 2019 22:01

Force use clang

62063da

Provide gcc as well

b4330e7

Use Jinja functions to load nvcc

d3155ee

Add omnia channel temporarily

08603bd

Readd workaround for missing CUDA_CUDA_LIBRARY

67c5c08

Switch to gcc again

166f874

jaimergp mentioned this pull request Aug 15, 2019

Add OpenMM recipe (CPU only) [WIP] #9163

Closed

6 tasks

jaimergp added 2 commits August 19, 2019 13:19

This is building locally now... should work on Azure

b62975f

Merge branch 'openmm' of github.com:jaimergp/staged-recipes into openmm

4e4fe14

Satisfy linter

2dc74df

Apply suggestions by @isuruf

e552f21

isuruf reviewed Oct 2, 2019

View reviewed changes

conda-forge.yml Outdated Show resolved Hide resolved

Fix channel targets

3153210

jakirkham reviewed Oct 2, 2019

View reviewed changes

jaimergp changed the title ~~Add recipe for OpenMM [WIP]~~ Add recipe for OpenMM Oct 2, 2019

jakirkham reviewed Oct 2, 2019

View reviewed changes

.circleci/conda_build_config.yaml Outdated Show resolved Hide resolved

jakirkham reviewed Oct 2, 2019

View reviewed changes

jaimergp added 2 commits October 2, 2019 19:09

Revert changes in .circleci

a009256

Add more CUDA versions following ucx-split example

4152539

isuruf reviewed Oct 2, 2019

View reviewed changes

recipes/openmm/meta.yaml Outdated Show resolved Hide resolved

Apply suggestions by @isuruf

2fba82d

jakirkham reviewed Oct 3, 2019

View reviewed changes

recipes/openmm/meta.yaml Outdated Show resolved Hide resolved

jakirkham reviewed Oct 3, 2019

View reviewed changes

jaimergp and others added 3 commits October 3, 2019 23:37

Cython shouldn't be a runtime dependency

66e1cc9

Co-Authored-By: jakirkham <[email protected]>

Skip builds for cuda != 10.0 for now

0d399f1

Skip CUDA!=10 at conda_build_config

0a53ce7

Revert hardcoded Docker image for CUDA

11eb8a3

isuruf merged commit 963af5f into conda-forge:master Oct 6, 2019

isuruf reviewed Oct 6, 2019

View reviewed changes

jaimergp mentioned this pull request Oct 9, 2019

Strategy for conda-forge packages openmm/openmm#2401

Closed

Add recipe for OpenMM #9102

Add recipe for OpenMM #9102

Conversation

jaimergp commented Aug 9, 2019 • edited Loading

conda-forge-linter commented Aug 9, 2019

conda-forge-linter commented Aug 9, 2019

jakirkham commented Aug 12, 2019

jaimergp commented Aug 12, 2019

conda-forge-linter commented Aug 12, 2019

conda-forge-linter commented Aug 12, 2019

conda-forge-linter commented Aug 19, 2019

isuruf commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Oct 3, 2019

jaimergp commented Oct 4, 2019

isuruf commented Oct 6, 2019

jaimergp commented Oct 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Oct 17, 2019

jaimergp commented Aug 9, 2019 •

edited

Loading

jakirkham Oct 2, 2019 •

edited

Loading