4dn-dcic / hic2cool

Lightweight converter between hic and cool contact matrices.
MIT License
63 stars 7 forks source link

Drop setup_requires #44

Open marcelm opened 4 years ago

marcelm commented 4 years ago

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.

paulmenzel commented 4 years ago

Sounds useful. Is anything blocking this from being accepted?

SooLee commented 4 years ago

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

Any suggestion?

marcelm commented 4 years ago

@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.

SooLee commented 4 years ago

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.

marcelm commented 4 years ago

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 commented 4 years ago

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

SooLee commented 4 years ago

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.

marcelm commented 4 years ago

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