[Agentic] Move array hashing into a shared _combine_common substrate

[bjlittle]

🤖 Agentic pull request

This change was written by Claude (Opus 5), driven by @bjlittle. Commits carry a
Co-Authored-By trailer. Please review it as you would any other contribution —
and more sceptically, if that is your inclination.

Design Specification

See Making Iris merge and concatenate robust, efficient and configurable on the greenfield feature branch for full details.

For convenience, Readthedocs has already been configure to automatically build the greenfield feature branch documentation.

What this does

Moves the array-hashing machinery out of lib/iris/_concatenate.py and into a
new private module, lib/iris/_combine_common.py.

This is row 1 of the roadmap in the merge and concatenate design spec (#7274),
and implements §5.1 of that spec. Nothing else in the programme lands until this
one is reviewed — it is deliberately the smallest possible first step, and it is
as much a test of whether the approach is worth continuing as it is a change.

The step-by-step plan this follows landed separately in #7275, and is at
docs/src/developers_guide/plans/2026-09-10-hashing-substrate.md if you want the
provenance. You should not need it to review this: the plan is a record of how
the change was arrived at, not an argument for it.

The hashing layer was added for concatenate in #5926. Merge needs exactly the
same capability, and today cannot have it without importing from
_concatenate.py — which _concatenate.py cannot offer, because it imports
iris.cube. The new module imports nothing from Iris at runtime, so either
engine can use it with no risk of a circular import. test_imports.py enforces
that by walking the module's AST, and a second test guards the guard so the
first cannot pass by failing to look.

This is a relocation, not a rewrite

No behaviour changes. The moved code is identical to the original apart from
dropping the leading underscore on seven names, plus three lines noted below.
Two checks you can run yourself:

1. The moved block is unchanged apart from the renames:

git show upstream/greenfield:lib/iris/_concatenate.py | sed -n '305,541p' \
  | sed -e 's/\b_hash_ndarray\b/hash_ndarray/g' -e 's/\b_hash_chunk\b/hash_chunk/g' \
        -e 's/\b_hash_aggregate\b/hash_aggregate/g' -e 's/\b_hash_array\b/hash_array/g' \
        -e 's/\b_ArrayHash\b/ArrayHash/g' -e 's/\b_array_id\b/array_id/g' \
        -e 's/\b_compute_hashes\b/compute_hashes/g' > /tmp/expected.py
sed -n '/^def hash_ndarray/,$p' lib/iris/_combine_common.py > /tmp/actual.py
diff -u /tmp/expected.py /tmp/actual.py

Output is two hunks, three lines — the de-shadowing described below.

2. No concatenate test was touched. test_hashing.py moves to the new
package unchanged but for its import lines; no other test under
lib/iris/tests/unit/concatenate/ is edited.

git diff upstream/greenfield --stat -- lib/iris/tests/unit/concatenate/

Where to actually look

Everything above is mechanical. The only part that needed judgement is the
array_id rename, in two places:

• Five call sites in _concatenate.py assigned to a local named
  array_id. Once the imported function has that name, the local shadows it and
  the next line raises UnboundLocalError. The locals are now points_id and
  bounds_id, which is what they always meant.
• Three bindings inside compute_hashes itself had the same collision. It is
  inert — no scope in that module calls the function — but it is the same trap,
  sitting in the module that defines the name. They are now _ and key.

Neither ruff nor mypy catches this class of shadowing, so it is worth a human
glance.

An open question for the reviewer

Dropping the underscore prefix makes this a move and a rename, and that is a
decision I would rather you took than I did.

The case for it: these names are now a small internal API consumed by two
modules, and the underscore no longer says anything the module's own _ prefix
does not already say.

The case against: it turns a diff you could verify by eye into one you have to
read, and it is the sole cause of all eight local renames above.

If you would rather review a pure git mv with the underscores kept, say so
and I will drop the rename. The module's location is what the rest of the
programme depends on; the spelling of the names is not, and I have no stake in
it.

Testing

pytest -n auto lib/iris/tests/unit gives 7072 passed against 7070 on
greenfield — the two additions are the new import-invariant tests. The one
pre-existing failure (test__chunk_control.py::test_netcdf_v3) and ten
pre-existing errors are unchanged by this branch and unrelated to it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (greenfield@c0dc1d8). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             greenfield    #7276   +/-   ##
=============================================
  Coverage              ?   90.36%           
=============================================
  Files                 ?       94           
  Lines                 ?    25822           
  Branches              ?     4796           
=============================================
  Hits                  ?    23333           
  Misses                ?     1709           
  Partials              ?      780           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bjlittle]

Codex review (model: GPT-5): I reviewed the full change against greenfield, including the hashing relocation, all updated call sites, the renamed hashing tests, the runtime-import invariant, and the design/changelog updates. I found no actionable issues.

The relocated implementation matches the original apart from the documented symbol and local-variable renames. Repository-wide inspection found no remaining callers of the old _concatenate hashing names, git diff --check is clean, and all GitHub CI checks are passing.

On the open naming question, I recommend keeping the unprefixed names. The containing _combine_common module already marks the API as private, while points_id, bounds_id, and key communicate the local roles more clearly than the shadowing names they replace.

Residual risk is low: an external consumer importing these private symbols directly from iris._concatenate would break, but there are no such callers in the repository.

[bjlittle]

@SciTools/iris-devs Anyone remotely interested in engaging with this?

If not, I'll close it.

[bouweandela]

I doubt whether I'm the right person for reviewing, but this looks like a really great development, as merge is a very time consuming operation at the moment, especially when there are many cubes in a file.

[bouweandela]

One potential point for improvement: in my experience, human written pull request descriptions and comments are more inviting for reviewers. E.g. numpy and other popular open source projects also have a strict rule about this in their AI policy.

[pp-mo]

This also basically delivers #7241

[stephenworsley]

From @SciTools/peloton: We think this is a worthwhile move in the right direction, but with our current resources we don't expect to have the space to commit to review this in the next few months. We're finding that, currently, reviewing is very much the bottleneck with agentic code. I think we're developing our approach to this with the current work, but this seems to us like the kind of thing with a big start up cost since this is such a new paradigm. Hopefully we will be able to streamline the review process when we understand it better and I expect this ought to be something we'd be more comfortable to review in future.

[bjlittle]

@SciTools/peloton @stephenworsley

I don't understand why you are raising the bar in the standard of review applied to an agentic generated PR compared to a human. For the very first time in the history of iris you have the whole design upfront to help core developers truly understand the implications and impact of a proposed change before it happens. Also, just to clarify the design is a living document.

Note that, banking a design along with a change/s not only provides a rich resource for developers but it also helps agents understand why the (proposed) implementation is how it is, the historical decisions that were taken along the way, tasks/approaches/work deemed not appropriate or out of scope et al - the benefits are huge.

Anyways, message received ... I'll leave this PR and associated greenfield feature branch as-is, but I think this a missed opportunity that would benefit the whole community i.e., #7276 (comment)

I'm happy to discuss this further with anyone, whenever you choose to engage and want to move this forwards (or not).

[bjlittle]

This also basically delivers #7241

Hmmm not really, the intention behind this series of changes is more profound from an architectural perspective.

Have you read the proposed design @pp-mo ?

[pp-mo]

This also basically delivers #7241

Hmmm not really, the intention behind this series of changes is more profound from an architectural perspective.

Have you read the proposed design @pp-mo ?

Not fully,
I am just noting that this will resolve that specific issue

[trexfeathers]

I don't understand why you are raising the bar in the standard of review applied to an agentic generated PR compared to a human.

Sorry for the misunderstanding @bjlittle. I know this is a core concern of yours so I have been paying close attention to it! This is only day 3 of the core dev team engaging with full-fat agentic dev, so we are still getting used to the style, just as we would with any new human developer. So far I haven't seen anyone treating contributions with a different standard - we're just taking longer to understand them.

Maybe in future we'll actually be able to lower the bar compared to a human, but we obviously need to understand the first few PRs to know if that's realistic.

And since our full attention is on #6961 - another agentic workflow - we thought it would be dishonest of us to commit to greenfield at the same time, hence wanting to defer it. We want to work on it like we mean it!

Note that, banking a design along with a change/s not only provides a rich resource for developers but it also helps agents understand why the (proposed) implementation is how it is, the historical decisions that were taken along the way, tasks/approaches/work deemed not appropriate or out of scope et al - the benefits are huge.

100%. But as I said it's taking a few days to get used to it.

I'm happy to discuss this further with anyone, whenever you choose to engage and want to move this forwards (or not).

❤️❤️❤️