hic2cool icon indicating copy to clipboard operation
hic2cool copied to clipboard

Drop setup_requires

Open marcelm opened this issue 5 years ago • 8 comments

setup_requires should only contain those dependencies that are required by the setup.py script itself. Since the setup.py script doesn’t actually import any of the dependencies in the requires list, setup_requires can be removed entirely. This reduces installation time under some circumstances, and will also simplify the Bioconda package.

marcelm avatar Feb 21 '20 17:02 marcelm

Sounds useful. Is anything blocking this from being accepted?

paulmenzel avatar Mar 27 '20 16:03 paulmenzel

I didn't get it to work on my current system based on ubuntu 16.04 with the following setup:

  • python 3.6.10
  • pip 19.1.1
  • setuptools 42.0.0

Any suggestion?

SooLee avatar Mar 27 '20 17:03 SooLee

@SooLee In which way is it not working?

I cloned the repo, removed setup_requires, ran these commands, and it worked:

python3 -m venv venv
venv/bin/pip install cython
venv/bin/pip install .

The install cython part was necessary because I got an error from pysam, which wants me to install Cython. This is a bug in the pysam package, though.

marcelm avatar Mar 27 '20 19:03 marcelm

That didn't work for me, either. I get this numpy not found error.

ModuleNotFoundError: No module named 'numpy'

Ideally, it should work without doing an additional pip install.

Is this change necessary (e.g. it doesn't work without this change)? If not, I will leave it at the back burner for now and come back to it when I have more time.

SooLee avatar Mar 27 '20 20:03 SooLee

When running the program, run it with venv/bin/hic2cool. Alternatively, activate the virtual environment first with source venv/bin/activate, and then you can run it with hic2cool.

This PR is just about incorrect metadata. The installation is doing too much work. The program works whether you fix this not, it just makes life harder when packaging for Bioconda.

Ideally, it should work without doing an additional pip install.

Of course, but even without this PR, it is needed. This PR doesn’t change that. You probably don’t notice it in your day to day development work because you already have Cython installed. But anyone else will need it.

marcelm avatar Mar 27 '20 20:03 marcelm

I’m of course fine with this sitting here until you have more time.

marcelm avatar Mar 27 '20 20:03 marcelm

Are you trying to package hic2cool along with other packages? It looks like hic2cool bioconda package already exists.

conda config --append channels conda-forge  # for multiprocess
conda install -c bioconda hic2cool

This works for me.

By the way, I always work inside a Docker image to make sure it starts in a fresh environment. Maybe you can try it. Something may be missing from my environment.

docker run -it 4dndcic/ubuntu16.04-python36-pip19:v1
apt-get update -y
apt-get install -y python3.6-dev  # libpython3.8 required for `Python.h` in `pypairix`
git clone https://github.com/4dn-dcic/hic2cool
cd hic2cool
pip install setuptools==42  # or >42, solves `pandas` `0.24.2` not being able to install its dependency `numpy` as part of the setup process
python setup.py install

It doesn't work if I remove setup_requires even if I install cython.

SooLee avatar Mar 27 '20 21:03 SooLee

It looks like hic2cool bioconda package already exists.

Yes, the package exists. I was reviewing the PR bioconda/bioconda-recipes#20462 and noticed that the host requirements section was unnecessarily long (these are the requirements that need to be installed at build time. For Python, this is usually only python and pip). This is often a sign that someone did something wrong when creating the recipe, and I wanted to rectify it, but then I noticed that your package actually needs all those dependencies at install time due to the use of setup_requires.

I was able to reproduce your error message using the commands that you gave. I noticed that you use python setup.py install, which appears to be the problem here. I highly recommend not to use pip install . (or pip install -e .) as recommended in the packaging docs. If you do so, the installation will work even when setup_requires. python setup.py install also doesn’t use wheels, so the installation is very slow.

There are a couple of web pages describing why pip install should be preferred over python setup.py install, such as this Stackoverflow answer. Also see the two warnings at the end of this section in the pip documentation. setup_requires is meant for dependencies that setup.py itself needs, and even for that it does not work very well.

TL; DR Please switch to pip install . over python setup.py install

marcelm avatar Mar 27 '20 22:03 marcelm