Type annotations / Compatibility with python <= 3.8

[sjoerd-bouma]

Type annotations were (?) introduced in python 3.9, leading to a number of issues when using (the Python backends of) mattak with older versions of Python. This is probably very straightforward to fix: add

from __future__ import annotations

add the top of each python file that uses type annotations (or maybe we can even just add it to the top-level __init__.py). I don't have time to test this right now but it should be a one-line fix : ) @fschlueter ?

[cozzyd]

Type annotations were added in Python 3.5 as far as I understand (https://peps.python.org/pep-0484/). And fairly recently I've tested on python 3.6 with no problems (though it's possible something recent has broken things, at one point we had to remove | in favor of Union, since | as a shortcut was added much more recently. What particular construct is giving you trouble?

[fschlueter]

See the failing actions

[cozzyd]

The ones on NuRadioMC? (Or elsewhere?) I must not be looking in the right place. NuRadioMC's python 3.6 action failures seem to be unrelated to typing from a first glance (but I'm probably looking in the wrong place).

[sjoerd-bouma]

Python 3.6 has separate issues (on NuRadioMC), but for Python 3.7 (the default 'Unit tests') and Python 3.8, the RNO-G Data Reader test on NuRadioMC fails with the error message

Traceback (most recent call last):
  File "NuRadioReco/examples/RNO_data/read_data_example/read_rnog.py", line 63, in <module>
    overwrite_sampling_rate=3.2
  File "/home/runner/work/NuRadioMC/NuRadioMC/NuRadioReco/modules/io/RNO_G/readRNOGDataMattak.py", line 395, in begin
    dataset = mattak.Dataset.Dataset(station=0, run=0, data_path=dir_file, verbose=verbose, **mattak_kwargs)
  File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/mattak/Dataset.py", line 252, in Dataset
    import mattak.backends.uproot.dataset
  File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/mattak/backends/uproot/dataset.py", line 6, in <module>
    from .voltage_calibration import VoltageCalibration
  File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/mattak/backends/uproot/voltage_calibration.py", line 8, in <module>
    class VoltageCalibration(object):
  File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/mattak/backends/uproot/voltage_calibration.py", line 132, in VoltageCalibration
    fit_min : float = -1.3, fit_max : float = 0.7) -> numpy.ndarray:
TypeError: 'type' object is not subscriptable

Googling suggested this is due to the pre-3.8 implementation of type annotations, which can be backported with the import statement in the original post. I've now implemented this in #51 and verified this makes the error go away on Python 3.7.

[cozzyd]

Ok, I see, it's https://peps.python.org/pep-0585/ that is the problem (which was indeed introduced in python 3.9).

I think this is easy enough to solve by using the older syntax for things like list[int]

$ python3
Python 3.6.8 (default, Aug 10 2023, 17:01:17) 
[GCC 8.5.0 20210514 (Red Hat 8.5.0-20)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import typing
>>> def f( a : typing.Optional[list[int]] ): 
...     pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'type' object is not subscriptable
>>> def f( a : typing.Optional[typing.List[int]]):
...     pass

[cozzyd]

As of right now, python3.6.8 is the default on both the Greenland server (and the RNO-G server at UChicago) since they're both EL8, so it's more convenient if things work on it, IMO (even though 3.6 is EOL).

[sjoerd-bouma]

If you want to enforce python 3.6-compatible type annotations rather than using __future__, which doesn't seem to work on python 3.6, we should probably add a test that runs on python 3.6, because this will be easy to miss - I don't think many people actively use 3.6 still.

[cozzyd]

Yes, that's probably a good idea. Testing EL8 in general is probably the right thing to do, if that's easy? (I have next-to-no familiarity with GitHub runners...)

[sjoerd-bouma]

I don't think EL8 is available as a runner OS, unfortunately, so it will probably have to be Ubuntu 20.04. Also I'm not sure when I'll be able to implement this, it may have to wait unless someone else wants to do it - probably can just be copied from the NuRadioMC implementation. Although IIRC getting ROOT to play nice with a non-native python installation in the runner was a bit complicated.

[cozzyd]

well we can probably self-host a runner on rno-g.uchicago.edu fairly straightforwardly, which is the environment we essentially want to test on? Not sure what goes into that exactly though...

[cozzyd]

ok, I guess that's a bad idea because it would run on a pull request from anybody, even people outside our organization... https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners#self-hosted-runner-security

[cozzyd]

I guess the work around is to create a private repository only for running the runners...