Installation fails from PyPI

sugatoray commented 2 years ago

I tried installing from PyPI on colab. But got the ModuleNotFoundError.

Colab ScratchPad: https://colab.research.google.com/notebooks/empty.ipynb

I discovered this while trying to package tensorly-torch for conda-forge. PR: https://github.com/conda-forge/staged-recipes/pull/17313.

sugatoray commented 2 years ago

cc: @JeanKossaifi

Apart from the problem with installation, I have a few observations/concerns:

A. Missing dependency (comment)
B. Tests should not be included in the source-code (comment)
C. Suggestion to use absolute import (over relative import) (comment)

sugatoray commented 2 years ago

A. Missing dependency `torch` from `setup.py:install_requires` and `requirements.txt`

Your setup.py:install_requires only lists ['numpy', 'scipy', 'nose'] and DOES NOT include torch which is clearly a dependency.

https://github.com/tensorly/torch/blob/0a32b764542cca037cc9dc3bead4f8338fae24a7/setup.py#L59

torch is a dependency:

sugatoray commented 2 years ago

B. Tests are included in source code

Please remove tests folders from within your source code. It unnecessarily bloats the source code that a user will install and use. Tests are only meant for testing if the source-code functions as designed and must reside in the repository alone.

tltorch
|
|--- tensor_hooks/
tests
|
|--- tensor_hooks/
       __init__.py
       various_tests.py

The typical tests folder should follow this structure.

sugatoray commented 2 years ago

C. Please consider adopting absolute import instead of relative import

Choosing absolute import over relative import has some advantages in terms of decoupling tests from the source code.

- from ..factorized_tensors import TuckerTensor, TTTensor, CPTensor
+ from tltorch.factorized_tensors import TuckerTensor, TTTensor, CPTensor
- from ..utils import ParameterList
+ from tltorch.utils import ParameterList

https://github.com/tensorly/torch/blob/0a32b764542cca037cc9dc3bead4f8338fae24a7/tltorch/tensor_hooks/_tensor_lasso.py#L9-L10

sugatoray commented 2 years ago

💡 I was able to figure it out. The import error happened because of missing dependency specifications. You need to add torch and tensorly to your setup.py:install_requires and to requirements.txt.

JeanKossaifi commented 2 years ago

Hi @sugatoray - thanks for opening the issue and creating the condo-forge recipe! I actually need to automate the creation of the conda package for TensorLy in the tensorly channel! Help is welcome if you already have experience there!

Regarding the dependencies: I didn't include torch on purpose as I don't want the installer to default on pypi - PyTorch installation can vary significantly depending on the user/platform.

For local imports, it's useful if i) you're working on a different source code than the one installed and ii) if you're working in an uninstalled repo. Also it tends to be tidier but that's anecdotal.

Finally, for tests (and more generally), I try to follow the standards of Scikit-Learn. I find it much tidier to have unit-tests for a function in the same folder, rather than have to browse to separate but identical hierarchies of folders.

JeanKossaifi commented 2 years ago

Hi @sugatoray - will close this issue. Would be great if you wanna make a small PR to add automatic conda build! :)

sugatoray commented 2 years ago

Hi @JeanKossaifi, I have not myself created any automated conda build yet. But you might find something useful in the repositories for deepchecks and torchcam. I hope that helps.

However, my personal note is that it is better to create conda package on conda-forge than your own channel because this allows others to use your package in downstream applications and make them available on conda-forge as well.

The conda-forge recipe creates the package on conda-forge and so it is now available on conda.

tensorly / torch