fujiisoup / py3nj

Wigner's 3J, 6J, 9J symbols for python
https://py3nj.readthedocs.io/
Apache License 2.0
18 stars 5 forks source link

Fix 'pip install' for pip>=23.1 #23

Closed kalekundert closed 1 year ago

kalekundert commented 1 year ago

Trying to install py3nj with pip>=23.1 results in the following error:

$ python -m venv pip-23.1
$ . pip-23.1/bin/activate
$ pip install pip==23.1 numpy
$ pip install --no-cache-dir py3nj
Collecting py3nj
  Downloading py3nj-0.1.2.tar.gz (28 kB)
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error

  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [17 lines of output]
      Traceback (most recent call last):
        File "/home/kale/research/software/forks/py3nj/pip-23.1/lib/python3.10/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/home/kale/research/software/forks/py3nj/pip-23.1/lib/python3.10/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
        File "/home/kale/research/software/forks/py3nj/pip-23.1/lib/python3.10/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
          return hook(config_settings)
        File "/tmp/pip-build-env-esw66jjg/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 355, in get_requires_for_build_wheel
          return self._get_build_requires(config_settings, requirements=['wheel'])
        File "/tmp/pip-build-env-esw66jjg/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 325, in _get_build_requires
          self.run_setup()
        File "/tmp/pip-build-env-esw66jjg/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 507, in run_setup
          super(_BuildMetaLegacyBackend, self).run_setup(setup_script=setup_script)
        File "/tmp/pip-build-env-esw66jjg/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 341, in run_setup
          exec(code, locals())
        File "<string>", line 5, in <module>
      ModuleNotFoundError: No module named 'numpy'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

The problem is related to the fact that setup.py needs to be able to import numpy in order to create the fortran modules. This is actually an issue for all versions of pip; if you try to install py3nj into an environment without numpy installed, you'll get the same error message. In fact, the discussion in #17 diagnoses this exact problem, and also proposes the exact solution implemented by this PR. But it's more urgent now, because it used to be that everything would work so long as numpy was installed.

What changed in pip>=23.1 is that packages are now built in their own virtual environments, isolated from any user-installed packages. See the CHANGELOG, and two relevant pull requests: pypa/pip#8368 and pypa/pip#8559. This means that there's no longer any way for users to provide the numpy dependency (note that installing numpy before py3nj in the above snippet didn't avoid the error), so py3nj needs to tell pip to include it in the build environment. This is done using the [build-system] section of the pyproject.toml file.

While I was at it, I moved most of the metadata from setup.py to pyproject.toml, and got rid of the now-redundant setup.cfg. I think this is considered best-practice, because it's easier for third-party tools to extra project metadata from TOML files than python scripts. I also noticed that setup.py specified a BSD 3-clause license, while the actual LICENSE file in the repository contains the Apache license. I assumed that the setup.py file was wrong, and changed it to just reference the LICENSE file.

fujiisoup commented 1 year ago

Thank you, @kalekundert ! I did not realize this issue.

This sould be a must-have update, but now I think it is a good time to reactivate the CI Previously the CI for py3nj was running on travis but because of the change in github I have needed to update it for a long time.

I am sorry but I pushed some files to trigger github actions.

Currently, the installation was not successful on the github-actions' server. Would you mind to update this PR so that the installation on that server becomes successful?

~I observed that with python=3.6, the installation succeeded, but not sure the reason of unsuccessful build for the newer versions.~ It seems wrong, and I should stop working on your branch.

fujiisoup commented 1 year ago

The installation error in the CI comes from this part https://github.com/kalekundert/py3nj/blob/b968c277e9df1c9507426c8064234b1cf9e6c8aa/.github/workflows/ci.yaml#L51

It looks like that a library requires fortran compilation does not support -e option, so we may need to remove this option.

          pip install --no-deps .;
          python -m pip install --no-deps .

Then, I still see an error in import py3nj line, which complains a circular import. It looks like that import py3nj tries to import the directory py3nj, not the installed package.

A possible solution would be

You can find an example for this modification in my fujiisoup:fix-install branch, but if you know better solution, feel free to ignore this.

kalekundert commented 1 year ago

Thanks for such a quick response. And feel free to make as many commits as you'd like to my branch.

I didn't test editable installs, so I'll have to look and see if there's a way to get that to work.

Regarding the error with the import py3nj line, I had the same error when I was testing my PR, and I found that I could avoid it simply by changing to any other directory. That's why I didn't worry about it (although in retrospect I should've mentioned it), but I see that you found a way to avoid the error altogether by changing the source directory name. That's probably better.

Reading this F2PY build guide and #22, I realize now that the install process might break again in a few months when python 3.12 comes out. In fact, it looks like it's already possible to test with python 3.12.0-rc.1 via github actions (supported python versions), so it might be worth doing that just to see what happens.

If it doesn't work, my impression was that the meson approach described in the above guide seemed best. Making this switch now would avoid the need to solve the setuptools-related editable install/source directory name problems. I'm gonna take a look now and see if I can get that working without too much trouble.

fujiisoup commented 1 year ago

Thank you for the info. I think we can merge this PR for now and start working #22 based on this PR.

I will work on a ci that can run python 3.12, which may be useful for avoiding distutils.