Remove `dummy_exchange_with_bound_dim` by Copilot · Pull Request #1177 · C2SM/icon4py

Copilot · 2026-03-30T09:27:26Z

Instead just use the already existing SingleNodeExchange which is a noop. This also fixes the mismatch in function parameters where exchange: Callable[...] = single_node_exchange is wrong, with single_node_exchange not being the correct type of callable.

jenkins-apn · 2026-03-30T09:27:30Z

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

jenkins-apn · 2026-03-30T09:27:30Z

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

…NodeExchange.exchange Remove the standalone dummy_exchange_with_bound_dim function and replace it with noop_exchange = functools.partial(single_node_default.exchange, dims.CellDim), which relies on the exchange objects directly as suggested in the issue. SingleNodeExchange.exchange is inherently a noop, making this a proper replacement. Updated all 9 test files that referenced the old function. Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/4b7a4e7b-4b0b-474f-b345-1d9cae17543d Co-authored-by: jcanton <5622559+jcanton@users.noreply.github.com>

havogt · 2026-04-01T07:26:34Z

This change doesn't make sense...

havogt · 2026-04-01T07:27:15Z

But probably because I already did the deduplication in a previous PR and it didn't understand that there is nothing to do... Now it's more or less a rename but not for the better.

msimberg · 2026-04-15T07:59:07Z

But probably because I already did the deduplication in a previous PR and it didn't understand that there is nothing to do... Now it's more or less a rename but not for the better.

Agree on the current changes not being an improvement. What I'd like to see is the dummy exchange removed, and all places where an exchange callable with a bound Dimension is expected be changed to that they take a plain ExchangeRuntime and the factory/whatever can supply the Dimension instead (I think it makes anyway more sense that the caller doesn't have to know which Dimension is going to get exchanged).

Probably even better: do #1136 first. If and only if there are still numpy functions that need to do the exchange internally, do the change I suggest above.

msimberg · 2026-04-15T08:33:14Z

Work on #1136 is ongoing in #1195. Let's resolve that first before continuing here.

…ummy-exchange-definition # Conflicts: # model/common/tests/common/grid/unit_tests/test_topography.py # model/common/tests/common/grid/unit_tests/test_vertical.py # model/common/tests/common/interpolation/unit_tests/test_interpolation_fields.py # model/common/tests/common/interpolation/unit_tests/test_rbf_interpolation.py # model/common/tests/common/metrics/unit_tests/test_compute_coeff_gradekin.py # model/common/tests/common/metrics/unit_tests/test_compute_weight_factors.py # model/common/tests/common/metrics/unit_tests/test_compute_zdiff_gradp.py # model/common/tests/common/metrics/unit_tests/test_metric_fields.py

…xchanges

github-actions · 2026-05-06T15:24:49Z

Mandatory Tests

Please make sure you run these tests via comment before you merge!

cscs-ci run default
cscs-ci run distributed

Optional Tests

To run benchmarks you can use:

cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

msimberg · 2026-05-07T07:15:30Z

cscs-ci run default

msimberg · 2026-05-07T07:15:35Z

cscs-ci run distributed

OngChia · 2026-05-07T07:23:30Z

    n_edges: int,
    nlev: int,
-    exchange: Callable[[data_alloc.NDArray], None],
+    exchange: decomposition.ExchangeRuntime,


Can I know why only this one does not need the default single node exchange?

I don't know the original reason, I just didn't want to change it.

But now that you bring this up, I'd probably remove the default in all the other cases as well?

I commented above for another function which does nto have teh single exchange as default. Probably we didn't set it here because we missed it, but I do prefer that we set up the single node as default everywhere

Hmm. I think that I will also vote for removing the default, because users should specify the exchange that they want to avoid unexpected results.

I'd propose to not make that decision in this PR since that's orthogonal to the main purpose of this PR which is to remove the dummy_exchange helper. I'm happy to follow up with another PR based on what we discuss here.

I agree with @OngChia. Using the current "default" is almost always a mistake in user code. There the decision should be explicit. Additionally, having single node exchange as the default for convenience in tests is I think adding convenience for the wrong target audience. If one has to choose between a good API for users and a good API for testing/development, the good API for users should always have precedence. (If one can satisfy both that's even better, but I think in this case helping users avoid making the mistake of not specifying the type of exchange has precedence).

It should possibly not even be a choice which type of exchange to do. The SingleNodeExchange is IMO an implementation detail that exists to avoid loading mpi4py. GHEX will happily not do an exchange if there's only one rank. So I think we could do a better job abstracting this away. Additionally, I'm toying with the idea of passing a ProcessProps in places like this instead, which is in the end used to decide whether an exchange should be done. But these two last points are definitely not for now, let's focus on default/no default.

@OngChia @nfarabullini are you ok with me resolving this comment? I'll open a separate PR for removing the default exchange and for the rest we need a bit more thinking...

Yes, I agree. Let's do it in another PR.

OngChia · 2026-05-07T07:23:55Z

    horizontal_start: gtx.int32,
    horizontal_start_1: gtx.int32,
-    exchange: Callable[[data_alloc.NDArray], None],
+    exchange: decomposition.ExchangeRuntime,


this one too

nfarabullini

I agree with the changes. I just left some comments here and there.

nfarabullini · 2026-05-07T07:43:10Z

    n_edges: int,
    nlev: int,
-    exchange: Callable[[data_alloc.NDArray], None],
+    exchange: decomposition.ExchangeRuntime,


I commented above for another function which does nto have teh single exchange as default. Probably we didn't set it here because we missed it, but I do prefer that we set up the single node as default everywhere

nfarabullini · 2026-05-07T07:43:24Z

    horizontal_start: gtx.int32,
    horizontal_start_1: gtx.int32,
-    exchange: Callable[[data_alloc.NDArray], None],
+    exchange: decomposition.ExchangeRuntime,


OngChia

LGTM.

* main: Remove `dummy_exchange_with_bound_dim` (#1177) py2fgen: all_bindings generate .f90 and .c separately (#1253) Update Plottingscripts (#1239) Fix default nox sessions (#1243) Improve return code handling in py2fgen generated code (#1245)

* save_json_nml_serialization: pre-commit small review Remove `dummy_exchange_with_bound_dim` (#1177) py2fgen: all_bindings generate .f90 and .c separately (#1253) Update Plottingscripts (#1239) Fix default nox sessions (#1243) Improve return code handling in py2fgen generated code (#1245)

Initial plan

7646bc0

Copilot AI assigned Copilot and jcanton Mar 30, 2026

Copilot started work on behalf of jcanton March 30, 2026 09:27 View session

Copilot AI linked an issue Mar 30, 2026 that may be closed by this pull request

Deduplicate dummy_exchange definition #1152

Closed

Copilot AI changed the title ~~[WIP] Deduplicate dummy_exchange definition~~ Replace dummy_exchange_with_bound_dim with noop derived from SingleNodeExchange Mar 30, 2026

Copilot finished work on behalf of jcanton March 30, 2026 09:46

Copilot AI requested a review from jcanton March 30, 2026 09:46

Merge branch 'main' into copilot/deduplicate-dummy-exchange-definition

129f7ed

msimberg changed the title ~~Replace dummy_exchange_with_bound_dim with noop derived from SingleNodeExchange~~ Remove dummy_exchange_with_bound_dim May 6, 2026

msimberg added 2 commits May 6, 2026 15:09

Remove dummy_exchange completely, use single_node_exchange for noop e…

2926979

…xchanges

msimberg marked this pull request as ready for review May 6, 2026 15:27

msimberg requested review from OngChia, nfarabullini and yiluchen1066 May 6, 2026 15:27

OngChia reviewed May 7, 2026

View reviewed changes

nfarabullini reviewed May 7, 2026

View reviewed changes

msimberg requested a review from OngChia May 7, 2026 14:30

msimberg requested a review from nfarabullini May 7, 2026 14:30

msimberg changed the title ~~Remove dummy_exchange_with_bound_dim~~ Remove dummy_exchange_with_bound_dim May 7, 2026

OngChia approved these changes May 8, 2026

View reviewed changes

msimberg merged commit c0649fa into main May 8, 2026
54 checks passed

msimberg deleted the copilot/deduplicate-dummy-exchange-definition branch May 8, 2026 14:41

Conversation

Copilot AI commented Mar 30, 2026 • edited by msimberg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenkins-apn commented Mar 30, 2026

Uh oh!

jenkins-apn commented Mar 30, 2026

Uh oh!

havogt commented Apr 1, 2026

Uh oh!

havogt commented Apr 1, 2026

Uh oh!

msimberg commented Apr 15, 2026

Uh oh!

msimberg commented Apr 15, 2026

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

msimberg commented May 7, 2026

Uh oh!

msimberg commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OngChia May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msimberg May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfarabullini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OngChia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Copilot AI commented Mar 30, 2026 •

edited by msimberg

Loading

OngChia May 7, 2026 •

edited

Loading

msimberg May 7, 2026 •

edited

Loading