search

recommenders-team / recommenders

Best Practices on Recommendation Systems
https://recommenders-team.github.io/recommenders/intro.html
MIT License
18.81k stars 3.07k forks source link

[FEATURE] On pymanopt #2138

Open daviddavo opened 1 month ago

daviddavo commented 1 month ago

Description

The pymanopt dependency must currently be installed with an external command, making the setup more difficult. Setup should be transparent to the user and just pip install recommenders.

Although the dependency can be included in install_requires, it won't let you upload the recommenders package to pypi.

Nevertheless, pymanopt team doesn't update the pypi package anymore.

Expected behavior with the suggested feature

pip install recommenders just works

Willingness to contribute

Possible solutions

tldr:

Forking

Making a fork or mirror in recommenders-team and then re-releasing it to pypi under a different name like recommenders-pymanopt

Vendoring

Including the code or the project as a git submodule, and then installing "from file".

Creating a requirements.txt-like file

Creating an external-dependencies.txt file, and then instructing people (and the CI flow) to use pip install -r external-dependencies.txt

daviddavo commented 1 month ago

I needed two different versions to work with python<=3.8 and python>=3.12, so I went with the requierements.txt approach.

miguelgfierro commented 1 month ago

Pymanopt is right now in the experimental https://github.com/recommenders-team/recommenders/blob/4f86e4785337d455aa3cb7e8920c3fab9a2a0140/setup.py#L75

Can you share the error? Maybe the problem is that the tests are executed but it should be commented?

daviddavo commented 1 month ago

I know, but the version in extras_require did not work. That's why a certain commit needs to be installed.

https://github.com/recommenders-team/recommenders/blob/4f86e4785337d455aa3cb7e8920c3fab9a2a0140/setup.py#L86-L88

It also says so on the README too. On python312 I created the requirements-external.txt file to simplify instructions. We could even add my lightfm fork to this file too.

https://github.com/recommenders-team/recommenders/blob/8515dbf17df5dff51c54deb0a1c2d7d6a7baaa41/requirements-external.txt#L1-L5

The tests always install pymanopt, even if its not needed. If they try to install a version that is not compatible with other deps, it will fail before even starting.

https://github.com/recommenders-team/recommenders/blob/4f86e4785337d455aa3cb7e8920c3fab9a2a0140/tests/ci/azureml_tests/aml_utils.py#L90-L96

Note: In the python3.12 branch, the tests use requirements-external.txt instead.

miguelgfierro commented 1 month ago

The tests always install pymanopt, even if its not needed.

This shouldn't happen. That line should be removed.

@daviddavo I think the cleanest and easiest way to proceed here is to remove any dependency of pymanopt in the installation process, move the deps to experimental, and then add in the notebook a note explaining how to install the library and in which python versions the notebook works. This is the process that we have followed in other similar cases.

This is yet another reminder that adding dependencies can cause us a lot of headaches in the future. I think I'm going to self proclaim as the Chief Dependency Remover lol

daviddavo commented 1 month ago

Then, should the tests that use pymanopt be moved to the experimental test groups and that line removed? Should I do it in another branch? Or just on the python312 branch?

I would keep the requirements-experimental.txt file or something similar (perhaps in another folder or along the notebook), and add lightfm there too. It "centralizes" the version requirements and makes them easier to install, instead of saying "if you have this version use this command, or if you have this version use this other command, if you have...", which could cause errors with inexperienced users.

miguelgfierro commented 1 month ago

Then, should the tests that use pymanopt be moved to the experimental test groups and that line removed? Should I do it in another branch? Or just on the python312 branch?

Yes, the tests should be moved. I think it would be better to do this on a different branch. Once it's reviewed, it can be merged to the python312.

About the requirements file, what I would suggest is to either have the deps in the setup file together with the experimental deps, or to have two different files, one for the standard requirements and another for the experimental. Having only one can cause confusion to users because they may think the experimental file is the standard requirements file.

I think the reason we haven't used the requirements.txt is because it was easier to operate with the azureml tests

daviddavo commented 1 month ago

About the requirements file, what I would suggest is to either have the deps in the setup file together with the experimental deps, or to have two different files, one for the standard requirements and another for the experimental. Having only one can cause confusion to users because they may think the experimental file is the standard requirements file.

The problem is that you can't use packages from outside pypi in the setup.py. It will work on local but it doesn't allow you to upload it.

There's no reason to name it just requirements.txt, in fact my syntax highlighter recognises it as long as it is *-requirements.txt. Python doesn't care about the file name, is just plain text, and the fact that is called requirements.txt is just a convention.

I would use the setup.py for everything except experimental, and a file called experimental-requirements.txt file. Also removing the [experimental] extra from setup.py to avoid confusion.

daviddavo commented 1 month ago

With #2139 merged, what about the experimental-requirements.txt file? I can also change the docs to reflect the change.

miguelgfierro commented 1 month ago

It would be good to have a point of view not only on this scenario, but in all other scenarios. Let me put the options on the table to discuss (others feel free to add more).

I think the options would be:

  1. Add a file like experimental-requirements.txt and explain in the notebook that one has to install the deps using pip install -r experimental-requirements.txt after installing recommenders. Here it would be quick, but we would add a new file that can cause confusion to users.
  2. Add installation details in the markdown of each notebook. In this case, we won't need to install dependencies that we are not going to use, but the process might not be as direct as just using one install line. I think in the past @gramhagen mentioned something similar.
  3. All the problem comes from pymanopt@https://github.com/pymanopt/pymanopt/archive/fb36a272cdeecb21992cfd9271eb82baafeb316d.zip, maybe there is a version of pymanopt that works without the use of the zip, we could explore as well.
  4. ChatGPT told me that there is a way to add the zip into the setup.py (we need to review):
setup(
    name='your-package-name',
    version='0.1',
    packages=['your_package'],
    install_requires=[
        'pymanopt @ https://github.com/pymanopt/pymanopt/archive/fb36a272cdeecb21992cfd9271eb82baafeb316d.zip',
    ],
)

@daviddavo @SimonYansenZhao @anargyri if we wanted to make a decision on what option to follow, not only on this specific usecase, but also in any generic case. What would be your point of view?

daviddavo commented 1 month ago

About modifying setup.py, it works if someone where to install via pip install git+github.com/recommenders-team/recommenders or git clone ... && cd recommenders && pip install ., but pypi does not allow you to upload a package that requires "direct dependencies".

About pymanopt, there are newer versions that work, but they have not been uploaded to pypi. Latest version is from 10 months ago, and does not include Python 3.12. If they were to release the current version, it would not work with 3.8.

Furthermore, we also have to take into account lightfm, which has not been updated to support Cython 3, but could be installed from a forked version. https://github.com/lyst/lightfm/issues/709

Therefore, LightFM should become one of these git dependecies.