[JOSS Review] psycopg2 installation

[nmstreethran]

Hi, I'm reviewing your JOSS submission at https://github.com/openjournals/joss-reviews/issues/6734.

Your recommended installation method is using the requirements file. This currently has both psycopg2 and psycopg2-binary, which is redundant. As stated in the psycopg documentation:

The psycopg2-binary package is meant for beginners to start playing with Python and PostgreSQL without the need to meet the build requirements.

If you are the maintainer of a published package depending on psycopg2 you shouldn’t use psycopg2-binary as a module dependency. For production use you are advised to use the source distribution.

I suggest removing the binary package from your requirements.

I noticed that in your setup.py, you use the binary package if the source installation fails (also it seems like this install_dependencies function isn't being used anywhere, so the pip installation doesn't install psycopg2):

def install_dependencies():
    try:
        subprocess.run([sys.executable, '-m', 'pip', 'install', 'psycopg2'])
    except subprocess.CalledProcessError:
        subprocess.run([sys.executable, '-m', 'pip', 'install', 'psycopg2-binary'])

I think you should address the root cause of the psycopg2 installation error, instead of using the binary as a workaround. The issue is that some prerequisites are missing (i.e. the PostgreSQL library and Python development tools).

These are my steps to successfully install your package on my Ubuntu-based system:

sudo apt install python3-dev libpq-dev
python -m venv .venv
source .venv/bin/activate
python -m pip install -r requirements.txt

See https://stackoverflow.com/a/5450183.

[DorsanL]

Thank you for pointing this out and for your feedback for solving it.

psycopg2-binary has been removed from the requirements, as well as the install_dependencies() function. https://github.com/IPESE/REHO/commit/0893291bce6685a2088a3e9152bcc27430176c77

The documentation has been improved (cf Getting started > Installation > From source > Requirements), and a warning box added.

[nmstreethran]

Thanks for addressing this!

I have the following recommendations:

1. Since you suggest Windows users to install psycopg2-binary, I recommend modifying your requirements.txt with the following:

   psycopg2>=2.9.4,<3.0.0 ; platform_system != "Windows"
   psycopg2-binary>=2.9.9,<3.0.0 ; platform_system == "Windows"

   This ensures that the correct package is installed depending on the user's platform. You can then update the documentation to state that the prerequisites for psycopg2 are only required for Linux and Mac users.

2. Your setup.py is missing psycopg2. You can similarly modify install_requires to include the following:

   'psycopg2>=2.9.4,<3.0.0 ; platform_system != "Windows"',
   'psycopg2-binary>=2.9.9,<3.0.0 ; platform_system == "Windows"'

Feel free to close my issues once you address them.

[DorsanL]

I recommend modifying your requirements.txt with the following

Done, and documentation updated accordingly (https://reho.readthedocs.io/en/main/sections/5_Getting_started.html#requirements)

Your setup.py is missing psycopg2

Indeed, it used to be installed through the former install_dependencies() function. It's now part of install_requires() as suggested.

I can close the issue, please let me know if some issue persists :)

[nmstreethran]

Sorry to bring this up again! I seem to have missed this earlier, but there seems to be another issue with the requirements file. When I create a new venv and run python -m pip install -r requirements.txt, the ampl_module_base and ampl_module_highs packages fail to install:

Looking in indexes: https://pypi.org/simple, https://pypi.ampl.com, https://pypi.ampl.com
Ignoring psycopg2-binary: markers 'platform_system == "Windows"' don't match your environment
...
...
...
Installing collected packages: pytz, kaleido, urllib3, threadpoolctl, tenacity, six, python-dotenv, pyparsing, psycopg2, pillow, packaging, numpy, kiwisolver, joblib, idna, humanfriendly, greenlet, fonttools, et-xmlfile, dill, cycler, click, charset-normalizer, certifi, attrs, sqlalchemy, shapely, scipy, requests, python-dateutil, pyproj, plotly, openpyxl, multiprocess, h5py, contourpy, coloredlogs, cligj, click-plugins, scikit-learn, qmcpy, pandas, matplotlib, fiona, ampltools, scikit-learn-extra, SALib, pvlib, geopandas, amplpy
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
reho 1.1.0 requires ampl_module_base, which is not installed.
reho 1.1.0 requires ampl_module_highs, which is not installed.
Successfully installed SALib-1.4.8 amplpy-0.12.2 ampltools-0.7.5 attrs-23.2.0 certifi-2024.2.2 charset-normalizer-3.3.2 click-8.1.7 click-plugins-1.1.1 cligj-0.7.2 coloredlogs-15.0.1 contourpy-1.2.1 cycler-0.12.1 dill-0.3.8 et-xmlfile-1.1.0 fiona-1.9.6 fonttools-4.52.1 geopandas-0.14.4 greenlet-3.0.3 h5py-3.11.0 humanfriendly-10.0 idna-3.7 joblib-1.4.2 kaleido-0.2.1 kiwisolver-1.4.5 matplotlib-3.9.0 multiprocess-0.70.16 numpy-1.26.4 openpyxl-3.1.2 packaging-24.0 pandas-1.5.3 pillow-10.3.0 plotly-5.22.0 psycopg2-2.9.9 pvlib-0.10.5 pyparsing-3.1.2 pyproj-3.6.1 python-dateutil-2.9.0.post0 python-dotenv-1.0.1 pytz-2024.1 qmcpy-1.4.5 requests-2.32.2 scikit-learn-1.5.0 scikit-learn-extra-0.3.0 scipy-1.13.1 shapely-2.0.4 six-1.16.0 sqlalchemy-1.4.52 tenacity-8.3.0 threadpoolctl-3.5.0 urllib3-2.2.1

Can you reproduce this?

I managed to fix the problem by updating the requirements file as follows:

--extra-index-url https://pypi.ampl.com
amplpy>=0.12.0,<0.13.0
ampl_module_base
ampl_module_highs
pandas>=1.5.3,<2.0.0
...

[nmstreethran]

Re: my previous comment. Can you please check whether you are able to install ampl_module_base and ampl_module_highs using the requirements file?

[DorsanL]

I updated the requirements.txt according to your suggestion, and verified that the package and all examples are properly working in a fully new virtual environment. 👌 (https://github.com/IPESE/REHO/commit/70d7b11dabd67d50d87c9025ea896435e377f53c)

[nmstreethran]

Thanks! This is now resolved.