nipy / PySurfer

Cortical neuroimaging visualization in Python
https://pysurfer.github.io/
BSD 3-Clause "New" or "Revised" License
240 stars 97 forks source link

FIX: Fix setup.py #213

Closed larsoner closed 6 years ago

larsoner commented 6 years ago

When trying to set up an Anaconda environment using pip to simultaneously install mayavi and PySurfer, I get these errors:

Installing collected packages: mne, nibabel, imageio, pysurfer, nitime, nilearn, quantities, neo, termcolor, pytest-sugar, pytest-faulthandler, pydocstyle, sphinx-bootstrap-theme, traits, pyface, traitsui, configobj, apptools, mayavi, sphinx-gallery
  Running setup.py install for pysurfer ... error
    Complete output from command /home/larsoner/anaconda2/envs/mne/bin/python -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-45nhe837/pysurfer/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record /tmp/pip-3smloy5u-record/install-record.txt --single-version-externally-managed --compile:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-45nhe837/pysurfer/setup.py", line 64, in <module>
        check_dependencies()
      File "/tmp/pip-build-45nhe837/pysurfer/setup.py", line 51, in check_dependencies
        raise ImportError("Missing dependencies: %s" % missing)
    ImportError: Missing dependencies: mayavi

This is because we don't tell pip about the mayavi dependency, so it tries to install PySurfer first. We should either remove our custom dependency check, or add mayavi to the deps list -- this PR does the latter.

mwaskom commented 6 years ago

Well, originally this was somewhat on purpose because we assumed most people probably didn't want pip trying to install mayavi. It used to be a operation with a low probability of success. Maybe with more modern python packaging infrastructure it is likely to work better?

mwaskom commented 6 years ago

Oh I see, the issue is that you are trying to pip install mayavi and pysurfer at the same time.

The middle ground is to limit the setup modes in which the dependency check is run: https://github.com/statsmodels/statsmodels/commit/f184b8b80ac9953487fd90e907ca165330373784

larsoner commented 6 years ago

Maybe with more modern python packaging infrastructure it is likely to work better?

It seems to work decently with Anaconda at least. I definitely don't want to add numpy to the list of dependencies, since that's still asking for trouble.

I think if we eliminate doing dependency checks for install as the mode (which is what I assume pip uses? or something similar that ends up being functionally equivalent?) there isn't much point in having a dependency check in there -- I'm not sure what modes we'd end up wanting to leave it on for.

Either solution is fine by me, though.

mwaskom commented 6 years ago

I think if we eliminate doing dependency checks for install as the mode (which is what I assume pip uses? or something similar that ends up being functionally equivalent?) there isn't much point in having a dependency check in there -- I'm not sure what modes we'd end up wanting to leave it on for.

The way that statsmodels code works is that if you don't have mayavi installed then pip install pysurfer will fail but pip install pysurfer mayavi will work.

larsoner commented 6 years ago

It looks like we already do this check:

https://github.com/nipy/PySurfer/blob/master/setup.py#L59

Does this mean it's not working? Or is the important change elsewhere?

mwaskom commented 6 years ago

It's certainly possible that my memory is failing on aspects of this issue. There was a lot of back and forth about it in seaborn. But honestly, pip is pretty good these days ... I just made a clean conda environment with nothing but python and pip and pip installed numpy/scipy/matplotlib/pandas/seaborn with no issues whatsoever. The default pip install -U behavior remains irritating, but at least they added --no-deps which I tend to do by default. All of this is to argue in favor just just declaring all dependencies in install_requires and washing our hands of the more complicated and less flexible homebrew dependency checking.

mwaskom commented 6 years ago

pip installed numpy/scipy/matplotlib/pandas/seaborn with no issues whatsoever

Forgot to say: on OSX!

larsoner commented 6 years ago

Wow that's pretty good. I was wondering when they'd add something like --no-deps, glad it's in!

So adding mayavi in the list is okay with you? I'm still too afraid to add numpy and scipy because their compiling configurations are harder.

mwaskom commented 6 years ago

With an up-to-date pip I don't think you should need to compile numpy or scipy, pip is pull down binary wheels for me. I'm not sure what the situation is on windows though.

larsoner commented 6 years ago

Okay with you just to add mayavi for now (current PR)?

mwaskom commented 6 years ago

I'm mildly unsatisfied with the piecemeal approach to dependency specification but I see the argument that it assumes the existence of a full Anaconda distribution.

This means mayavi should also come out of the needed_deps list in check_dependencies() right?

larsoner commented 6 years ago

This means mayavi should also come out of the needed_deps list in check_dependencies() right?

Good call, removed (and comment added that it and nibabel are in install_requires).