OpenTerrace / openterrace-python

OpenTerrace: A fast, flexible and extendable Python framework for packed bed thermal energy storage simulations
https://openterrace.github.io/openterrace-python/
GNU General Public License v3.0
19 stars 2 forks source link

Installation issue #182

Closed AdamRJensen closed 2 months ago

AdamRJensen commented 2 months ago

I created a clean conda environment based on Python 3.11:

conda create -n openterrace python=3.11

and subsequently tried to pip install openterrace:

pip install openterrace

However, I was met with the following error: image

jakobhaervig commented 2 months ago

Thanks for trying out the installation. Make sure to carefully follow all steps in the installation. It seems like you are missing pip in the conda environment. To create the conda environment you have to do:

conda create -n ot python=3.11.8 pip

AdamRJensen commented 2 months ago

Pip works fine for all other packages: image

Admittedly it does work when I specify python=3.11.8

What are the python tolerances on the package?

AdamRJensen commented 2 months ago

The issue seems to be that the package does not work on the latest 3.11 version (3.11.9). When I create a 3.11.9 environment as such:

conda create -n openterrace3 python=3.11.9 pip

the same error as previous shows up:

image

AdamRJensen commented 2 months ago

I think that the specification of specific dependencies version is too strict:

https://github.com/OpenTerrace/openterrace-python/blob/b132b1ad70dfae34174a9716fffe79924d5db33a/pyproject.toml#L39-L44

I think that these should be minimum versions and not fixed versions. The use of the ^ symbol makes it a minimum version. For example,

[tool.poetry.group.dev.dependencies]
pytest = "^6.0.0"
jakobhaervig commented 2 months ago

Thanks for the suggestion :-)

Long story short: Initially I did specify minimum versions but I ran into trouble trouble with dependencies and especially the Numba version supplied with Conda. Also, in .github/workflows/pytest.yml I extract Python version from the pyproject.toml on line 36. Specifying a minimum Python version number complicates this line if the version isn't hardcoded pyproject.toml. Any ideas welcome here!

For the above reasons I decided to simply fix all versions and suggest installing OpenTerrace into a virtual environment (which I think is best practice anyway).

If you see any specific reason to loosen up the versioning let me know and I'll spend some time on this. Otherwise, I suggest sticking with fixed versions.

AdamRJensen commented 2 months ago

Conda can be a bit tricky, which is why I tend to prefer pip tbh.

I'm convinced that it is rather poor practice to be this restrictive in the dependencies when there's no extraordinary reason. In fact, I've never encountered a package that required a specific minor version of python

Similar to how there's a matrix for different operating systems, I also suggest using a matrix for python versions:

    os: [ubuntu-latest, macos-latest, windows-latest]
    python-version: [3.8, 3.9, "3.10", "3.11", "3.12"]
jakobhaervig commented 2 months ago

Thanks, I found a way to loosen up version req. Now only minimum versions are specified throughout.

Regarding the test matrix, I prefer to keep it nice and tidy by letting it read Python version from pyproject.toml by python-version-file: ['pyproject.toml']

AdamRJensen commented 2 months ago

Works like a charm now with python 3.11.9 - thanks!

It is a sure thing that people will be using different versions of Python to run the package, it's a good idea with the version matrix. However, it's not a blocker.

jakobhaervig commented 2 months ago

I agree! But specifying e.g. "3.12" will only test the newest current version/minor version, e.g. 3.12.3.. What if people are using 3.12.2? Wouldn't the test matrix have to include all micro versions as well? It shouldn't break it but we never know :)

AdamRJensen commented 2 months ago

I agree! But specifying e.g. "3.12" will only test the newest current version/minor version, e.g. 3.12.3.. What if people are using 3.12.2? Wouldn't the test matrix have to include all micro versions as well? It shouldn't break it but we never know :)

The third digit denotes the "patch version", and according to the semantic version guidelines should only include "backward compatible bug fixes". Therefore, it's a fairly safe assumption to only test minor versions (e.g., 3.10, 3.11)

jakobhaervig commented 2 months ago

@AdamRJensen, https://github.com/OpenTerrace/openterrace-python/commit/6de0fc88397b45d4f8ea7b3eda449f1798e17c3c now tests ["3.9", "3.10", "3.11", "3.12"]. I would expect noone else than the reviewer to continuously update this in future when 3.13 is released ;-)