Skip to content

drop py36 support #4163

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 2, 2021
Merged

drop py36 support #4163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c6e90c9
drop py36 support
bjlittle Jun 1, 2021
8150691
add a whatsnew entry
bjlittle Jun 1, 2021
d13f203
review actions
bjlittle Jun 1, 2021
410a534
warn assert oddness on ci
bjlittle Jun 1, 2021
8a81860
update pyproject.toml
bjlittle Jun 1, 2021
40f447c
remove py36-linux-64.lock file
bjlittle Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,6 @@ test_minimal_task:
only_if: ${SKIP_TEST_MINIMAL_TASK} == "" && ${SKIP_ALL_TEST_TASKS} == ""
<< : *CREDITS_TEMPLATE
matrix:
env:
PY_VER: 3.6
env:
PY_VER: 3.7
env:
Expand All @@ -159,8 +157,6 @@ test_full_task:
only_if: ${SKIP_TEST_FULL_TASK} == "" && ${SKIP_ALL_TEST_TASKS} == ""
<< : *CREDITS_TEMPLATE
matrix:
env:
PY_VER: 3.6
env:
PY_VER: 3.7
env:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/refresh-lockfiles.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ jobs:

strategy:
matrix:
python: ['36', '37', '38']
python: ['37', '38']

steps:
- uses: actions/checkout@v2
Expand Down
37 changes: 32 additions & 5 deletions docs/src/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@

# ----------------------------------------------------------------------------

import datetime
import ntpath
import os
from pathlib import Path
import re
import sys
import warnings

import iris


# function to write useful output to stdout, prefixing the source.
Expand Down Expand Up @@ -53,9 +59,6 @@ def autolog(message):
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.

import datetime
import warnings

# custom sphinx extensions
sys.path.append(os.path.abspath("sphinxext"))

Expand All @@ -80,8 +83,6 @@ def autolog(message):
# |version| and |release|, also used in various other places throughout the
# built documents.

import iris

# The short X.Y version.
if iris.__version__ == "dev":
version = "dev"
Expand All @@ -101,9 +102,35 @@ def autolog(message):

build_python_version = ".".join([str(i) for i in sys.version_info[:3]])


def _dotv(version):
result = version
match = re.match(r"^py(\d+)$", version)
if match:
digits = match.group(1)
if len(digits) > 1:
result = f"{digits[0]}.{digits[1:]}"
return result


# Automate the discovery of the python versions tested with CI.
python_support = sorted(
[fname.stem for fname in Path(".").glob("../../requirements/ci/py*.yml")]
)

if not python_support:
python_support = "unknown Python versions"
elif len(python_support) == 1:
python_support = f"Python {_dotv(python_support[0])}"
else:
rest = ", ".join([_dotv(v) for v in python_support[:-1]])
last = _dotv(python_support[-1])
python_support = f"Python {rest} and {last}"

rst_epilog = f"""
.. |copyright_years| replace:: {copyright_years}
.. |python_version| replace:: {build_python_version}
.. |python_support| replace:: {python_support}
.. |iris_version| replace:: v{version}
.. |build_date| replace:: ({datetime.datetime.now().strftime('%d %b %Y')})
"""
Expand Down
3 changes: 1 addition & 2 deletions docs/src/installing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ any WSL_ distributions.

.. _WSL: https://docs.microsoft.com/en-us/windows/wsl/install-win10

.. note:: Iris is currently supported and tested against Python ``3.6``,
``3.7``, and ``3.8``.
.. note:: Iris is currently supported and tested against |python_support|.

.. note:: This documentation was built using Python |python_version|.

Expand Down
14 changes: 10 additions & 4 deletions docs/src/whatsnew/latest.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ This document explains the changes made to Iris for this release
:title: text-primary text-center font-weight-bold
:body: bg-light
:animate: fade-in
:open:

The highlights for this major/minor release of Iris include:
The highlights for this minor release of Iris include:

* N/A
* We've dropped support for `Python 3.6`_

And finally, get in touch with us on `GitHub`_ if you have any issues or
feature requests for improving Iris. Enjoy!
Expand All @@ -27,8 +28,8 @@ This document explains the changes made to Iris for this release
#. Congratulations to `@jamesp`_ who recently became an Iris core developer
after joining the Iris development team at the `Met Office`_. 🎉

#. A special thanks goes to `@akuhnregnier`_, `@gcaria`_, `@jamesp`_ and
`@MHBalsmeier`_ all of whom made their first contributions to Iris, which
#. A special thanks goes to `@akuhnregnier`_, `@gcaria`_, `@jamesp`_, `@MHBalsmeier`_
and `@Badboy-16`_ all of whom made their first contributions to Iris, which
were gratefully received and included in this release. Keep up the awesome
work! 🍻

Expand Down Expand Up @@ -168,6 +169,10 @@ This document explains the changes made to Iris for this release
templates to remove an external URL reference that caused un-posted user issue
content to be lost in the browser when followed. (:pull:`4147`)

#. `@bjlittle`_ dropped `Python 3.6`_ support, and automated the discovery of
supported Python versions tested by `cirrus-ci`_ for documentation.
(:pull:`4163`)

.. comment
Whatsnew author names (@github name) in alphabetical order. Note that,
core dev names are automatically included by the common_links.inc:
Expand All @@ -191,6 +196,7 @@ This document explains the changes made to Iris for this release
.. |PyPI| image:: https://img.shields.io/pypi/v/scitools-iris?color=orange&label=pypi%7Cscitools-iris
.. _PyPI: https://pypi.org/project/scitools-iris/
.. _Python 3.8: https://www.python.org/downloads/release/python-380/
.. _Python 3.6: https://www.python.org/downloads/release/python-360/
.. _README.md: https://github.com/SciTools/iris#-----
.. _xxhash: http://cyan4973.github.io/xxHash/
.. _conda-lock: https://github.com/conda-incubator/conda-lock
2 changes: 1 addition & 1 deletion lib/iris/tests/integration/test_pp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ def test_hybrid_height_round_trip_no_reference(self):
"Unable to create instance of HybridHeightFactory. "
"The source data contains no field(s) for 'orography'."
)
warn.assert_called_once_with(msg)
warn.assert_called_with(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned that we got this error before you made the change:

FAIL: test_hybrid_height_round_trip_no_reference (iris.tests.integration.test_pp.TestVertical)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/lib/iris/tests/integration/test_pp.py", line 417, in test_hybrid_height_round_trip_no_reference
    warn.assert_called_once_with(msg)
  File "/tmp/cirrus-ci-build/.nox/tests/lib/python3.7/unittest/mock.py", line 888, in assert_called_once_with
    raise AssertionError(msg)
AssertionError: Expected 'warn' to be called once. Called 2 times.

Doesn't this mean behaviour has changed? Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if I can replicate this locally...

Copy link
Member Author

@bjlittle bjlittle Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, so frustratingly I can't recreate this failure locally. Either with a test environment that I resolve myself from the py37 dependencies, nor with the py37 lock...

I'm not overly concerned about this... although, I'm not loving this inconsistency in behavour. It could be down to a nose test race condition... dunno.

That said, I'd be more worried if the warning wasn't being generated.

So, personally, I'm not seeing this as a blocker to merging unless you guys want to pursue it further.

Copy link
Member

Choose a reason for hiding this comment

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

I think I’ve seen this elsewhere, and it went away when tests were re-spun.


# Check the data cube is set up to use hybrid height.
self._test_coord(
Expand Down
2 changes: 1 addition & 1 deletion noxfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
PACKAGE = str("lib" / Path("iris"))

#: Cirrus-CI environment variable hook.
PY_VER = os.environ.get("PY_VER", ["3.6", "3.7", "3.8"])
PY_VER = os.environ.get("PY_VER", ["3.7", "3.8"])
Copy link
Member

Choose a reason for hiding this comment

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

Could /should this use the same discovery code that you've written for the docs?

Copy link
Member

Choose a reason for hiding this comment

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

What we're actually testing in nox is not a specific version of python, but a specific environment that is defined by a yaml file in requirements/ci.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm I'll give it a try and see what it looks like... the hardest part is deciding on where such common code will live.

But yeah, that would be a nice win 👍

Copy link
Member

Choose a reason for hiding this comment

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

@bjlittle I'm happy for this to go in as-is with your latest updates. Let me know if you plan to rework otherwise I'm merging today

Copy link
Member Author

Choose a reason for hiding this comment

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

@jamesp Okay, so I gave this a reasonably punt... but you swiftly run into a bit of a pickle as everything under the requirements directory isn't installed as a package, so you can't really safely reference it from nox or the docs without jumping through some hoops.

It is possible, and I suspect we'd either want to re-structure our resources or use setuptools sub-packages to do that properly... but I'm not really inclined to pursue this ATM, unless you want me to follow through on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this is an issue. Sure, we run our tests on a installed instance of Iris, so there are concerns on inclusion/exclusion there. But as far as I was aware Nox was expected to operate on a checkout of a repo, rather than within an install. Indeed I've based a few things on this assumption.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, you must be referencing requirements somewhere from nox as those environments yamls are being read to create the test envs. But I think I understand your point to be that the introspection bit of nox where it works out what sessions it has might not be able to safely operate. either way though, nox isn't going to work if the requirements/ci folder isn't there.

This isn't a big issue, happy to leave it hard coded.

Copy link
Member Author

@bjlittle bjlittle Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just to be clear... the issue isn't about whether the directory exists, per se... rather that I was attempting to bank common code under requirements/__init__.py for both the docs and nox to use - that's the problem here.

The simple alternative is to put common code within iris (which I really don't want to do) or I duplicate the code within the noxfile and within the docs, or we have a subpackage for iris.... or some other option.

So it's do-able, just whatever you prefer... if you want to follow-thru with this

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way, I don't think this should be a blocker for this PR. We can happily bank this PR "as is", and iterate on this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, for me this is all becoming a bit of a moot point, as a developer still requires to manually update the matrix for the minimal and full tests.


#: Default cartopy cache directory.
CARTOPY_CACHE_DIR = os.environ.get("HOME") / Path(".local/share/cartopy")
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.black]
line-length = 79
target-version = ['py36', 'py37', 'py38']
target-version = ['py37', 'py38']
include = '\.pyi?$'
exclude = '''

Expand Down
Loading