icon4py week 2026-W33 review

.github/workflows/icon4py-weekly-review.yml

@@ -0,0 +1,78 @@
+name: Weekly icon4py review
+
+on:
+  schedule:
+    # Sunday at 22:00 Europe/Zurich
+    - cron: "0 22 * * 0"
+      timezone: "Europe/Zurich"
+  workflow_dispatch:
+  # TODO: Remove this before merge.
+  pull_request:
+
+permissions:
+  contents: write
+  pull-requests: write
+
+concurrency:
+  group: icon4py-weekly-review
+  cancel-in-progress: false
+
+env:
+  TZ: Europe/Zurich
+
+jobs:
+  review:
+    runs-on: ubuntu-latest
+    timeout-minutes: 180
+    steps:
+      - name: Checkout icon4py-knowledge
+        uses: actions/checkout@v4
+
+      - name: Checkout icon4py
+        uses: actions/checkout@v4
+        with:
+          repository: C2SM/icon4py
+          ref: main
+          path: icon4py-checkout
+          fetch-depth: 1
+          persist-credentials: false
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y bubblewrap
+
+      - name: Enable unprivileged user namespaces
+        run: |
+          # Ubuntu 24.04 (the current ubuntu-latest image) restricts
+          # unprivileged user namespaces via AppArmor
+          # (kernel.apparmor_restrict_unprivileged_userns=1), so bwrap fails
+          # with "setting up uid map: Permission denied" because the
+          # bubblewrap package ships no AppArmor profile. Relax the
+          # restriction for this job. See
+          # https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces
+          sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
+          sudo sysctl -w kernel.unprivileged_userns_clone=1 || true
+
+      - name: Install uv
+        uses: astral-sh/setup-uv@v6
+
+      - name: Install Node.js
+        uses: actions/setup-node@v4
+        with:
+          node-version: '22.19'
+
+      - name: Install pi
+        run: npm install -g @earendil-works/pi-coding-agent@0.80.6
+
+      - name: Smoke test
+        run: |
+          pi --version
+          bwrap --version | head -1
+
+      - name: Run review
+        env:
+          CSCS_INFERENCE_API_KEY: ${{ secrets.CSCS_INFERENCE_API_KEY }}
+          ICON4PY_CHECKOUT: ${{ github.workspace }}/icon4py-checkout
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: ./scripts/icon4py-review/run.sh --commit-and-pr

.github/workflows/weekly-slack-summary.yml

@@ -9,6 +9,8 @@ on:
 
 permissions:
   contents: read
+  issues: read
+  pull-requests: read
 
 jobs:
   weekly-summary:
@@ -18,32 +20,27 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v4
 
-      - name: Install uv
-        uses: astral-sh/setup-uv@v7
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y bubblewrap
+
+      - name: Install Node.js
+        uses: actions/setup-node@v4
         with:
-          enable-cache: true
+          node-version: '22.19'
 
       - name: Install pi
-        run: npm install -g --ignore-scripts @earendil-works/pi-coding-agent
+        run: npm install -g @earendil-works/pi-coding-agent@0.80.6
 
-      - name: Configure pi for CSCS inference
+      - name: Smoke test
         run: |
-          mkdir -p ~/.pi/agent
-          cp .github/workflows/weekly-slack-summary/settings.json ~/.pi/agent/settings.json
-          cp .github/workflows/weekly-slack-summary/models.json ~/.pi/agent/models.json
+          pi --version
+          bwrap --version | head -1
 
-      - name: Generate weekly summary
+      - name: Generate and post summary
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           CSCS_INFERENCE_API_KEY: ${{ secrets.CSCS_INFERENCE_API_KEY }}
-        run: |
-          pi -p --approve \
-            --tools read,bash,edit,write,grep,find,ls \
-            --skill .github/workflows/weekly-slack-summary \
-            "Generate the weekly Slack activity summary for icon4py-knowledge and write it to weekly_slack_summary.md"
-          test -s weekly_slack_summary.md
-
-      - name: Post summary to Slack
-        env:
           SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
-        run: ./scripts/post-slack-summary.py weekly_slack_summary.md
+        run: ./scripts/post-slack-summary/run.sh

.gitignore

@@ -2,6 +2,9 @@
 *.local.md
 .claude/settings.local.json
 
+# Python cache
+__pycache__/
+
 # Obsidian
 .obsidian/
 

README.md

@@ -53,3 +53,6 @@ cd /tmp/quartz-preview && npx quartz build --serve   # http://localhost:8080
 
 - **Weekly Slack summary**:— `.github/workflows/weekly-slack-summary.yml` posts a
   Monday-morning summary of the last calendar week's activity to Slack.
+- **Weekly icon4py review**: `.github/workflows/icon4py-weekly-review.yml` runs an
+  automated review of C2SM/icon4py and opens a PR with accepted
+  findings. See [`scripts/icon4py-review/README.md`](scripts/icon4py-review/README.md).

content/index.md

@@ -10,6 +10,10 @@ topics a document discusses — scan them to find overlapping or conflicting ide
 See `AGENTS.md` in the repository root for how to add a proposal and keep this
 index current. (Keep entries and their keywords in sync with each document's `tags`.)
 
+## Automated review tracker
+
+- [[review/index|Weekly icon4py review tracker]] — keywords: review, correctness, performance, issues
+
 ## Shared
 
 Proposals the group broadly agrees are implementation-ready.

content/review/index.md

@@ -0,0 +1,25 @@
+---
+title: Automated icon4py review tracker
+---
+
+<!-- This file is auto-generated by scripts/icon4py-review/update-index.py. Edits will be overwritten. -->
+
+This page tracks automated review findings for C2SM/icon4py. Individual issue files live in `review/issues/`. Weekly overview reports live in `review/reports/`.
+
+## Open issues
+
+- [[review/issues/icon4py-2026-08-10-1440baa.md|Global reductions issue a redundant MPI.Allreduce and device synchronization on every min/max/sum/mean call]] - severity: high, tags: allreduce, global-reduction, gpu, hot-path, mpi, synchronization
+- [[review/issues/icon4py-2026-08-10-89ecd75.md|PPM4GPU integer vertical tracer flux sums out-of-bounds K levels at the top and bottom]] - severity: high, tags: k-offset, out-of-bounds, ppm, tracer-advection, vertical-advection
+- [[review/issues/icon4py-2026-08-10-8de4796.md|PPM4GPU fractional vertical tracer flux sums out-of-bounds K levels at the top and bottom]] - severity: high, tags: k-offset, out-of-bounds, ppm, tracer-advection, vertical-advection
+
+## Fixed issues
+
+_No fixed issues._
+
+## Invalid issues
+
+_No invalid issues._
+
+## Reports
+
+- [[review/reports/2026-08-10-1905.md|Weekly icon4py review 2026-08-10]]

content/review/issues/icon4py-2026-08-10-1440baa.md

@@ -0,0 +1,47 @@
+---
+id: icon4py-2026-08-10-1440baa
+title: Global reductions issue a redundant MPI.Allreduce and device synchronization
+  on every min/max/sum/mean call
+issue_status: open
+severity: high
+confidence: high
+fingerprint: performance:model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py:GlobalReductions._calc_buffer_size:redundant-global-allreduce
+tags:
+- allreduce
+- global-reduction
+- gpu
+- hot-path
+- mpi
+- synchronization
+created: '2026-08-10'
+updated: '2026-08-10'
+last_seen: '2026-08-10'
+source:
+  repo: C2SM/icon4py
+  ref: main
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+  file: model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py
+  lines:
+  - 403
+  - 453
+  symbol: GlobalReductions._calc_buffer_size
+found_by:
+- icon4py-performance-reviewer
+run_id: weekly-2026-W33
+history:
+- date: '2026-08-10'
+  event: detected
+  run_id: weekly-2026-W33
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+---
+## Summary
+
+In the distributed (MPI) reduction implementation, every call to `GlobalReductions.min`, `.max`, `.sum`, and `.mean` invokes `_calc_buffer_size`, which itself performs a full `_reduce` — i.e. a second `mpi4py.MPI.Allreduce` plus, on a GPU backend, a `cuda.runtime.deviceSynchronize()`. The result is used only to detect the degenerate globally-empty-buffer case and raise `ValueError`. In normal production runs the buffer is never empty, so each global reduction does 2 `MPI.Allreduce` calls and 2 device-wide synchronizations instead of 1 + 1. This directly hits the per-timestep hot path: `Icon4pyDriver._adjust_ndyn_substeps_var` calls `self.global_reductions.max(...)` once per timestep whenever `nonhydrostatic` is configured (the default dycore), over a 0-d scalar (`max_vertical_cfl[()]`), so the second `Allreduce` is over a single integer every timestep. Global allreduce is a synchronizing collective and a primary MPI scaling bottleneck; the extra `deviceSynchronize()` additionally drains the GPU pipeline and defeats kernel overlap/concurrency at scale.
+
+## Evidence
+
+`_calc_buffer_size` (mpi_decomposition.py:403-408) returns `self._reduce(array_ns.asarray([buffer.size]), array_ns.sum, mpi4py.MPI.SUM)`, i.e. it triggers a second `self.process_props.comm.Allreduce(...)` preceded by `array_ns.cuda.runtime.deviceSynchronize()` (mpi_decomposition.py:399-401). `min`/`max`/`sum` only consume this value as `if self._calc_buffer_size(buffer) == 0: raise ValueError(...)` (mpi_decomposition.py:413, 424, 435), so in the non-empty case the second collective is computed and discarded. `mean` (mpi_decomposition.py:446) reuses `global_buffer_size` as a divisor but still runs it as a separate collective instead of folding the count into the reduction. Hot-path caller `standalone_driver.py:452` runs `global_max_vertical_cfl = self.global_reductions.max(...)` every timestep via `_adjust_ndyn_substeps_var`, which `time_integration` calls whenever `self.config.nonhydrostatic is not None`. The single-node `SingleNodeReductions` has no such redundant collective, confirming the cost is specific to production MPI/GPU runs.
+
+## Suggested fix
+
+Skip the global size check on the common non-empty path. For `min`/`max`/`sum`, only compute a global non-empty flag when the local buffer is already empty, e.g. guard with `if buffer.size == 0:` and then (and only then) reduce a single `int(buffer.size > 0)` with `MPI.MAX`/`MPI.SUM` to decide whether to raise `ValueError`. Alternatively, cache the global owned count per dimension/shape, computed once, instead of recomputing it via an `Allreduce` on every call. For `mean`, which legitimately needs the count as divisor, either cache the count or fuse `sum` and `count` into a single `Allreduce` over a 2-element buffer with a custom reduction op, instead of issuing two separate collectives. These changes halve the number of global collectives and remove the avoidable `deviceSynchronize()` from the per-timestep reduction while preserving the existing all-empty `ValueError` behaviour.

content/review/issues/icon4py-2026-08-10-89ecd75.md

@@ -0,0 +1,46 @@
+---
+id: icon4py-2026-08-10-89ecd75
+title: PPM4GPU integer vertical tracer flux sums out-of-bounds K levels at the top
+  and bottom
+issue_status: open
+severity: high
+confidence: high
+fingerprint: correctness:model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_integer_flux.py:_sum_neighbor_contributions_all:oob-k-offset-read
+tags:
+- k-offset
+- out-of-bounds
+- ppm
+- tracer-advection
+- vertical-advection
+created: '2026-08-10'
+updated: '2026-08-10'
+last_seen: '2026-08-10'
+source:
+  repo: C2SM/icon4py
+  ref: main
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+  file: model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_integer_flux.py
+  lines:
+  - 34
+  - 57
+  symbol: _sum_neighbor_contributions_all
+found_by:
+- icon4py-correctness-reviewer
+run_id: weekly-2026-W33
+history:
+- date: '2026-08-10'
+  event: detected
+  run_id: weekly-2026-W33
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+---
+## Summary
+
+`_sum_neighbor_contributions_all` builds the integer-flux contribution by selecting `p_cc(dims.KDim + n) * p_cellmass_now(dims.KDim + n)` for `n = 0..4` upward and `p_cc(dims.KDim - (n+1)) * p_cellmass_now(dims.KDim - (n+1))` for `n = 0..4` downward, each gated by `where(mask1 & js_gtN, …, 0.0)` / `where(mask2 & js_gtN, …, 0.0)` (lines 34-43) with `js = floor(abs(z_cfl)) - 1`. The stencil runs over `KDim = (1, num_levels)` (`tracer_advection_vertical.PiecewiseParabolicMethod`), but `p_cc` (the tracer field) and `p_cellmass_now` (airmass/rhodz) are allocated with only `num_levels` K levels and no extension (`tracer_states.initialize_tracer_state`, `tracer_advection_states.AdvectionDiagnosticState`). At `k = num_levels - 1` the upward reads `p_cc(dims.KDim + 1..4)` access indices `num_levels..num_levels+3` (past the end); at `k = 1` the downward reads `p_cc(dims.KDim - 2..5)` access indices `-1..-4` (before the start). Because `js_gtN` is `True` whenever `|vertical CFL| > N + 1`, these out-of-bounds values are selected and added into `prod_jks` (lines 50-56) and hence into `z_iflx`/`p_upflux` (lines 84-88), corrupting the vertical tracer mass flux at the top and bottom levels. On the `roundtrip`/embedded backend the positive out-of-bounds read raises `IndexError` (crash); on `gtfn_cpu` the negative index wraps/reads adjacent memory and the positive index reads adjacent or past-array memory (wrong values). The stencil is annotated `# TODO(dastrm): this stencil has no test` and `# this stencil does not strictly match the fortran code`.
+
+## Evidence
+
+The dycore guards the same K-neighbour pattern with `concat_where` plus a K-extended `w` field (`compute_advection_in_vertical_momentum_equation`, `prognostic_state.initialize_prognostic_state`), confirming that plain `where` evaluates both branches and that the `dims.KDim ± n` accesses here are out of bounds for a non-extended field. The stencil is driven with `vertical_start=1, vertical_end=num_levels`, so `k = num_levels - 1` (and `k = 1`) are always reached. The integration test (`test_standalone_driver.py`) uses a balanced reference with small boundary vertical mass flux (`|CFL| < 2`, so `js_gt1` is `False` and the reads are masked) and stays bit-exact; the corruption appears only during strong vertical convection, which is routine with `ndyn_substeps > 1` because the tracer step uses the full physics time step with the dycore substep-averaged mass flux.
+
+## Suggested fix
+
+Use `concat_where` keyed on `slev <= k <= nlev - 1 - n` (and the symmetric lower bound for the downward branch) so each `p_cc(dims.KDim ± n)` / `p_cellmass_now(dims.KDim ± n)` access is evaluated only where it is in bounds, matching the dycore's handling. Alternatively extend the tracer and airmass/rhodz fields by enough K levels to cover the maximum `±5` offset and ensure the selection default reproduces ICON's boundary behaviour.

content/review/issues/icon4py-2026-08-10-8de4796.md

@@ -0,0 +1,46 @@
+---
+id: icon4py-2026-08-10-8de4796
+title: PPM4GPU fractional vertical tracer flux sums out-of-bounds K levels at the
+  top and bottom
+issue_status: open
+severity: high
+confidence: high
+fingerprint: correctness:model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_fractional_flux.py:_sum_neighbor_contributions:oob-k-offset-read
+tags:
+- k-offset
+- out-of-bounds
+- ppm
+- tracer-advection
+- vertical-advection
+created: '2026-08-10'
+updated: '2026-08-10'
+last_seen: '2026-08-10'
+source:
+  repo: C2SM/icon4py
+  ref: main
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+  file: model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_fractional_flux.py
+  lines:
+  - 33
+  - 55
+  symbol: _sum_neighbor_contributions
+found_by:
+- icon4py-correctness-reviewer
+run_id: weekly-2026-W33
+history:
+- date: '2026-08-10'
+  event: detected
+  run_id: weekly-2026-W33
+  commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351
+---
+## Summary
+
+`_sum_neighbor_contributions` selects `p_cc(dims.KDim + n)` for `n = 0..4` upward and `p_cc(dims.KDim - (n+1))` for `n = 0..4` downward, each gated by `where(mask1 & js_eqN, …, 0.0)` / `where(mask2 & js_eqN, …, 0.0)` (lines 33-42) with `js = floor(abs(z_cfl))`. The stencil runs over `KDim = (1, num_levels)` (`tracer_advection_vertical.PiecewiseParabolicMethod`), but `p_cc` (tracer) and `p_cellmass_now` (airmass/rhodz) are allocated with only `num_levels` K levels and no extension (`tracer_states.initialize_tracer_state`, `tracer_advection_states.AdvectionDiagnosticState`). At `k = num_levels - 1` the upward reads `p_cc(dims.KDim + 1..4)` access indices `num_levels..num_levels+3` (past the end); at `k = 1` the downward reads `p_cc(dims.KDim - 2..5)` access indices `-1..-4` (before the start). Because `js_eqN` is `True` whenever `floor(|vertical CFL|) == N`, these out-of-bounds values are selected and summed into `p_cc_jks` (lines 44-54), then into `z_q_int` and the fractional flux `p_upflux` (lines 84-97), corrupting the vertical tracer flux at the top and bottom levels. On the `roundtrip`/embedded backend the positive out-of-bounds read raises `IndexError` (crash); on `gtfn_cpu` the negative index wraps/reads adjacent memory and the positive index reads adjacent or past-array memory (wrong values). The stencil is annotated `# TODO(dastrm): this stencil has no test` and `# this stencil does not strictly match the fortran code`.
+
+## Evidence
+
+As with the integer-flux and Courant-number cases, the dycore guards the identical pattern with `concat_where` and a K-extended `w` field (`compute_advection_in_vertical_momentum_equation`, `prognostic_state.initialize_prognostic_state`), confirming that plain `where` reads out of bounds here. The stencil is driven with `vertical_start=1, vertical_end=num_levels`, so both boundary levels are reached. The balanced integration reference (`|vertical CFL| < 2`, so `js_eq1..4` are `False` at the boundaries) masks the bug and keeps the comparison bit-exact; it surfaces only during strong vertical convection, which is routine with `ndyn_substeps > 1` because the tracer step uses the full physics time step with the dycore substep-averaged mass flux.
+
+## Suggested fix
+
+Use `concat_where` keyed on `slev <= k <= nlev - 1 - n` (and the symmetric lower bound for the downward branch) for each `p_cc(dims.KDim ± n)` access, matching the dycore. Alternatively extend the tracer and airmass/rhodz fields by enough K levels to cover the maximum `±5` offset and align the selection default with ICON's boundary behaviour.