nomad-coe / nomad

NOMAD lets you manage and share your materials science data in a way that makes it truly useful to you, your group, and the community.
https://nomad-lab.eu
Apache License 2.0
64 stars 14 forks source link

Running `./scripts/setup_dev_env.sh` fails at MDAnalysis build #81

Open berquist opened 10 months ago

berquist commented 10 months ago

This is part of the JOSS review.

Following https://nomad-lab.eu/prod/v1/docs/develop/setup.html#install-nomad, running the script after performing

pyenv install 3.9.17
pyenv local 3.9.17
python -m venv ./venv
source ./venv/bin/activate

gives the following pip traceback, failing to compile MDAnalysis 2.1.0:

Collecting mdanalysis==2.1.0 (from -r requirements-dev.txt (line 144))
  Downloading MDAnalysis-2.1.0.tar.gz (3.5 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.5/3.5 MB 33.2 MB/s eta 0:00:00
  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
  ╰─> [58 lines of output]

      Error compiling Cython file:
      ------------------------------------------------------------
      ...
          array_wrapper = ArrayWrapper()
          array_wrapper.set_data(<void*> data_ptr, <int*> &dim[0], dim.size, data_type)

          cdef np.ndarray ndarray = np.array(array_wrapper, copy=False)
          # Assign our object to the 'base' of the ndarray object
          ndarray.base = <PyObject*> array_wrapper
                 ^
      ------------------------------------------------------------

      MDAnalysis/lib/formats/cython_util.pyx:115:11: Assignment to a read-only property
      Attempting to autodetect OpenMP support... Compiler supports OpenMP
      Will attempt to use Cython.
      Compiling MDAnalysis/lib/formats/libdcd.pyx because it changed.
      Compiling MDAnalysis/lib/c_distances.pyx because it changed.
      Compiling MDAnalysis/lib/c_distances_openmp.pyx because it changed.
      Compiling MDAnalysis/lib/qcprot.pyx because it changed.
      Compiling MDAnalysis/lib/formats/libmdaxdr.pyx because it changed.
      Compiling MDAnalysis/lib/formats/cython_util.pyx because it changed.
      Compiling MDAnalysis/analysis/encore/cutils.pyx because it changed.
      Compiling MDAnalysis/analysis/encore/clustering/affinityprop.pyx because it changed.
      Compiling MDAnalysis/analysis/encore/dimensionality_reduction/stochasticproxembed.pyx because it changed.
      Compiling MDAnalysis/lib/_cutil.pyx because it changed.
      Compiling MDAnalysis/lib/_augment.pyx because it changed.
      Compiling MDAnalysis/lib/nsgrid.pyx because it changed.
      [ 1/12] Cythonizing MDAnalysis/analysis/encore/clustering/affinityprop.pyx
      [ 2/12] Cythonizing MDAnalysis/analysis/encore/cutils.pyx
      [ 3/12] Cythonizing MDAnalysis/analysis/encore/dimensionality_reduction/stochasticproxembed.pyx
      [ 4/12] Cythonizing MDAnalysis/lib/_augment.pyx
      [ 5/12] Cythonizing MDAnalysis/lib/_cutil.pyx
      [ 6/12] Cythonizing MDAnalysis/lib/c_distances.pyx
      [ 7/12] Cythonizing MDAnalysis/lib/c_distances_openmp.pyx
      [ 8/12] Cythonizing MDAnalysis/lib/formats/cython_util.pyx
      Traceback (most recent call last):
        File "/home/eric/repositories/nomad-coe/nomad/venv/lib/python3.9/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/home/eric/repositories/nomad-coe/nomad/venv/lib/python3.9/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/eric/repositories/nomad-coe/nomad/venv/lib/python3.9/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-f6kxxutk/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 341, in get_requires_for_build_wheel
          return self._get_build_requires(config_settings, requirements=['wheel'])
        File "/tmp/pip-build-env-f6kxxutk/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 323, in _get_build_requires
          self.run_setup()
        File "/tmp/pip-build-env-f6kxxutk/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 487, in run_setup
          super(_BuildMetaLegacyBackend,
        File "/tmp/pip-build-env-f6kxxutk/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 338, in run_setup
          exec(code, locals())
        File "<string>", line 590, in <module>
        File "<string>", line 449, in extensions
        File "/tmp/pip-build-env-f6kxxutk/overlay/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1134, in cythonize
          cythonize_one(*args)
        File "/tmp/pip-build-env-f6kxxutk/overlay/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1301, in cythonize_one
          raise CompileError(None, pyx_file)
      Cython.Compiler.Errors.CompileError: MDAnalysis/lib/formats/cython_util.pyx
      [end of output]

I haven't confirmed, but I wouldn't be surprised if it's one of the many issues related to the new Cython 3.0.0 release, since it's probably picking up my system install, and https://github.com/MDAnalysis/mdanalysis/blob/2bf55dd714292bbf7c12023a28153a6443a52ec6/package/CHANGELOG#L86 shows this may not have been fixed until MDAnalysis 2.5.0.

Two questions:

berquist commented 10 months ago

Installing Cython 0.29.36 into a fresh conda-based env before running the script again produced a different error, so if downgrading the version is a possible workaround, that version isn't old enough:

      [ 8/12] Cythonizing MDAnalysis/lib/formats/cython_util.pyx
      Traceback (most recent call last):
        File "/home/eric/.pyenv/versions/miniforge3-4.10.3-10/envs/nomad_env/lib/python3.9/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/home/eric/.pyenv/versions/miniforge3-4.10.3-10/envs/nomad_env/lib/python3.9/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/eric/.pyenv/versions/miniforge3-4.10.3-10/envs/nomad_env/lib/python3.9/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-pabb7lu_/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 341, in get_requires_for_build_wheel
          return self._get_build_requires(config_settings, requirements=['wheel'])
        File "/tmp/pip-build-env-pabb7lu_/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 323, in _get_build_requires
          self.run_setup()
        File "/tmp/pip-build-env-pabb7lu_/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 487, in run_setup
          super(_BuildMetaLegacyBackend,
        File "/tmp/pip-build-env-pabb7lu_/overlay/lib/python3.9/site-packages/setuptools/build_meta.py", line 338, in run_setup
          exec(code, locals())
        File "<string>", line 590, in <module>
        File "<string>", line 449, in extensions
        File "/tmp/pip-build-env-pabb7lu_/overlay/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1134, in cythonize
          cythonize_one(*args)
        File "/tmp/pip-build-env-pabb7lu_/overlay/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1301, in cythonize_one
          raise CompileError(None, pyx_file)
      Cython.Compiler.Errors.CompileError: MDAnalysis/lib/formats/cython_util.pyx
berquist commented 10 months ago

Regarding the pin, I've read https://github.com/nomad-coe/nomad/issues/75#issuecomment-1633223069 and do agree, but you should also know that since many projects bring in Cython as a transitive dependency those projects don't necessarily pin Cython versions, their upgrade from from 0.29 to 3.0 broke a lot of code.

lauri-codes commented 10 months ago

Thanks @berquist for this report and all of the details.

@JFRudzinski: I remember that you were trying out updating the MDAnalysis version: Did you update the mda version in the issue you are working on: #1643? In principle, I don't see any problems updating our pinned version to e.g. 2.5.0.

JFRudzinski commented 10 months ago

@lauri-codes Actually, I had updated to 2.5.0 some time ago (not sure which issue or branch). However, in my tests for #1643 I found that 2.5.0 has some issues reading certain versions of .trr and .xtc files.

So, I was planning to revert now back to 2.1.0. However, once I am done with my dev, I can do some more testing if it would help to see what the root of the issues are with 2.5.0 in my case?

lauri-codes commented 10 months ago

@JFRudzinski: Yes, it would be great if you could debug the issue in reading .trr and .xtc files. If the problem is with MDA itself, then you should search for any existing issues in their GitHub page, and possibly file a new one if nothing exists already.

It is a bit unfortunate that MDA has a cython build step: the official recommendation is to ship the resulting .c files in a release and use them primarily even if the user has cython installed. But looking at their setup.py, they seem to prefer the cython compilation if cython exists in the installation environment.

JFRudzinski commented 10 months ago

Ok, np, I will look into it and get back to you here

JFRudzinski commented 10 months ago

@lauri-codes I am fairly confident now that the only issue with MDA version 2.5.0 is something to do with a sort of advanced usage that I am performing when calculating the msds. Additionally, this is only happening for 1 trajectory file obtained from a user. (As I said before, I had been using 2.5.0 for a while without any issues on many other data).

I would say then that you can pretty safely go ahead and update the MDA version. I will address my own specific issue in the mean time, but I don't really think you need to wait for this, as it is so specific (and possibly unique to this single example).

lauri-codes commented 10 months ago

The MDA version has now been updated to v2.5.0.