PP/GRIB load rules as a function

[rhattersley]

This PR was motivated by the discussion at: https://groups.google.com/d/msg/scitools-iris-dev/4GXbePUWtMY/oqVO_3gFFoAJ

7.5% reduction in the overall time for pp.load_cube, or a 9.5% reduction in the field-to-cube time.

That result still holds true in this branch.

Plus there's an added benefit, the rule processing system becomes a whole load easier to read and debug!

Sadly, there's not yet much reduction in the total complexity of the rule system. That's because of the need to keep the custom user-rules capability around for a while. But at least the normal processing is definitely simpler and more easily debugged/profiled.

NB. Also see SciTools/iris-code-generators#3 for the code which produced the new rules modules.

[rhattersley]

Yes - this uses a private function. My "justification" is that the version on master uses iris.fileformats.pp._load_rules so this is a like-for-like swap. But I'll be the first to admit it's a weak defence!

[bblay]

I don't think you need justification, it should do whatever it must to test the unit.

[bblay]

OK, maybe it's not a unit test, that restriction would make sense for system and acceptance tests but I still think it's fine.
However, the name is misleading, it should probably be make_bare_cube or make_data_cube or something indicating it's in the nooddy.

[bblay]

A lot of the GribWrapper code is just there to service the old rules.
There's probably a Smörgåsbord of readability / debuggability / hygene gains to be made if that were to be refactored together with these rules but that's probably for another ticket.

[rhattersley]

There's probably a Smörgåsbord of readability / debuggability / hygene gains to be made if that were to be refactored together with these rules but that's probably for another ticket.

Indeed. NB. @metarelate is also in this space. (See SciTools/iris-code-generators#1 for an early example). So close collaboration is essential (e.g. with @marqh) to ensure we don't waste effort or close any doors unwittingly.

[bblay]

Again, probably out of scope, we don't seem to need iris.fileformats.rules.Loader. It just adds more obfuscation.

[rhattersley]

Again, probably out of scope, we don't seem to need iris.fileformats.rules.Loader. It just adds more obfuscation.

I agree Loader doesn't need to exist. But I think it makes to sense to keep this PR as small and focussed as possible so I think we should leave it alone for now.

[bblay]

(trivial) If you're doing any more pushes I'd consider removing this comment.

[bblay]

There's nothing that tests Loader.legacy_custom_rules.
I'd be easily convinced we don't care.

[rhattersley]

There's nothing that tests Loader.legacy_custom_rules.

Au contraire:

• test_grib_load.py TestGribLoad.test_custom_rules
• test_pp_to_cube.py TestPPLoadRules.test_custom_rules

I'd be easily convinced we don't care.

I don't think anyone's trying to convince you. 😉

[bblay]

Au contraire:

Ahh, sorry. That top one is mine, too!

[bblay]

There's probably a Smörgåsbord of readability / debuggability / hygene gains to be made if that were to be refactored together with these rules but that's probably for another ticket.

Indeed. NB. @metarelate is also in this space. (See SciTools/iris-code-generators#1 for an early example). So close collaboration is essential (e.g. with @marqh) to ensure we don't waste effort or close any doors unwittingly.

Good thinking. I started a discussion about readability here.

[rhattersley]

@bblay - given the somewhat controversial nature of this change (to other people, even if not to you 😉) it would have been better to hold off on merging until this PR had some other opinions.

Plus, it would make more sense to merge SciTools/iris-code-generators#3 before this one. As things stand it's not possible for someone to make changes to the rules as they'd need to re-run the code generator ... which doesn't exist on master in its repo yet!

[rhattersley]

@bblay - given the somewhat controversial nature of this change (to other people, even if not to you 😉) it would have been better to hold off on merging until this PR had some other opinions.

That said - thank you for taking the time to review this PR and spot what I'd overlooked.

[bblay]

As things stand it's not possible for someone to make changes to the rules as they'd need to re-run the code generator

Hmm. I'm probably wrong, but I don't see the point.
The original rules were hand-crafted text. They're essentially hand-crafted Python now.
There's no sudden reason to edit the original rules elsewhere (yet) is there?

[rhattersley]

The original rules were hand-crafted text. They're essentially hand-crafted Python now.
There's no sudden reason to edit the original rules elsewhere (yet) is there?

The new rules modules are not intended to be hand-crafted text. That's something which I should have made clear in the new modules but it slipped through the net. They are intended to be automatically created using a utility in iris-code-generators - so I'll add that as feedback to SciTools/iris-code-generators#3.

And yes, there is a reason to edit the rules files already - see #619.

[bblay]

And yes, there is a reason to edit the rules files already - see #619.

I meant there's no sudden reason we shouldn't be editing the Python you just made, in Iris,
just as we would have edited the old text.
I can't see any benefit but it seems it's already been decided?

[bblay]

I'm concerned I've merged this without being aware of these extra restrictions being imposed alongside it.
I would not have merged this had I known, it really needs a group discussion, sorry.

[cpelley]

In future, might be worth writing a more suitable PR description or commit message perhaps. I'm having to look at the code changes themselves to understand whether this is applicable to be added to the 'whatsnew' (Id prefer not to have to read the google groups discussion either for context).

[bblay]

The title kind of covers it all for this one, the load rules are now encoded as a function rather than text files.
There was another side-effect that we're currently having to edit rules outside of Iris.