Fix pytest github actions #187

leila-pujal · 2024-06-25T16:21:14Z

This PR addresses the latest updates in IOData repository for finishing support for Python versions < 3.9.

Checklist

Write a good description of what the PR does.
~~Add tests for each unit of code added (e.g. function, class)~~
~~Update documentation~~
~~Squash commits that can be grouped together~~
Rebase onto master

Type of Changes

	Type
✓	🐛 Bug fix

After some tries I converged to a setup that works for python versions 3.9, 3.10, and 3.11. First I only updated the python versions on Github actions to be >= 3.9 and checking from 3.9 to 3.12 (commit 85ae589). At this point I was not specifying any version for numpy and scipy and all python versions failed on Windows with what look like was a problem with scipy eval_hermite function (https://github.com/theochem/gbasis/actions/runs/9666120325/job/26664879878). Then I saw in IOData's pytest workflow for python 3.9 that the oldest possible versions of numpy(1.22) and scipy(1.11) are installed so I tried to specify them on the workflow and with that ubuntu python 3.12 was giving strange errors but the other were runing fine so I excluded it. The last change I needed to do was to upgrade numpy from 1.22 to 1.23 because python 3.11 on ubuntu was giving some errors and upgrading numpy locally worked. I am writing this to keep track. I planning to merge this shortly but feel free to add any comments that you may find useful or relevant, @FarnazH, @PaulWAyers, @tovrstra.

tovrstra · 2024-06-26T12:37:58Z

I quickly checked the error you are getting. It is probably the following:

>>> from scipy.special import eval_hermite
>>> eval_hermite(2.0, 3.5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'eval_hermite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> eval_hermite(2, 3.5)
47.0

The first argument (order) must be of integer type. Reasonable, I guess, but it will require some fixes elsewhere in GBasis, where the array is created or modified, to make sure it is integer.

Fixing the versions of dependencies can be useful to check that the tests also pass with older versions of the library. In IOData, this is only done when testing with the oldest version of Python we test on. (The old versions of dependencies will not always be available for the newest Python version.)

leila-pujal · 2024-06-26T16:16:47Z

Hi @tovrstra, thank you so much for checking that error. Is that prompt from python in a Windows machine? I was kind of confused because the error only showed up for Windows tests and not for ubuntu. I just checked python 3.11 and 3.12 locally in my ubuntu and with scipy=1.14 and numpy=2.0.0 (including the changes done by @msricher in PR #188 ) all the tests run.

tovrstra · 2024-06-26T19:38:46Z

Not really. My most recent experience with Windows dates back to the previous century :)

This was on tested on Fedora Linux. Here is a more complete prompt with versions:

Python 3.12.3 (main, Apr 17 2024, 00:00:00) [GCC 14.0.1 20240411 (Red Hat 14.0.1-0)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from scipy.special import eval_hermite
>>> eval_hermite(2.0, 3.5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'eval_hermite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> eval_hermite(2,3.5)
47.0
>>> import scipy
>>> scipy.__version__
'1.11.3'
>>> import numpy
>>> numpy.__version__
'1.26.4'

The GitHub actions failed on Windows first, after which all the other ones were aborted, but that does not mean they would have passed.

Same test with more recent SciPy and NumPy:

Python 3.12.4 | packaged by conda-forge | (main, Jun 17 2024, 10:23:07) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from scipy.special import eval_hermite
>>> eval_hermite(2.0, 3.5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'eval_hermite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> eval_hermite(2,3.5)
np.float64(47.0)
>>> import scipy
>>> scipy.__version__
'1.14.0'
>>> import numpy
>>> numpy.__version__
'2.0.0'

Can you check that your pytest is the intended one? When you run which pytest, does it point to the one in your virtual env? If it is /usr/bin/pytest, you'll be using the NumPy and SciPy from Ubuntu, which will be a bit older.

leila-pujal · 2024-07-02T18:24:33Z

Hi @tovrstra, sorry for the late answer and thanks for your follow up. I had a vague memory you don't mingle with Windows but I just wanted to double check :). I checked what pytest I am using and it looks it takes the one from the enviroment:

(base) leila@ulysses:~$ conda activate gbasis_py312
(gbasis_py312) leila@ulysses:~$ which pytest
/home/leila/anaconda3/envs/gbasis_py312/bin/pytest
(gbasis_py312) leila@ulysses:~$

I tried your code using the python prompt and in that case I got the expected error:

Python 3.12.4 | packaged by conda-forge | (main, Jun 17 2024, 10:23:07) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from scipy.special import eval_hermite
>>> eval_hermite(2.0, 3.5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'eval_hermite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> eval_hermite(2,3.5)
np.float64(47.0)
>>> import scipy
>>> scipy.__version__
'1.14.0'
>>> import numpy
>>> numpy.__version__
'2.0.0'
>>>

But then my test pass fine and I am not sure why. I added a print of numpy and scipy versions in one of the tests and it printed the ones I posted above. By any chance did you run the test too? If so did they fail in your fedora using those versions? I was checking the other runs for ubuntu on pytest to see if the ones that use eval_hermite fail (for test_density.py the ones that use derivatives) and if I checked correctly they passed the tests before the run was aborted (https://github.com/theochem/gbasis/actions/runs/9666120325/job/26664878634). @msricher, could you try to run the gbasis tests with python 3.12 and those versions of numpy and scipy and see if they fail/pass? it could be that maybe I am actually using my base enviroment packages and I am not able to check it properly.

tovrstra · 2024-07-04T13:00:44Z

@leila-pujal No worries about the delay. I took a closer look and my impression is that the cause is already fixed in the main branch, so the error no longer appears.

If you want to bring the fix into the current PR, you can merge the main branch into this development branch and rerun the fixes. (This kind of opposite merging can be done by running git merge main on your local repository after you have switched to the fix_actions branch.) Then push the merged branch to your fork on GitHub and tests will be executed again. This procedure requires no force-push and allows you to continue working on this PR with improvements from main.

leila-pujal · 2024-07-04T15:28:36Z

@tovrstra, thanks again for your answer and for taking the time to look at this. I merged the main into the PR and tried again the newest versions for dependencies (scipy==1.14 and numpy==2.0.0) but I still got tests failing on Windows but passing on Linux . I tried scipy issues on github and found this opened only a week ago scipy/scipy#21052 . Then I downgraded numpy version and then tests passed on Windows. What do you think about this? Before checking this issue I was thinking of changing the floats to integers when using eval_hermite but it looks like maybe is on scipy side to fix it?

tovrstra · 2024-07-04T16:01:54Z

Good that you noticed the SciPy issue! I'd suggest adding version constraints in the pyproject.toml, so it is also correctly handled when gbasis is installed with pip.

See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#dependencies-and-requirements for how this can be done per operating system.

After updating pyproject.toml you can remove the line pip install numpy==1.26.0 scipy==1.14.0 from the step name: Install new dependencies for testing in pytest.yaml. This way, the tests validate that gbasis works on the newest version of dependencies when they appear (and when they are supported). For macos and linux, there is no need to set an upper limit, if I understand correctly. (To test with older versions, you still need to manually intall them first, so that's ok.)

A few other suggestions:

You can remove the lines pip install pytest pytest-md pytest-emoji pytest-cov from the workflow and include them in the [dev] dependencies, or create [actions] dependencies for them. See e.g. https://github.com/theochem/iodata/blob/main/pyproject.toml#L42
There is no set of [iodata] dependencies in pyproject.toml, which you can also add. Then you can remove the lines pip install --user git+https://github.com/theochem/iodata.git@main from the pytest workflow. It is ok to just add the qc-iodata dependency from PyPI, which will install a fairly recent pre-release.

- Constrain version dependencies for different platforms - Use python==3.9 in pytest as a test for older dependencies - Update pyproject.toml

leila-pujal · 2024-07-04T19:52:25Z

Thanks @tovrstra for your detailed answer. I think I included all the changes you suggested and now all pytests pass. I will try to figure out why pre-commit stoped working but if I don't I'll just merge this PR and check in more detail later so new PR can have proper pytests.

tovrstra · 2024-07-05T10:46:56Z

pyproject.toml

@@ -10,12 +10,14 @@ authors = [
 description = "A module for evaluating, differentiating, and integrating Gaussian functions."
 readme = "README.md"
 license = {text = "GPL-3.0-or-later"}
-requires-python = ">=3.5"
+requires-python = ">=3.7"


Suggested change

requires-python = ">=3.7"

requires-python = ">=3.9"

tovrstra · 2024-07-05T10:47:48Z

pyproject.toml

    "pytest >=2.6",
-    "scipy>=1.4",
+    "scipy>=1.5",


Suggested change

"scipy>=1.5",

"scipy>=1.11.1",

tovrstra · 2024-07-05T10:48:59Z

pyproject.toml

+    "numpy >=1.20, <2.0.0; platform_system=='Windows'",
+    "numpy >=1.20; platform_system=='Linux'",


Suggested change

"numpy >=1.20, <2.0.0; platform_system=='Windows'",

"numpy >=1.20; platform_system=='Linux'",

# Ensure the minimal versions are kept consistent with those in .github/workflows/pytest.yaml

"numpy >=1.22, <2.0.0; platform_system=='Windows'",

"numpy >=1.22; platform_system=='Linux'",

tovrstra · 2024-07-05T10:50:07Z

.github/workflows/pytest.yaml

-
+      - name: Install old versions of supported dependencies
+        if: ${{ matrix.py == '3.9'}}
+        run: pip install numpy==1.22 scipy==1.11.1 attrs==21.3.0


Suggested change

run: pip install numpy==1.22 scipy==1.11.1 attrs==21.3.0

# Ensure these versions are consistent with the minimal version requirements in pyproject.toml

run: pip install numpy==1.22 scipy==1.11.1

tovrstra · 2024-07-05T10:52:31Z

pyproject.toml

 ]
 dependencies = [
-    "numpy>=1.16",
+    "numpy >=1.20, <2.0.0; platform_system=='Windows'",
+    "numpy >=1.20; platform_system=='Linux'",
    "pytest >=2.6",


Suggested change

"pytest >=2.6",

(already listed in dev depdencies)

tovrstra · 2024-07-05T10:56:04Z

I just added some nitty-gritty comments.

It is probably better to defer the pre-commit issue to another pull request. I quickly tried to run pre-commit run --all and it cleans up a lot of stuff. That's more suited for a separate PR.

We haven't activated pre-commit.ci yet for this repo, so it is not checked yet in PRs. If you like, I can already switch it on.

tovrstra · 2024-07-05T11:02:24Z

I created an issue to discuss pre-commit. This way, you can proceed with the current PR and merge it sooner.

leila-pujal · 2024-07-05T14:55:31Z

I just pushed a commit with all your suggestions. I will merge this one today so at least pytest is set up properly. About the pi-commit bot, thanks for the information! It was really strange because it worked before and all of a sudden it didn't so it was difficult for me to understand what happened. I am learning on the go and I understand what pi-commit bot does but sometimes setting up these continuous integration tools in the remote repo is not as easy as to use them in the command line locally. Anyway, we can talk more about this in the issue you opened. Thanks for all your help!

tovrstra

Thanks for this. Getting CI to work can be tricky.

FarnazH

Thank you, @leila-pujal and @tovrstra, for making this work! It looks great to me.
My only concern is dropping support for Python<3.9 because, then again, we cannot have HORTON2 and HORTON3 installed in the same environment. We have projects that still use them together and this is problematic and inconvenient. However, as iodata no longer supports Python<3.9, this change is inevitable. I agree that it's best to make these changes sooner rather than later, but as we have worked on package development a lot this year, I'd like to spend more time on actually using them than package-update fixes. Despite this, it’s probably wise to adapt sooner rather than later.

tovrstra · 2024-08-08T14:11:42Z

@FarnazH I agree that this is a bit difficult. Once Matt has merged his HORTON2 update, it should not be too hard to upgrade it to more recent Python 3 versions. My reason for not supporting 3.8 in IOData is that in a few months Python 3.8 is end of life. See https://devguide.python.org/versions/

leila-pujal added 5 commits June 25, 2024 12:15

Exclude python versions < 3.9

16f9c3e

Change master to main when installing IOData

85ae589

Install specific versions of numpy and scipy

06be16d

Exclude python 3.12

3d5dbb0

Upgrade numpy version in pytest

640ff5b

leila-pujal mentioned this pull request Jun 26, 2024

Change IOData git master branch to main in GitHub Actions #189

Closed

5 tasks

leila-pujal mentioned this pull request Jul 3, 2024

Add functions for 1rdms and 2-body exchange hole #165

Open

5 tasks

leila-pujal added 4 commits July 4, 2024 10:20

Merge branch 'master' into fix_actions

c7ea60c

Check again newer versions for dependencies

3059d2a

Fix available python versions

30e6f9b

Check numpy version with newest scipy

b777cf7

leila-pujal added 2 commits July 4, 2024 11:39

Update pre-commit-config.yaml

3463501

Try to fix pre-commit bot

4cb7c08

leila-pujal added 3 commits July 4, 2024 15:16

Update pyproject.toml and pytest.yaml

478f174

- Constrain version dependencies for different platforms - Use python==3.9 in pytest as a test for older dependencies - Update pyproject.toml

Delete duplicate optional-dependencies

60b5a37

Minor syntax fixes

30ac798

leila-pujal added 3 commits July 4, 2024 16:22

Revert changes in pre-commit-config.yaml

abd745c

Update actions version

8b7868a

Try original .pre-commit-config.yaml

150c034

tovrstra reviewed Jul 5, 2024

View reviewed changes

pyproject.toml Outdated

"pytest >=2.6",

"scipy>=1.4",

"scipy>=1.5",

Copy link

Member

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

"scipy>=1.5",

"scipy>=1.11.1",

tovrstra reviewed Jul 5, 2024

View reviewed changes

tovrstra mentioned this pull request Jul 5, 2024

[REQUEST] Get pre-commit.ci up and running (without errors) #192

Open

leila-pujal added 2 commits July 5, 2024 10:42

Finalize update pyproject.toml and pytest.yaml

9915d59

Revert to no changes for .pre-commit-config.yaml

7b45ca1

tovrstra approved these changes Jul 5, 2024

View reviewed changes

leila-pujal mentioned this pull request Jul 10, 2024

Fix pre-commit errors (#192) #193

Open

5 tasks

leila-pujal requested a review from FarnazH July 10, 2024 15:19

leila-pujal mentioned this pull request Aug 2, 2024

Fix compatibility with changes in IOData (prepare_segmented) #196

Merged

7 tasks

FarnazH approved these changes Aug 7, 2024

View reviewed changes

leila-pujal merged commit 1dcc4f6 into theochem:master Aug 7, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pytest github actions #187

Fix pytest github actions #187

leila-pujal commented Jun 25, 2024

leila-pujal commented Jun 25, 2024

tovrstra commented Jun 26, 2024

leila-pujal commented Jun 26, 2024

tovrstra commented Jun 26, 2024

leila-pujal commented Jul 2, 2024

tovrstra commented Jul 4, 2024

leila-pujal commented Jul 4, 2024

tovrstra commented Jul 4, 2024

leila-pujal commented Jul 4, 2024

tovrstra Jul 5, 2024

tovrstra Jul 5, 2024

tovrstra Jul 5, 2024

tovrstra Jul 5, 2024

tovrstra Jul 5, 2024

tovrstra commented Jul 5, 2024

tovrstra commented Jul 5, 2024

leila-pujal commented Jul 5, 2024

tovrstra left a comment

FarnazH left a comment

tovrstra commented Aug 8, 2024

		"numpy >=1.20, <2.0.0; platform_system=='Windows'",
		"numpy >=1.20; platform_system=='Linux'",

-    "numpy >=1.20, <2.0.0; platform_system=='Windows'",
-    "numpy >=1.20; platform_system=='Linux'",
+    # Ensure the minimal versions are kept consistent with those in .github/workflows/pytest.yaml
+    "numpy >=1.22, <2.0.0; platform_system=='Windows'",
+    "numpy >=1.22; platform_system=='Linux'",

	run: pip install numpy==1.22 scipy==1.11.1 attrs==21.3.0
	# Ensure these versions are consistent with the minimal version requirements in pyproject.toml
	run: pip install numpy==1.22 scipy==1.11.1

Fix pytest github actions #187

Fix pytest github actions #187

Conversation

leila-pujal commented Jun 25, 2024

Checklist

Type of Changes

Related

leila-pujal commented Jun 25, 2024

tovrstra commented Jun 26, 2024

leila-pujal commented Jun 26, 2024

tovrstra commented Jun 26, 2024

leila-pujal commented Jul 2, 2024

tovrstra commented Jul 4, 2024

leila-pujal commented Jul 4, 2024

tovrstra commented Jul 4, 2024

leila-pujal commented Jul 4, 2024

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

tovrstra Jul 5, 2024

Choose a reason for hiding this comment

tovrstra commented Jul 5, 2024

tovrstra commented Jul 5, 2024

leila-pujal commented Jul 5, 2024

tovrstra left a comment

Choose a reason for hiding this comment

FarnazH left a comment

Choose a reason for hiding this comment

tovrstra commented Aug 8, 2024