networkx / networkx-metis

NetworkX Addon to allow graph partitioning with METIS
Other
79 stars 21 forks source link

Add requirements.txt #35

Closed OrkoHunter closed 9 years ago

OrkoHunter commented 9 years ago

Fixes #34

networkx needs to be networkx>1.9.1. But as we don't have a final decision on whether we are including it in 1.10 or not, I'm keeping it without the version number.

chebee7i commented 9 years ago

Yeah, we can always modify this later.

SanketDG commented 9 years ago

Could someone fill me on this? Why are the rest of the packages here? This is duplication of information. Only Cython and the test packages should be here, in case that is not handled by setup.py.

Also,can't Cython be handled with setup.py? Atleast an import check to choke an exception?

Edit: Relevant read: https://caremad.io/2013/07/setup-vs-requirement/

chebee7i commented 9 years ago

We can certainly make setup.py pull this information from requirements.txt, but I think having a requirements.txt with Cython included is a good thing.

Cython cannot be handled with setup.py, since it must exist in order to complete the setup. At best, we can raise an ImportError, but what does that solve?

SanketDG commented 9 years ago

We can certainly make setup.py pull this information from requirements.txt, but I think having a requirements.txt with Cython included is a good thing.

Yes that could be done, but just having Cython is more explicit.

Cython cannot be handled with setup.py. At best, we can raise an ImportError, but what does that solve?

Atleast it gives the user a heads up, instructions to install Cython is only written in the Readme. For a reference, we could just easily write that Python 2.7 isn't supported in the readme instead of checking for it in setup.py. Doing it in setup.py is way more intuitive instead of the Readme.

Pandas does it: https://github.com/pydata/pandas/blob/master/setup.py#L80

OrkoHunter commented 9 years ago

I must say that the given example of raising check on installation of Cython does not appropriately fit here. NetworkX-Metis is the repository where a Python and a C library is being interfaced. Raising a check saying "Cython not installed" sounds trivial to me. However, I agree that there is duplication of install_requires in setup.py and requirements.txt. But can't we consider both? After all, it's not much in the sense of code duplication.

SanketDG commented 9 years ago

This is what happens when I dont have Cython:

  File "setup.py", line 10, in <module>
    from Cython.Build import cythonize
ImportError: No module named 'Cython'

If this is helpful enough, then that's okay.

However, I agree that there is duplication of install_requires in setup.py and requirements.txt. But can't we consider both? After all, it's not much in the sense of code duplication.

As you said, the difference would be trivial.

I must say that the given example of raising check on installation of Cython does not appropriately fit here. NetworkX-Metis is the repository where a Python and a C library is being interfaced

Yes, everybody would know that this is a package which helps in using python as a means to talk to a very popular graph partitioning library,in simple terms. But I don't think they would know about Cython. Again I think this depends on the traceback above and the competency of the end user.

chebee7i commented 9 years ago

We can certainly make setup.py pull this information from requirements.txt, but I think having a requirements.txt with Cython included is a good thing.

Yes that could be done, but just having Cython is more explicit.

Isn't that somewhat counter to the point of a requirements.txt, which is to list all dependencies of the package?

All we have to do is something like:

with open('requirements.txt') as fobj:
    install_requires = fobj.readlines()
SanketDG commented 9 years ago

Isn't that somewhat counter to the point of a requirements.txt, which is to list all dependencies of the package?

For a application, yes. For a reusable library, its recommended to just use setup.py. We are using setuptools to distribute, so I cant reason on why we might need a requirements.txt. http://blog.miguelgrinberg.com/post/the-package-dependency-blues

I think in this case, it can be perfectly used for listing dependencies that are otherwise not handled by setuptools i.e Cython and optionally, the test suite.

I now see too very clear options:

I would go with the second one.

SanketDG commented 9 years ago

I found another pretty way of handling it, they just include the compiled .c file. https://github.com/LukeCarrier/py3k-oursql/blob/master/setup.py#L49-#L55 So basically, cython wont even be a dependency.

Sorry if I'm teribbly wrong in these comments. Just trying to provide a better solution.

chebee7i commented 9 years ago

Aha yes, that is also a pretty good option. I think NumPy does (or did) that too. So we could remove the Cython dependency altogether. Then the requirements.txt is mostly a convenience for developers and would only include what install_requiresdoes not.