Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ci): Run tests for more python versions #296

Merged
merged 9 commits into from
Mar 8, 2024
Merged
30 changes: 26 additions & 4 deletions .github/workflows/regression.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,25 @@ jobs:
runs-on: ubuntu-20.04
strategy:
fail-fast: false
matrix:
python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11']
# 3.12 does not yet work because numpy removed numpy.distutils that pygacode relies on

steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v4

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v2
uses: actions/setup-python@v5
with:
python-version: '3.6'
python-version: ${{ matrix.python-version }}
cache: 'pip'


- name: Extra dependencies required for 3.7
if: ${{matrix.python-version == '3.7'}}
run: |
sudo apt-get update
sudo apt-get install libhdf5-dev libnetcdf-dev

- name: Install numpy
run: |
Expand All @@ -32,7 +43,18 @@ jobs:
run: |
python3 -m pip install -r requirements.txt

- name: Install OMAS
# numpy is a build time dependency of pygacode
# but before pygacode 1.0 it did not specify its build dependencies correctly
# and pygacode requires python >= 3.8,
# so for 3.7 we need to build without build isolation and manually install numpy
- name: Install OMAS (Py 3.7)
if: ${{matrix.python-version == '3.7'}}
run: |
python3 -m pip install wheel
python3 -m pip install --no-build-isolation .[machine]

- name: Install OMAS (normal)
if: ${{matrix.python-version != '3.7'}}
run: |
python3 -m pip install .[machine]

Expand Down
2 changes: 1 addition & 1 deletion omas/omas_plot.py
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,7 @@ def get2d(contour_quantity):
z = scipy.ndimage.zoom(z, sf)
value_2d = scipy.ndimage.zoom(value_2d, sf)

cs = ax.contour(r, z, value_2d, levels, **kw)
cs = ax.contour(r, z, value_2d, levels=levels, **kw)

if label_contours or ((label_contours is None) and (contour_quantity == 'q')):
ax.clabel(cs)
Expand Down
2 changes: 2 additions & 0 deletions omas/tests/failed_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@
failed_OMFIT = False
except ImportError as _excp:
failed_OMFIT = _excp
except AttributeError as _excp:
failed_OMFIT = "omfit_classes, from xarray import * bug "
hassec marked this conversation as resolved.
Show resolved Hide resolved

try:
import MDSplus
Expand Down
6 changes: 6 additions & 0 deletions omas/tests/test_omas_examples.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ def test_uncertain(self):

@unittest.skipIf(failed_OMFIT, str(failed_OMFIT))
def test_plot_g_s_2_ip(self):
# on 3.7 this test raises:
# ValueError: Number of rows must be a positive integer, not 7.0
# It seems like this is caused by a bug in omfit_classes where a float
# instead of int is passed to plot
Copy link

Choose a reason for hiding this comment

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

@orso82 Any idea if this has been fixed on the OMFIT side? I assume we should just cast it?

Copy link
Member

Choose a reason for hiding this comment

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

Probably this needs to be fixed on the OMFIT side. Could you please take care of it @kalling ?

Copy link

Choose a reason for hiding this comment

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

@orso82 Alright.
@hassec Would you mind sending me (or pointing me to) a trace of the error that occurs if you don't skip this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling no problem. It's in omfit_gapy.py which has the following lines:

        cplot = max([np.floor(np.sqrt(nplot)), 1.0])
        rplot = np.ceil(nplot / cplot)

which should probably we wrapped in int(...)

Corresponding ci run with the error is here

Copy link

@kalling kalling Mar 4, 2024

Choose a reason for hiding this comment

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

cplot is used in other calculations later that look like they may need it to be a float, so I submitted a change to OMFIT auto_merge that stores the cast versions passed in to subplot as plot_cols and plot_rows instead of doing the cast at assignment, just to be safe. Re-ran the tests locally and it took care of the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling are you sure? The latest version on pypi is from Nov 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling @orso82 How would you feel about merging as is, and I can follow up as soon as the fix on omfit_classes is released on PyPi? Or if it's coming in the next days I'm also happy to wait.

There isn't really a strong urgency, but I'd like to avoid that the PR sits too long, goes stale and gets forgotten.

Copy link

Choose a reason for hiding this comment

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

@hassec Interesting, I wonder if the 2 factor auth introduced messed up the automation for uploading to pypi. It should currently be at 3.2024.09
I'll take a look.

Copy link

Choose a reason for hiding this comment

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

Ok, I fixed the pypi uploading issue for omfit-classes and 3.2024.9.2 was uploaded. Hopefully now it'll actually be weekly again :) (Assuming tests pass)
Thanks for pointing that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kalling I'll have an update PR for OMAS ready asap to remove the workaround :)

if sys.version_info.minor==7:
raise unittest.SkipTest("Avoid Py 3.7 omfit_classes bug.")
from omas.examples import plot_g_s_2_ip

def test_plot_saveload_scaling(self):
Expand Down
31 changes: 16 additions & 15 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@
#
# usage: pip install -r requirements.txt

numpy>=1.16.1 # required
uncertainties # required
pint # required
netCDF4 # required
boto3 # required
matplotlib # required
scipy # required
h5py # required
pymongo # required
dnspython # required
xmltodict # required
xarray # required
setuptools>=41.2 # required
tqdm # required
Cython # required
numpy>=1.16.1 # required
uncertainties # required
pint # required
netCDF4 # required
boto3 # required
matplotlib # required
scipy # required
h5py # required
pymongo # required
dnspython # required
xmltodict # required
xarray # required
setuptools>=41.2 # required
tqdm # required
Cython # required
importlib_metadata <5;python_version=='3.7' # required

# omfit_classes # machine
# pexpect # machine
Expand Down
6 changes: 4 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import os
import sys
import glob
import subprocess

Expand All @@ -19,6 +18,9 @@
'setuptools>=41.2',
'tqdm',
'Cython',
# latest xarray version that works with 3.7 doesn't work
# with importlib_metadata >5 since they deprecated the `get()` method on Entrypoints
"importlib_metadata <5;python_version=='3.7'"
]

extras_require = {
Expand All @@ -44,7 +46,7 @@
f.write('# Do not edit this file by hand, operate on setup.py instead\n#\n')
f.write('# usage: pip install -r requirements.txt\n\n')
for item in install_requires:
f.write(item.ljust(25) + '# required\n')
f.write(item.ljust(50) + ' # required\n')
for requirement in extras_require:
f.write('\n')
for item in extras_require[requirement]:
Expand Down