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

libint: expose tunable configuration options for integral generation #202658

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

sheepforce
Copy link
Member

@sheepforce sheepforce commented Nov 24, 2022

Description of changes

Libint for integrals in quantum chemistry can be built in a variety of ways. Up to now, only one has been supported suitable for CP2K included in Nixpkgs. However, other quantum chemistry programmes require very different settings. This PR exposes the majority of libints configuration options within reasonable constraints, so that multiple versions can be built. More integrals are built by default now (for example one-electron integrals and multipole integrals).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 24, 2022
@sheepforce sheepforce force-pushed the libint branch 2 times, most recently from 76d63d8 to 691c92d Compare December 1, 2022 19:00
@ofborg ofborg bot requested a review from markuskowa December 1, 2022 19:02
@sheepforce sheepforce force-pushed the libint branch 6 times, most recently from dffe732 to d220005 Compare December 2, 2022 17:03
@sheepforce
Copy link
Member Author

This one is nice. It builds relatively fast and still covers all use cases. Tested cp2k with HF+ADMM and works as expected. The psi4 version works out of the box for Psi4 1.6

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Those are to many options and I am against merging this in the current state.

I would be fine with having an easier option to overwrite the configureFlags or a very small subset of this.

pkgs/development/libraries/libint/default.nix Show resolved Hide resolved
pkgs/development/libraries/libint/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libint/default.nix Outdated Show resolved Hide resolved
libint: more conservative optimisation settings


libint: fix integral angular momentum


remove leftover log file


libint: more options


libint: more options


libint: more options


libint: more fixes


libint: more fixes


libint: more fixes


libint: more fixes


libint: more fixes


libint: more configuration options


libint: psi4-compatible alias


libint: 2.7.1 -> 2.7.2 + expose build options


libint: fix formatting


libint; change psi4 settings


libint: adapt psi4 settings


libint: fix typo


libint: fix compilation


libint: disable SSE for Psi4


libint: review changes
@sheepforce
Copy link
Member Author

Those are to many options and I am against merging this in the current state.

I would be fine with having an easier option to overwrite the configureFlags or a very small subset of this.

Certainly, and I would be a much in favour of handling less myself. Unfortunately libint is used in a large variety of variants and we are not the first bitten by it:

https://github.com/evaleev/libint/wiki
evaleev/libint#190

Unfortunately again, it is a quite performance critical part of quantum chemistry.

While there are many options now I've went some way to ensure only reasonable combinations can be specified. Then again I see no harm more options cause to Nixpkgs.

@markuskowa what is your opinion on this?

@SuperSandro2000
Copy link
Member

Unfortunately again, it is a quite performance critical part of quantum chemistry.

nixpkgs is a general purpose linux distribution. If you need really specific compile flags to that level and granularity that cannot be generalized, I would highly recommend to start a side project that can fulfill that exact need. Then you most likely also want to compile with AVX which we are not going to do for a while.

Then again I see no harm more options cause to Nixpkgs.

It is a maintenance burden and makes the code more complex than it needs to be. Also those flags are not tested or understood by most maintainers so the package will be less good maintained and in case of breakages likely just marked broken.

@markuskowa
Copy link
Member

markuskowa commented Dec 3, 2022

@SuperSandro2000 I usually would agree with you that this doesn't look too good. However, this package is a bit of an exception. I am in favor of exposing these configuration options. This is a library that is used by multiple theoretical chemistry packages and with many requiring the package configured slightly differently. I would say that providing that flexibility it is strength of nix. Other distribution have a hard time doing that.

nixpkgs is a general purpose linux distribution. If you need really specific compile flags to that level and granularity that > cannot be generalized, I would highly recommend to start a side project that can fulfill that exact need.

There a lot of specialized packages in nixpkgs, including a lot of scientific software packages. This probably even one of the less exotic ones.

It is a maintenance burden and makes the code more complex than it needs to be. Also those flags are not tested or understood by most maintainers so the package will be less good maintained and in case of breakages likely just marked broken.

I actually agree here with @sheepforce that there is no harm done of trying this version and see how it holds up. If it turns out to not be a good solution in practice, we simply change it to something simpler. Regarding maintenance: this change is proposed a maintainer of the package. This is a well maintained packaged that will certainly not be left behind as a "marked broken" package.

@SuperSandro2000
Copy link
Member

we simply change it to something simpler.

Removing options afterwards is always harder. How about we use this feature somewhere and only implement what we truly need?

@sheepforce
Copy link
Member Author

sheepforce commented Dec 7, 2022

@SuperSandro2000 I really appreciate your feedback. In this case, however I think this version is the best option forward. I will try to give some context.

I would highly recommend to start a side project that can fulfill that exact need.

Indeed we have such a side project (https://github.com/markuskowa/NixOS-QChem) and libint originated from there. We are gradually upstreaming stuff suitable for nixpkgs. Libint is a quite basic building block for quantum chemistry, is widely used and works generally well.

Then you most likely also want to compile with AVX which we are not going to do for a while.

We are doing so in NixOS-QChem by modifying the stdenv for several packages and/or directly passing the AVX options similar to how it is done in fftw.
For libint AVX has been advertised and can be enabled in the build system. However, libint completely breaks when doing so. Even SSE2 breaks most builds. Basically no downstream project is using it. Thus, in this specific case vectorisation does not really matter.

Also those flags are not tested or understood by most maintainers so the package will be less good maintained

That is probably correct. However, this is also quite hard and I've tried to document them. These things have a physical meaning. They are not merely changing something in the build. The reason is, that libint builds in two phases. Phase 1 is the libint generator. Basically a computer algebra system which code-generates many millions of specific cases in the Obara-Saika recursion scheme required to solve molecular integrals. It absolutely hardcodes every single case and there are many, here is some overview: http://www.zhjun-sci.com/software-libreta-os.php. The flags control (via the build system!) how this equation (see link) is solved. There is no easy way around knowing what this equation does and give it reasonable parameters. We simply can not expect this to be understandable by each maintainer in Nix. The build system is basically already doing quantum mechanics. However, they are tested via assertions. There are simply impossible combinations of values and I catch them.

the package will be less good maintained and in case of breakages likely just marked broken.

Oh, if libint breaks we will certainly notice. Half of our research group indirectly depends on libint and a breakage will have a noticable effect. :) I think its the same for @markuskowa

If you need really specific compile flags to that level and granularity that cannot be generalized

It cannot be generalised for reasons explained above. The version we had in nixpkgs previously is suitable for CP2K (also in nixpkgs) but for nearly no other quantum chemistry package. It is not myself who requires this level of granularity, it is simply how quantum chemistry is being built. Currently, I have introduced the libintPsi4 alias (and other QC could use others, perspectively)
Debian has this problem as well as Fedora. Debian for instance, builds libint2 in a version suitable just for CP2K. Psi4 uses deprecated libint1, as they cannot have two versions of libint. MPQC in Debian is stuck with an old version, as this requires libint with yet another set of configuration parameters.

  • eriDeriv (and eri2Deriv, eri3Deriv) - Changing one of those values will result in a segfault at runtime for some specific jobs at runtime in Psi4. Increasing those numbers to a higher value will easily make the code generation phase require multiple days to run.
  • eriAm and the others describe for which cases the Obara-Saika scheme is solved, depending on the values given in eriDeriv. Changing one of those numbers will lead to a segfault at runtime for some specific chemical elements. One can increase these values but those are exotic calculations then. Again we would require multiple days of build time.
  • enableOneBody = true will break CP2K but is required for Psi4
  • enableContracted completely changes the way molecular integrals are handled and is individual for each package. There is no "one fits all" for this option.
  • cartGaussOrd, shGaussOrd, eri2PureSh and eri3PureSh are especially insidious. They refer to different conventions how integrals are stored in different quantum chemistry programmes. Changing those will not lead to a runtime crash, compilation error or something like that. Instead the wave function diverges in a non-obvious way and gives subtly wrong or very wrong results, depending on the molecule. An unreactive molecule may become an explosive or vice versa. Who knows.

In conclusion, there is no way to correctly set these parameters without some knowledge of quantum chemistry and what each of the individual programmes utilising libint does in detail, I am afraid.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 7, 2022

Okay fine but if any work needs to be done then we should leave that up to you two and otherwise mark it broken.

Also building the package timed out on ofborg after one hour.

@sheepforce
Copy link
Member Author

Perfectly fine for me, that is why we are listed as maintainers :)

Yes, even here where I've tried much to reduce build time requires at least two hours. The code generation step is unfortunately completely serial. Bigger builds can take more than three days, so this is the best I can do, I am afraid.

@SuperSandro2000
Copy link
Member

Then we should set meta.hydraPlatforms to an empty list because it will likely also time out on hydra.

@markuskowa
Copy link
Member

@SuperSandro2000 ofBorg has relatively short timeout. It should be no problem on Hydra though. The build now should be on the order of about 2 hours.

@SuperSandro2000
Copy link
Member

then lets try that

@SuperSandro2000 SuperSandro2000 merged commit 8a6e684 into NixOS:master Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants