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 PCMSolver #22286

Merged
merged 48 commits into from
May 26, 2023
Merged

Add PCMSolver #22286

merged 48 commits into from
May 26, 2023

Conversation

loriab
Copy link
Contributor

@loriab loriab commented Mar 14, 2023

PCMSolver is a quantum chemistry library with parts in C++, Fortran, and Python. Pretty much this recipe is RTG, but a few things I need to address before requesting review.

  • package v1.2.x or v1.3.x ? get my cmake/windows changes into upstream. Ans: from primary developer's response below, we're going to package v1.2.x for c-f and then update to v1.3.1 once that release gets finalized
  • see if RDR wants to be maintainer. Ans: yes (see below)
  • release the constraint on one Py that's lowering build times for iteration
  • decide whether to package exe or just copy it for tests section. Ans: packaging exe until there's a reason not to
  • only W Fortran compiler I could find that worked was ming. will I be able to link Psi4 built with clang-cl with this? Ans: history with dkh in Adding psi4 and dkh #22328 that's also Fortran inspires confidence that PCMSolver built with ming and with GNUtoMS will work with Psi4.

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source. (Yes, approximately. It's the latest in the last release series plus some Windows, syntax, and cmake modernizations cmake modernization to 1.2.x PCMSolver/pcmsolver#203)
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged. (Static libs are purged)
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • 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.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

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/pcmsolver) and found some lint.

Here's what I've got...

For recipes/pcmsolver:

  • The top level meta key run is unexpected
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • If python is a host requirement, it should be a run requirement.
  • pin_compatible should be used instead of pin_subpackage for libpcmsolver because it is not a known output of this recipe: ['pcmsolver'].

For recipes/pcmsolver:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-webservices
Copy link

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/pcmsolver) and found some lint.

Here's what I've got...

For recipes/pcmsolver:

  • The top level meta key run is unexpected
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • If python is a host requirement, it should be a run requirement.

For recipes/pcmsolver:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-webservices
Copy link

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/pcmsolver) and found some lint.

Here's what I've got...

For recipes/pcmsolver:

  • The top level meta key run is unexpected
  • If python is a host requirement, it should be a run requirement.

@conda-forge-webservices
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/pcmsolver) and found it was in an excellent condition.

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

It looks like your python module is pure python. If so, Could you please separate the python module into a noarch: python output so that it is built once instead of for each python version?

An example recipe of this is tomopy.

I'm not exactly sure of the dependency relationship between your various outputs, but you would have two or three outputs: pcmsolver, libpcm, pcmsolver-python(?).

doc_source_url: https://github.com/PCMSolver/pcmsolver/tree/master/doc
license: LGPL-3.0-only
license_url: https://opensource.org/license/lgpl-3-0/
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.

You need to list the license files of the projects in /external, they are not all LGPL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I never noticed libtaylor was brought in as a header-only library.

  • Catch -- only used in tests so never becomes part of the project, so I didn't add its license
  • eigen -- I switched to forcing it to detect the conda pkg, so the external/ copy no longer used
  • boost -- same situation as eigen
  • libtaylor -- MIT

Then I started hunting through the source a little more and found 10 in total, so I added a THIRD-PARTY-LICENSE file to the upstream repo to cover them all.

dev_url: https://github.com/PCMSolver/pcmsolver
doc_url: https://pcmsolver.readthedocs.io/en/stable/
doc_source_url: https://github.com/PCMSolver/pcmsolver/tree/master/doc
license: LGPL-3.0-only
Copy link
Member

Choose a reason for hiding this comment

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

Use "AND" to include the license types of the projects in /external

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGPL3 + MIT + MIT-0 should cover it.

@loriab
Copy link
Contributor Author

loriab commented Apr 19, 2023

It looks like your python module is pure python. If so, Could you please separate the python module into a noarch: python output so that it is built once instead of for each python version? I'm not exactly sure of the dependency relationship between your various outputs, but you would have two or three outputs: pcmsolver, libpcm, pcmsolver-python(?).

Ordinarily I'm all for multiple outputs so the user can choose the base library or the language bindings. This project's an odd case where the python files are parsing helpers for the standard input mode of the compiled library (https://pcmsolver.readthedocs.io/en/stable/users/input.html#input-description). So, by my understanding, the full library plus py helpers are the only package that's useful to users. I could still split them up with a dummy (=user never directly installs) py package, but I think that's going to be hard to bring about cleanly and still do the main ctest testing in build.sh/bld.bat that itself needs Python (https://github.com/PCMSolver/pcmsolver/blob/master/tests/input/input_cpcm.cpp#L42-L48). I understand the CI time minimization motivation, but this project is in maintenance mode (last release in 2020 https://github.com/PCMSolver/pcmsolver/tags), so it's not going to be eating up resources. I don't like to push back on such a reasonable request, but I don't think this is a great candidate for a split-output recipe. What do you think?

@conda-forge-webservices
Copy link

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/pcmsolver) and found some lint.

Here's what I've got...

For recipes/pcmsolver:

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

For recipes/pcmsolver:

  • It looks like the 'libpcm' output doesn't have any tests.

@carterbox
Copy link
Member

Looks like the python scripts are pre-processors/parsers for the run_pcm binary. Why would users of the C/Fortran API who link dynamically to libpcm.so.1 need the python scripts?

We can also ask conda to do the splitting if the build system isn't set up to break the installation into multiple steps. I've added a commit which demonstrates this kind of syntax. You will need to move some of the existence tests around and add platform selectors so that libpcm is correctly split on Windows.

You'll also need to use the canonical repository for the source download not your personal one. Conda recipes can apply git patches if needed.

@loriab
Copy link
Contributor Author

loriab commented Apr 25, 2023

Looks like the python scripts are pre-processors/parsers for the run_pcm binary. Why would users of the C/Fortran API who link dynamically to libpcm.so.1 need the python scripts?

It's actually the run_pcm binary that's optional, and the python scripts are the parsers for the library-without-binary. As far as I know, one uses the API for the overall commands and the Python for the options parsing. I suspect this came about so the project could service multiple downstream packages that were in different languages and very set in their ways. But I shouldn't be troubling you with all that. I'll fix it up so it looks more canonical.

We can also ask conda to do the splitting if the build system isn't set up to break the installation into multiple steps. I've added a commit which demonstrates this kind of syntax. You will need to move some of the existence tests around and add platform selectors so that libpcm is correctly split on Windows.

Will do. Your proposal is certainly the more canonical packaging arrangement.

You'll also need to use the canonical repository for the source download not your personal one. Conda recipes can apply git patches if needed.

Ok, I'll drop it all into a patch. I think the upstream dev / co-recipe-maintainer will mint a v1.2.4 once the buildsystem changes settle down.

Thanks for all your comments!

@conda-forge-webservices
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/pcmsolver) and found it was in an excellent condition.

@loriab
Copy link
Contributor Author

loriab commented May 2, 2023

I think your concerns are addressed, @carterbox, and this is ready for consideration again. No particular urgency on this PR. I'm curious to see how a noarch generates here, as each arch has a different build string for the py package.

@carterbox
Copy link
Member

@conda-forge/help-c-cpp, please review

@loriab
Copy link
Contributor Author

loriab commented May 16, 2023

In case you, too, are worried about selectors in a noarch like https://github.com/conda-forge/staged-recipes/pull/22286/files#diff-793925cb8d7cb1c1f77b1a259178a638e058b7f01abf7a1b60d91127c8994b1eR41-R54 (the windows tests don't work without them), I can see a few strategies:

  1. Adopt the semi-noarch approach characterized by __win and described here. My concern with that is it's designed for arch-dependent dependency packages, not arch-dependent file positions (like the python script in bin/).
  2. Drop the noarch but separate the py packages into a per-arch per-py setup that only copies files. Model: https://github.com/conda-forge/libxc-feedstock/blob/main/recipe/meta.yaml#L138-L140 . That would involve 3 expensive library builds (one per arch) and 12 cheap py copies (one per arch per py).
  3. Merge the "noarch" as-is and hope my worries are mistaken.
  4. Revert to 3224aef where pcmsolver is monolithic and wasteful of CI but self-contained.

I'm glad to do however you advise, @carterbox . Sorry this package is so peculiar.

@carterbox
Copy link
Member

Because the recipe has arch-specific outputs (libpcm), the noarch: python package (pcmsolver) will be built on all platforms. If the package hashes collide, then there will be only pcmssolver variant available for download. This is fine if the noarch packages are being built correctly because they will be valid installs on all platforms. We can download the artifacts from the builds on azure to check if this is the case.

If the packages are different, then we go with Option 1 (one pcmsolver for each platform shared between many python versions).

@carterbox
Copy link
Member

The contents of the noarch package generated by Linux seems correct. Let's just check that the package hashes are the same for all platforms from the logs.

@carterbox
Copy link
Member

@loriab, I'm pretty sure one noarch package will work for all platforms. If you have problems, revert to Option 1 by removing the custom build string and adding

      - __linux  # [linux]
      - __osx    # [osx]
      - __win    # [win]

To the run requirements.

@carterbox carterbox merged commit d17ef5a into conda-forge:main May 26, 2023
@loriab
Copy link
Contributor Author

loriab commented May 27, 2023

Sorry I missed your recent messages, @carterbox. Thanks for sharing the info on noarch strategies. I'll test pcmsolver with psi4 to confirm the unified py build string is acting as it ought and, if not, switch to the __win strategy you suggest. Thanks for the review and merge.

@loriab loriab deleted the loriab-pcmsolver branch May 27, 2023 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants