Add double precision builds

mattwthompson commented 2 years ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[ ] Reset the build number to 0 (if the version changed)
[ ] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[ ] Ensured the license file is being packaged.

conda-forge-linter commented 2 years ago

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 (recipe) and found it was in an excellent condition.

mattwthompson commented 2 years ago

@conda-forge-admin please rerender

mattwthompson commented 2 years ago

@conda-forge-admin please rerender

douglowe commented 2 years ago

I think the build is failing because of this error:

CMake Error at cmake/gmxManageOpenCL.cmake:42 (message):
  OpenCL acceleration is not available in double precision

Perhaps there is a build flag that can be switched on for the double precision build to turn off OpenCl support?

In addition - can you add to the tests in the meta.yaml file - to cover the new executables (or these will cause failures later)?

test:
  commands:
    - gmx -version  # [mpi == 'nompi']
    - export OMPI_MCA_plm=isolated  # [mpi == "openmpi" and linux]
    - export OMPI_MCA_btl_vader_single_copy_mechanism=none  # [mpi == "openmpi" and linux]
    - export OMPI_MCA_rmaps_base_oversubscribe=yes  # [mpi == "openmpi" and linux]
    - gmx_mpi -version  # [mpi != 'nompi']

douglowe commented 2 years ago

Also, we should think about package priority in the conda-forge channel.

In the bioconda build (for a short while) we had this code in the meta-yaml file:

{% set build = 1 %}
{% set build = build + 100 %}  # [mpi == 'nompi']

And this was referenced when creating the build string.

I think we need similar here, if 4 different packages are going to be provided. I think a sensible priority order would be: nompi; nompi_dbl; mpi; mpi_dbl.

mattwthompson commented 2 years ago

if 4 different packages are going to be provided

Just to reiterate, I only need one double precision builds, and not for production runs. So my use case would be satisfied only adding serial double precision builds. If the community would at all benefit from parallel double precision builds we can include those, but if nobody would use them and it makes packaging harder, I'd rather not include them.

It comes down to what users want - I don't personally know, and I'm hoping somebody more looped in with the user base has their ear to the ground.

mattwthompson commented 2 years ago

Perhaps there is a build flag that can be switched on for the double precision build to turn off OpenCl support?

The docs don't say -DGMX_GPU=OFF is allowed, but it's an option I swear I used in the past and is floating out there online, so I'll give it a whirl.

mattwthompson commented 2 years ago

@conda-forge-admin please rerender

douglowe commented 2 years ago

if 4 different packages are going to be provided

Just to reiterate, I only need one double precision builds, and not for production runs. So my use case would be satisfied only adding serial double precision builds. If the community would at all benefit from parallel double precision builds we can include those, but if nobody would use them and it makes packaging harder, I'd rather not include them.

It comes down to what users want - I don't personally know, and I'm hoping somebody more looped in with the user base has their ear to the ground.

Fair point - I jumped the gun on talking about 4 builds.

I don't think including the extra mpi_d build would make packaging any more complicated than it is already (we still should adjust the version numbers to give consistent package priority, whether we have 2, 3, or 4 builds). Main question would be the build time, and if we would risk hitting any resources limits for the conda-forge infrastructure if another build is added in.

For user want - I don't know the community well enough to know if there is demand. But, "build it, and they will come".

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/gromacs-feedstock/actions/runs/2023598747.

douglowe commented 2 years ago

It's looking good.

I've setup the build number bump for the nompi / single precision build (https://github.com/mattwthompson/gromacs-feedstock/pull/1). Thinking about priority more - we only need to promote our favoured package, the order of the rest should be irrelevant.

mattwthompson commented 2 years ago

@conda-forge-admin please rerender

mattwthompson commented 2 years ago

Hmm, I'm not getting a CI re-trigger. https://conda-forge.org/status/ shows me green lights, as does https://status.dev.azure.com/ - but oh, GitHub is having a grumpy time today, that's probably it https://www.githubstatus.com/

Locally re-rendering shows everything is up-to-date, so I expect the bot will figure that out soon as well.

mattwthompson commented 2 years ago

Thanks!

eirrgang commented 2 years ago

I don't propose to expand the matrix. I think it would be a good idea to either prevent the combination of double and mpi, or to otherwise avoid confusing results if someone attempts such a build without further updates to the script.

On Mon, Mar 21, 2022, 19:22 Matt Thompson @.***> wrote:

@.**** commented on this pull request.

In recipe/build.sh https://github.com/conda-forge/gromacs-feedstock/pull/1#discussion_r831298338 :

@@ -64,6 +69,9 @@ if [ "${mpi}" = 'nompi' ] ; then else gmx='gmx_mpi' fi +if [ "${double}" = 'yes' ] ; then

That was intentional - the use case I'm trying to support needs double precision but does not need MPI, and I thought adding one row to the list of variants (using https://docs.conda.io/projects/conda-build/en/latest/resources/variants.html#coupling-keys) would be cheaper than adding in an extra dimension to the build matrix - both now and if the matrix expands further in the future. If double precision with MPI support is something that people would use, I could switch that over. I just didn't want to double the build time for my one-off use.

— Reply to this email directly, view it on GitHub https://github.com/conda-forge/gromacs-feedstock/pull/1#discussion_r831298338, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILPWAMM5QBRR54AP3M7W3VBCO5ZANCNFSM5RIC6Y6A . You are receiving this because you commented.Message ID: @.***>

mattwthompson commented 2 years ago

Memory is fuzzy but IIRC I only need one double-precision build build and only for testing purposes, not necessarily for production runs

conda-forge / gromacs-feedstock

Add double precision builds #1

@.**** commented on this pull request.