Replace pyke nopyke

[pp-mo]

🚀 Pull Request

Encompasses all of the work in #4170, but further changes now complete the removal of pyke, switch all loading to 'new' mechanisms, and simplify all the testing that used to perform new/old comparison testing.

I probably should have squashed the commits from #4170 , but instead I squashed what was formerly here to aid rebase/merge.

---

Consult Iris pull request check list

[pp-mo]

Sorry, for clarity should be targetting the #4170 branch ... will replace ...

[pp-mo]

Reopened because pp-mo#69 won't run tests.

[pp-mo]

Rebased, to see how it goes

[pp-mo]

re-merged with auto commit from pre-commit.ci, and added small change to run local pre-commit process

? This seems to have been necessary to satisfy CI pre-commit process after rebasing, as it won't apply automatic Black changes.
Or maybe it's because the black version updated ??

[trexfeathers]

@pp-mo from what I can see it was isort changes that needed to be applied.

[pp-mo]

@trexfeathers from what I can see it was isort changes that needed to be applied.

Actually, the failing 'pre-commit' initially showed a merge-check failure.
But now it seems to be showing something different -- as you can see by navigating from the red cross.
Maybe it was due to change and I looked at it too soon (it had not finished all other tests).

I don't really understand why we seem to be doing this twice : once in the "pre-commit.ci" test, and once in the "black, flake8 and isort". I thought one test had made the other obsolete ?
Can you explain what this actually means @trexfeathers ?

[trexfeathers]

I don't really understand why we seem to be doing this twice : once in the "pre-commit.ci" test, and once in the "black, flake8 and isort". I thought one test had made the other obsolete ?
Can you explain what this actually means @trexfeathers ?

In short: no 😄. I actually raised the same question with @bjlittle earlier today.

[bjlittle]

I'll take a look at this... the only advantage of doing it locally is that the round-trip time to detecting a failure and fixing it is shorter i.e., on your git commit you find a problem, rather than committing, pushing and waiting for the pre-commit.ci bot to barf

I'll see what I can do... perhaps there is a compromise here, but I agree it making developer git commits painful

[pp-mo]

Rebased.
next intending to merge these changes back into #4170
See comment there

[pp-mo]

making developer git commits painful

nothing particularly to do with this PR.
But a key problem now is that nothing happens automatically when you rebase - e.g. latest on #4170
Which is one time I really did prefer to let the CI do the work.

[pp-mo]

I have now merged this with #4170
I had to squash the previous commits here, but it was not that complicated.
A certain amount of manual resolution was needed, in view of subsequent changes and additions on #4170 .
This should now work, but we'll see if tests pass ...

[pp-mo]

Ooh. That's nasty.
The recently-added testcase for what happens if you have a variable with two hybrid coords is failing under py37 and ok under py38.
But it turns out there is an indeterminacy in the order in which it processes the coords, presumably down to the use of sets in netcdf.py.
So for me, it literally succeeds on some runs and fails on others ...

$ python -m unittest discover ./lib/iris/tests/unit/fileformats/nc_load_rules
..........................................F............................................................................................................./home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/_nc_load_rules/helpers.py:643: UserWarning: Ignoring netCDF variable 'sound_frequency' invalid units '♫'
  warnings.warn(msg)
.....................................
======================================================================
FAIL: test_two_formulae (actions.test__hybrid_formulae.Test__formulae_tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/net/home/h05/itpp/git/iris/iris_main/lib/iris/tests/unit/fileformats/nc_load_rules/actions/test__hybrid_formulae.py", line 239, in test_two_formulae
    formula_terms=["a", "b", "depth", "eta", "orog", "sigma"],
  File "/net/home/h05/itpp/git/iris/iris_main/lib/iris/tests/unit/fileformats/nc_load_rules/actions/test__hybrid_formulae.py", line 117, in check_result
    self.assertEqual(cube._formula_type_name, factory_type)
AssertionError: 'atmosphere_hybrid_height_coordinate' != 'ocean_sigma_coordinate'
- atmosphere_hybrid_height_coordinate
+ ocean_sigma_coordinate


----------------------------------------------------------------------
Ran 189 tests in 2.954s

FAILED (failures=1)

$ python -m unittest discover ./lib/iris/tests/unit/fileformats/nc_load_rules
......................................................................................................................................................../home/h05/itpp/git/iris/iris_main/lib/iris/fileformats/_nc_load_rules/helpers.py:643: UserWarning: Ignoring netCDF variable 'sound_frequency' invalid units '♫'
  warnings.warn(msg)
.....................................
----------------------------------------------------------------------
Ran 189 tests in 2.481s

OK
$ 

( and the same for running just that specific test :
python ./lib/iris/tests/unit/fileformats/nc_load_rules/actions/test__hybrid_formulae.py Test__formulae_tests.test_two_formulae )

And I presume, the old code is quite probably doing the same, so the load will randomly return one coord with a factory and not the other.
Ouch 😱

[pp-mo]

Nearly there. I'm just scanning what is introduced, and correcting some comments.

[stephenworsley]

I suspect that the test currently failing is due to a failure elsewhere. The warning message causing a failure seems to be raised due to garbage collection of an Animation object (probably created during this test

iris/lib/iris/tests/experimental/test_animate.py

Line 54 in 9268ca9

def test_cube_animation(self):

). I would imagine that the error can be considered unrelated and would go away after respinning the test, though it may be worth raising another issue to investigate the source.

[stephenworsley]

Shouldn't this line be removed from setup.cfg?

iris/setup.cfg

Line 126 in af13e14

compiled_krb,

[stephenworsley]

Also, shouldn't

iris/pyproject.toml

Lines 35 to 36 in 9a06485

	"compiled_krb",
	"fc_rules_cf.krb",

also be removed?

[pp-mo]

Shouldn't this line [[iris/setup.cfg:: 26]] be removed from setup.cfg?
Also, shouldn't .. [[iris/pyproject.toml:: 35-36]] also be removed?

👍 Whoops.

I think missed these, because they don't mention Pyke directly.
I'll do another search + fix those things...

[pp-mo]

Thanks @stephenworsley .
Hope I have now addressed all the outstanding comments.
Please re-review.

[pp-mo]

Some further small changes, as requested.
Thanks for your conscientious application + patience @stephenworsley !

[stephenworsley]

I think this looks good now, all my comments have been addressed and I'd be happy with merging this now.