drop py36 support #4163
drop py36 support #4163
Conversation
docs/src/conf.py
Outdated
|
|
||
| def _dotv(version): | ||
| result = version | ||
| match = re.match("^py(\d+)$", version) # noqa: W605 |
There was a problem hiding this comment.
Could be a raw string instead of overriding the linter?
There was a problem hiding this comment.
|
|
||
| #: 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"]) |
There was a problem hiding this comment.
Could /should this use the same discovery code that you've written for the docs?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "The source data contains no field(s) for 'orography'." | ||
| ) | ||
| warn.assert_called_once_with(msg) | ||
| warn.assert_called_with(msg) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'll see if I can replicate this locally...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I’ve seen this elsewhere, and it went away when tests were re-spun.
|
I'm happy with this, are you all done with changes @bjlittle? |
|
@jamesp Go for it 🚀 |
|
I'll nuke the lockfile btw... in-bound, hand tight |
|
@jamesp and @trexfeathers Awesome, thanks 🍻 |
* master: refactor setup.py to setup.cfg (SciTools#4168) update docs pypi release (SciTools#4173) Update CI environment lockfiles (SciTools#4137) update CONTRIBUTING.md (SciTools#4165) RTD support link update (SciTools#4166) drop py36 support (SciTools#4163) github issues contact link for discussions (SciTools#4164) Bump black version (SciTools#4162) Stop CI from clobbering commits on lockfile updates (SciTools#4157) [pre-commit.ci] pre-commit autoupdate (SciTools#4161) Add a method to return a CubeList from CubeList.copy() (SciTools#4094) Update black et al (SciTools#4155)
4 participants
🚀 Pull Request
Description
This PR drops
irissupport forpython3.6.x, which is now well overdue, see https://endoflife.date/python.Consult Iris pull request check list