getkeops / keops

KErnel OPerationS, on CPUs and GPUs, with autodiff and without memory overflows
https://www.kernel-operations.io
MIT License
1.03k stars 65 forks source link

pykeops dependency on keopscore when installing from git repository #226

Closed gdurif closed 2 years ago

gdurif commented 2 years ago

A little brain-teaser

In pykeops setup.py file, keopscore requirement is hard-coded on the current version number in the project with "keopscore==" + current_version.

Problem: the master branch is moving faster than PyPI release, and the current version of the repository on master may not be compatible with the latest release of keopscore on PyPI (despite wearing the same version number, which is 2.0b1 at the moment).

For instance, either of the following commands

python setup_pykeops.py install # from the project root directory on master branch
pip install git+https://github.com/getkeops/keops.git@master
pip install ./pykeops # from the project root directory on python_package_subdir branch
pip install git+https://github.com/getkeops/keops.git@python_package_subdir#subdirectory=pykeops

will provide a buggy install of pykeops, because pip retrieve keopscore==2.0b1 from PyPI which is not compatible with the current version of pykeops on master branch (or on python_package_subdir branch, which is the same), c.f. https://github.com/getkeops/keops/issues/225

To (partially^[1]) avoid this, we should keep the master always corresponding to the latest release on PyPI. Hence, when merging a pull request, or pushing to master branch, we should always make a release on PyPI.

[1] this would ensure that at least pip install git+https://github.com/getkeops/keops.git@master install a working version of pykeops and keopscore.

To ensure consistency while installing from development branch, I don't know how we could do that.

joanglaunes commented 2 years ago

Maybe another possibility : have an "option" in the setup.py from folder pykeops to install without dependency, then put a specific setup.py in the root folder that "calls" the setup.py from folder keopscore and then setup.py from pykeops one without dependency option. would it be possible to do that ?

gdurif commented 2 years ago

For the moment (after talking with @bcharlier), I removed the dependency "keopscore==" + current_version in the pykeops setup.py file (c.f. #222 ) and the dependency will be hard-coded at the deploy stage (I will work on that next).

I keep this open until I create the corresponding PR.

jeanfeydy commented 2 years ago

Hi all,

To be clear, when installing KeOps from git, users should now update their PYTHONPATH to include /path/to/keops/pykeops and /path/to/keops/keopscore, right? If this is so, I can update the Python doc.

gdurif commented 2 years ago

Hi,

The dependency in pykeops on keopscore version is not hard-coded anymore (it will only be hard-coded at the moment of the release, I am working on that).

So I edited the doc here so that it advices to both install keopscore and pykeops with

pip install git+https://github.com/getkeops/keops.git@main#subdirectory=keopscore
pip install git+https://github.com/getkeops/keops.git@main#subdirectory=pykeops

Note: the doc on the website is not up-to-date (c.f. here), we should update it.

Regarding the PYTHONPATH setup, in the second point of the corresponding paragraph, I would change if and recommend to install locally^[1] with

# from project root directory
pip install -e ./keopscore
pip install -e ./pykeops

which is (for me) more simple that setting up the PYTHONPATH.

[1] with the standard explanation about using pip install --user or even better about using a Python environment (or a conda env).

Would that be ok with you?

Edit: I fixed the local pip install, my bad

joanglaunes commented 2 years ago

Hello @gdurif , It looks ok for me, except that when doing pip install -e, I guess it installs the files to the package repository, so it is not good for developers who need to update the code in the git repo. Is this correct ?

gdurif commented 2 years ago

@joanglaunes I fixed the local pip install command:

pip install [-e] ./keopscore
pip install [-e] ./pykeops

the ./ is mandatory to tell pip to use the local directory and not the package of the same name from PyPI, or it is possible to do:

cd keopscore
pip install [-e] .
cd ../pykeops
pip install [-e] .

Note: the -e argument is not mandatory as pip install man page says:

-e, --editable <path/url>   Install a project in editable mode (i.e. setuptools "develop mode") from a local project path or a VCS url.
gdurif commented 2 years ago

Hello @joanglaunes @bcharlier I found a solution. So keopscore is listed without any version requirements in keopscore/setup.py and the trick is to generate a setup.cfg file only for the deployment phase containing:

[options]
install_requires =
  numpy
  pybind11
  keopscore == "2.0b1"

(with the current version number) and this will hard-code the version requirements. I am implementing this at the moment and will create a pull request ASAP.

gdurif commented 2 years ago

Should be fixed by #229