search

ESCOMP / CAM-SIMA

Community Atmosphere Model - System for Integrated Modeling of the Atmosphere
4 stars 12 forks source link

Need to restrict CAMDEN python version to 3.7 or later #156

Closed nusbaume closed 2 years ago

nusbaume commented 2 years ago

There are several python features and code-cleanup requests that would be beneficial to apply in CAMDEN, but which require python 3.7 or later. Thus it would be helpful to explicitly require that CAMDEN use a python version that is newer or equal to version 3.7, which can now be done given that CIME has now moved over entirely to python 3.

In order to make this shift and extract the maximum set of benefits, several modifications should be made in the existing CAMDEN code and repo. These are:

  1. Need to update the CIME tag to at least cime5.8.44 or later. Otherwise python 2 can mistakenly be called during runtime which will cause CAMDEN to fail if the code changes below are implemented.

  2. Need to remove python 3.5 and 3.6 checks from Github Actions workflow, because they will fail if these listed changes are implemented.

  3. Add a python 3.7 check in ConfigCAM, and remove all other python version checks (e.g. the allow_abrev option should now always be present and set to False).

  4. Remove the (object) line from ConfigGen.

  5. Remove REGEX_TYPE = type(re.compile(r" ")) and replace it with re.Pattern in the instance-checking code.

  6. Replace current string.format(X) code with f-strings, and re-enable the consider-using-f-string check in the .pylintrc file.

Any other possible version-dependent changes that are found during development should also be added to this issue, at least until the version switch has been made and this issue is closed.

nusbaume commented 2 years ago

Making a note here that there are multiple python version checks in cam_config_unit_tests.py and write_init_unit_tests.py that can be removed once the 3.7 version restriction has been implemented.

nusbaume commented 2 years ago

Adding another note here that unless one is doing multi-class inheritance, the super() function does not require any (class, self) arguments in python 3 (apparently some versions of pylint/black complain about it). Might be worth removing?

nusbaume commented 2 years ago

Making yet another note to update the Github Action python scripts as well, as they too are linted, and so should conform to python 3.7 or later.

Also noting that the error message starting on line 223 in pr_mod_file_tests.py is missing the format() statement, which can be added/replaced during the python version switch as well.

gold2718 commented 2 years ago

Replace current string.format(X) code with f-strings, and re-enable the consider-using-f-string check in the .pylintrc file.

f strings seem to work in 3.6 (at least 3.6.8 on Izumi).

nusbaume commented 2 years ago

I think the only new feature we would miss with python 3.6 is the use of re.Pattern. Given that we have a work-around for it already, I am ok with dropping the minimum version down to 3.6.

If that is ok with everyone else I'll go ahead and change the issue title.

gold2718 commented 2 years ago

I think the only new feature we would miss with python 3.6 is the use of re.Pattern.

Right. I mentioned this over in ESMCI/cime#4116 I think we should bump the CAMDEN version to 3.7 once that is resolved. In the meantime, since this only affects testing at the moment, I think we are stable with 3.6, just make sure you use >= 3.7 when running tests.