ukhsa-collaboration / pygom

ODE modelling in Python
GNU General Public License v2.0
27 stars 20 forks source link

CI setup convoluted and not useful #40

Closed edwintye closed 1 month ago

edwintye commented 4 years ago

Problem

The current setup in travis is confusing due to the almost duplicated steps in line 31 and 33

https://github.com/PublicHealthEngland/pygom/blob/18bdd6ad8c3ecd66371a0ee8d72457d3d695ef0d/.travis.yml#L31-L33

because python setup.py install in fact checks the environment against the variable install_requires which has strict version requirements. This can be observed in the latest master build where the step python setup.py install downloads and installed scipy=1.4.1; conda installed scipy=1.1.0 at the start.

Possible resolution

1.) One of the ways to eliminate the extra step is to remove the conda environment setup and instead let python (which will default to pip) take care of the installation. This is a relatively simple fix given that a requirements.txt exists. The only issue is the potential out-of-sync between the variable install_requires and requirements.txt.

2.) Another way to solve the problem would be to not use python setup.py install at all. This is probably a better approach given most users will setup their environment via conda anyway. However, looking through the logs suggests that conda is broken — possibly linked to https://github.com/conda/conda/issues/9268 tho I cannot confirm atm — and scipy=1.1.0,dask=0.19.2,pandas=0.23.4 was installed when python=3.5. This is grossly out of date as we have dask>=2.0.0,pandas>=1.0.0 for the other python versions.

3.) Third option which I can think of is to embed requirements.txt directly into setup.py. This is not a complicated, and the only drawback is pip tries to make use of the latest version in a very aggressive manner, something of an issue for backward compatibility without further setup in travis.

twomagpi commented 1 month ago

Replaced (most of) setup.py with a simpler pyproject.toml file and moved to github actions fully for CI in #114.