phonopy/phono3py

Cannot pip download source

Opened this issue · 15 comments

ccoulombe commented

Describe the bug
The setup.py requires an empty site.cfg. When running pip download phono3py, it crashes since there's no config file.

phono3py/setup.py

Line 8 in 6c4c561

if not os.path.exists("site.cfg"):

To Reproduce

$ pip download phono3py==2.3.0
Collecting phono3py==2.3.0
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  ERROR: Command errored out with exit status 1:

Solutions

  • Remove check for an empty site.cfg
  • Provide an empty site.cfg
atztogo commented

It is needed a way to tell users to avoid using single-threaded phono3py.

But I never considered your case. I will follow

Remove check for an empty site.cfg

Do you want to the release as soon as possible?

ccoulombe commented

Would providing a site.cfg with openmp works? Or adding the extra cflag in the setup.

I built the wheels using the sources from Github so I do not need the release as soon as possible, thanks though!

atztogo commented

There are several flags of openmp for different compilers and environments.
From the point in PR #60, no default setting is expected. But empty site.cfg results in phono3py without openmp, which is too slow to use. So I added the check in setup.py. But as you suggested, raising exception in setup.py is clearly a bad practice. What I am considering now is in run time to warn if single thread version phono3py is found.
Thanks for your advice.

ccoulombe commented

Suggestion as seen in many other setup.py. The OpenMP flag could be provided by default for the main compilers and their platform, with the flexibility to define or override with a site.cfg file.

For instance, on Linux platform with gcc the flag is -fopenmp. But if one provide a site.cfg, then use it.
(As long as this is clearly documented in the build/install instructions.)

atztogo commented

How do you think @zerothi?

ccoulombe commented

Circling back to my previous suggestion, this could be simpler by providing by default a site.cfg with OpenMP assuming the compiler is GCC such as mentioned in the raised exception from the setup.py. Commented into the file could be the other varian such as for clang, etc.

As long as all this is documented!

# This assume that the compiler is GCC
"[phono3py]",
        "extra_compile_args = -fopenmp",
zerothi commented

My only objection would be that a default site.cfg will have the same problem when people are using compilers that don't support the -fopenmp flag. NAG for instance (I don't know if they have made a redirect yet).

Another route would be to default the -fopenmp flag, and let another command line option redefine the openmp flag, that can still be done using pip.

I can see that @atztogo changed it to fail if site.cfg was not present. That was not part of my original re-implementation.

ccoulombe commented

My suggestion is to keep things simple since the preference is to build with OpenMP by default. As long as it is documented, it is not an issue (imho) to edit a configuration file to suit the current compiler/platform.
As with Numpy site.cfg, one can still provide the different configuration but commented and document how to use it.

atztogo commented

I can see that @atztogo changed it to fail if site.cfg was not present.

If someone automates the deployment of phono3py, their system could produce phono3py without openmp, but the maintainer might not realize the fact (most probably their tests pass). This situation is expected to be avoided. So some way to inform this transition of compilation way was needed. Documentation may be read when people find the problem, but the problem is silent, it is not easy to lead people to the documentation.

atztogo commented

Probably we have to give up site.cfg approach due to deprecation of numpy.distutils.

ccoulombe commented

In order to keep things simple: adding the flag to the setup, you can output a clear message to the user that the flag was added and directing him to the documentation.

atztogo commented

@ccoulombe, thank for your comment. What do you mean about "the flag"? Could you explain a bit more detail?

ccoulombe commented

The flag we are discussing is -fopenmp or its equivalent. What I am suggesting is doing as Pytorch: https://github.com/pytorch/pytorch/blob/937ca69f15f8f1c8516c7d4cd86bdcaeb765a23c/setup.py#L758
In order to force the compilation with openmp by default, for various platforms
Something like :

if is windows:
extra_compile_flags += /openmp
else:
extra_compile_flags += -fopenmp

And you can add a map of compilers and their respective flag : {'gcc': '-fopenmp', 'clang': '-fopenmp', 'icc': '-openmp'}

But with the deprecation of distutils, it might be a very good opportunity (and cleaner) to switch to Meson and ask it for OpenMP
https://mesonbuild.com/Dependencies.html#openmp

Another option, could be user options as pyFAI does: https://github.com/silx-kit/pyFAI/blob/819aa277c637d13388a72f4e513367bd87279e46/setup.py#L482

You could even combine both (option and meson) as scipy has done:
https://github.com/scipy/scipy/blob/main/meson.build
https://github.com/scipy/scipy/blob/main/meson_options.txt

atztogo commented

Thanks @ccoulombe for detailed information that helps me a lot. I will consider how we should do among those options.

atztogo commented

I could find some time and energy to challenge this issue. The comments above were very helpful and useful to start since this kind of information is fairly limited. The following is just for my memorization of my attempt.

As the initial attempt, I employed cmake rather than meson. The reason I didn't use meson was just that I am a little bit familiar with cmake. If this approach works, I consider (hope) that it will be easy to move to meson.

Automation and customization are needed in the build process under our many constraints. This attempt was/is the former. Still it is under the development. The latter will be next.

Basic idea

  • Cmake is called in setup.py using subprocess. The temporary working directory is _build.
  • Codes in C are made as static-link libraries that are compiled using cmake.
  • Python-C interfaces (c/_phono3py.c, c/_phononmod.c) are complied by setuptools that is invoked in setup.py and the libraries made by cmake are linked with them.

Use of find_package in cmake for finding external libraries

  • OpenMP, BLAS, and LAPACK libraries and their required compile-flags are searched by using find_package.
  • Information found are sent to stdout (using message of cmake) and subprocess receives them as strings.
  • The information from cmake is used for linking the external libraries by setuptools.

Things to do

  • Currently no means of customization by users such as proposed by @ccoulombe just above.