[FEATURE] On pymanopt

[daviddavo]

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

• Yes, I can contribute for this issue independently.
• Yes, I can contribute for this issue with guidance from Recommenders community.
• No, I cannot contribute at this time.

Possible solutions

• https://discuss.python.org/t/packages-installed-from-pypi-cannot-depend-on-packages-which-are-not-also-hosted-on-pypi/3736/4

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]

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

[miguelgfierro]

Pymanopt is right now in the experimental

recommenders/setup.py

Line 75 in 4f86e47

extras_require["experimental"] = [

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

[daviddavo]

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

recommenders/setup.py

Lines 86 to 88 in 4f86e47

	# The following dependency can be installed as below, however PyPI does not allow direct URLs.
	# Temporary fix for pymanopt, only this commit works with TF2
	# "pymanopt@https://github.com/pymanopt/pymanopt/archive/fb36a272cdeecb21992cfd9271eb82baafeb316d.zip",

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.

recommenders/requirements-external.txt

Lines 1 to 5 in 8515dbf

	# 2024/02/29: pymanopt bumped to python 3.8
	pymanopt @git+https://github.com/pymanopt/pymanopt@e13cecaec3089c790cc93174840b2f557d179b3f ; python_version<'3.12'
	# Jun 2024: Fixes py312
	pymanopt @git+https://github.com/pymanopt/pymanopt@1de3b6f47258820fdc072fceaeaa763b9fd263b0 ; python_version>='3.12'

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.

recommenders/tests/ci/azureml_tests/aml_utils.py

Lines 90 to 96 in 4f86e47

	dependencies:
	- python={python_version}
	- {conda_pkg_jdk}
	- pip
	- pip:
	- pymanopt@https://github.com/pymanopt/pymanopt/archive/fb36a272cdeecb21992cfd9271eb82baafeb316d.zip
	- recommenders[dev{",gpu" if use_gpu else ""}{",spark" if use_spark else ""}]@git+https://github.com/recommenders-team/recommenders.git@{commit_sha}

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

[miguelgfierro]

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]

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]

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]

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]

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

[miguelgfierro]

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]

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".

• https://discuss.python.org/t/packages-installed-from-pypi-cannot-depend-on-packages-which-are-not-also-hosted-on-pypi/3736
• https://stackoverflow.com/a/54894359/4505998

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. lyst/lightfm#709

Therefore, LightFM should become one of these git dependecies.

[anargyri]

Btw see this issue #1597 as to why we removed pymanopt.