jeanfeydy / geomloss

Geometric loss functions between point clouds, images and volumes
MIT License
599 stars 60 forks source link

Refactor setup #59

Closed josephenguehard closed 1 year ago

josephenguehard commented 2 years ago

Add Pytorch requirement and remove import geomloss from setup.py

bcebere commented 2 years ago

@jeanfeydy Can this PR be merged and released, please? Without ' torch ' installed, there are problems with usinggeomloss in new environments. This is because of the import geomloss line in the setup.py file cannot find torch.

Thanks!

jeanfeydy commented 2 years ago

Hi @josephenguehard, @bcebere,

Thanks for your PR and comment! I am working for a new GeomLoss release in ~2 weeks (I've been getting up to speed with reproducible containers this week), and will definitely merge this.

Best regards, Jean

aldopareja commented 2 years ago

The package can't be used on travis because the setup fails. Merging this would make the package easier to depend on.

Snagnar commented 2 years ago

I'm currently trying to get another package installed that depends on geomloss. But this is prohibited by the error fixed in this PR. So, if this could be merged as soon as possible, that would be awesome :sweat_smile:

matthewfeickert commented 1 year ago

I am working for a new GeomLoss release in ~2 weeks (I've been getting up to speed with reproducible containers this week), and will definitely merge this.

:wave: Hi @jeanfeydy. Please consider making a patch release v0.2.6 which is just v0.2.5 but with this PR added in it. The last release (v0.2.5) is coming up on 1 year old, and while I'm sure that you want to have the next release be the interesting one that you mentioned this package is currently in an uninstallable state for many systems. A patch release would make a huge difference for many people!

matthewfeickert commented 1 year ago

@josephenguehard I don't think @jeanfeydy is coming back. Given the lack of response after describing a release they were excited about and that the last commit they made to a public repository was 2022-08-15 I am assuming they now have a job that legally doesn't allow them to contribute to open source anymore (these things happen, and I can't blame people in these situations).

You could try emailing them directly and seeing if you can get a response, but I think that if you want to keep using this project moving forward you might want to consider forking the development (you already have it forked, so just keep going).

jeanfeydy commented 1 year ago

Hi all,

To be clear, I am still 100% invested in open source, and my job involves a significant amount of library development. However, I also have new duties (teaching in the Fall, administrative stuff to create a research team, interviews for new PhD students and engineers, grants at the French and European level...) which mean that I now have a limited bandwidth.

GeomLoss is very important to me, but it is just one block in a stack that goes from KeOps (low level) to scikit-like libraries for survival analysis and shape analysis that we are going to release in 2023. Since KeOps is already released as a stable library, I commit to frequent interactions on GitHub - but for GeomLoss, which is still in beta (I.e. prior to the v1.0), I can only take your (great) feedback into account when making a new release.

Now, at the moment and to be transparent with you: I am waiting for the release of PyTorch 2.0 (which is planned for early March, I.e. any day now) to update the pip packages and Docker images for both KeOps and GeomLoss. I expect some breaking changes from the new PyTorch release (hopefully I'm wrong!) and since making new releases of the full "KeOps suite" usually takes me 1-2 days of work (with all the doc rendering, testing, etc.), I want to make sure that this is done with the "new" PyTorch.

In any case, apologies for the lack of responsivity. I have had to prioritize investments on new lectures, projects and students in 2022, and could only stay "afloat" for the core KeOps library. I hope that this didn't impact you guys too much.

Best, Jean

jeanfeydy commented 1 year ago

P.S.: I'm always happy to learn about new applications of OT/GeomLoss (beyond my core motivations in healthcare), and will actually attend a workshop on OT in physics next week 🤓 I will certainly check your work too!

With respect to the lack of visible contributions on GitHub - this is just an artifact of the fact that my two main new projects, "survivalGPU" and "scikit-shapes" (which depends on GeomLoss) are still in private repositories.

DrShushen commented 1 year ago

@jeanfeydy - I would also hugely appreciate a patch release with this PR added as suggested by @matthewfeickert, as I manage several open source projects with geomloss dependency that have to rely on a workaround (which isn't necessarily obvious to the end users) to avoid installation failure:

jeanfeydy commented 1 year ago

Hi @DrShushen , @matthewfeickert : I have just uploaded the v0.2.6 on PyPi, please let me know if there is any issue. Apologies for the delay between the PR merge and the pull request - I hope that everything is well now.

DrShushen commented 1 year ago

Fantastic, I will check that this resolves the issues on my end and will drop a note here if it's all good.

matthewfeickert commented 1 year ago

LGTM :+1:

$ docker run --rm -ti python:3.10 /bin/bash
root@ba229530db4c:/# python -m venv venv && . venv/bin/activate
(venv) root@ba229530db4c:/# python -m pip --quiet install --upgrade pip setuptools wheel
(venv) root@ba229530db4c:/# python -m pip --quiet install --upgrade geomloss
(venv) root@ba229530db4c:/# python -m pip show geomloss
Name: geomloss
Version: 0.2.6
Summary: Geometric loss functions between point clouds, images and volumes.
Home-page: 
Author: Jean Feydy
Author-email: jean.feydy@inria.fr
License: LICENSE.txt
Location: /venv/lib/python3.10/site-packages
Requires: numpy, torch
Required-by: 
(venv) root@ba229530db4c:/# python -c 'import geomloss; print(geomloss)'
<module 'geomloss' from '/venv/lib/python3.10/site-packages/geomloss/__init__.py'>
(venv) root@ba229530db4c:/# 

Thanks @jeanfeydy

DrShushen commented 1 year ago

Can confirm, resolved issues on my end too.