Skip to content

Enable Cache in ICON4Py checks #493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jgphpc merged 4 commits into from
Jan 19, 2026
Merged

Enable Cache in ICON4Py checks #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12eebd7
Enable Cache in ICON4Py checks
kotsaloscv Dec 19, 2025
cb5786c
Merge branch 'main' into icon4py_cache
kotsaloscv Dec 19, 2025
9ea6cc9
Merge branch 'main' into icon4py_cache
kotsaloscv Dec 19, 2025
c43b38a
Merge branch 'main' into icon4py_cache
jgphpc Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions checks/apps/icon4py/icon4py_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#
# SPDX-License-Identifier: BSD-3-Clause

import os
import re
import reframe as rfm
import reframe.utility.sanity as sn

Expand All @@ -28,16 +30,28 @@ class ICON4PyBenchmarks(rfm.RunOnlyRegressionTest):
'HUGETLB_MORECORE': 'no',
'GT4PY_UNSTRUCTURED_HORIZONTAL_HAS_UNIT_STRIDE': '1',
'PYTHONOPTIMIZE': '2',
# GT4Py cache does not work properly for dace backend yet
# 'GT4PY_BUILD_CACHE_LIFETIME': 'persistent',
# 'GT4PY_BUILD_CACHE_DIR': '...',
'GT4PY_BUILD_CACHE_LIFETIME': 'persistent',
}
executable = './_run.sh'
executable_opts = ['2>&1']

@run_before('run')
def prepare_env(self):
gpu_arch = self.current_partition.select_devices('gpu')[0].arch

cache_folder = (
f"{os.environ.get('SCRATCH')}/"
f".cache/reframe_bencher_icon4py/"
Comment on lines +42 to +44
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache directory is constructed using environment variables that may not be set. If the SCRATCH environment variable is not set, os.environ.get('SCRATCH') will return None, resulting in a cache path starting with "None/" which is likely not the intended behavior. Consider adding validation or a fallback value.

Suggested change
cache_folder = (
f"{os.environ.get('SCRATCH')}/"
f".cache/reframe_bencher_icon4py/"
scratch_dir = os.environ.get('SCRATCH') or os.path.expanduser('~')
cache_folder = os.path.join(
scratch_dir,
'.cache',
'reframe_bencher_icon4py',

Copilot uses AI. Check for mistakes.
)
sub_folder = (
f"{self.current_system.name}="
f"{gpu_arch}="
f"{self.current_environ}"
)
Comment on lines +46 to +50
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex substitution removes all non-alphanumeric characters except '=' from the sub_folder string. However, this doesn't handle potential issues if self.current_system.name, gpu_arch, or self.current_environ contain special characters that could cause problems. More importantly, if any of these attributes could be None or empty, the resulting subfolder name could be malformed (e.g., "=="). Consider adding validation to ensure these values are populated before constructing the path.

Suggested change
sub_folder = (
f"{self.current_system.name}="
f"{gpu_arch}="
f"{self.current_environ}"
)
# Normalize components used in cache subfolder to avoid None/empty values
system_name = getattr(self.current_system, 'name', '')
if not system_name:
system_name = 'unknownsystem'
if not gpu_arch:
gpu_arch = 'unknowngpu'
if self.current_environ not in (None, ''):
env_name = str(self.current_environ)
else:
env_name = 'unknownenv'
sub_folder = f"{system_name}={gpu_arch}={env_name}"

Copilot uses AI. Check for mistakes.
sub_folder = re.sub(r'[^a-zA-Z0-9=]', '', sub_folder)
cache_folder = os.path.join(cache_folder, sub_folder)
self.env_vars['GT4PY_BUILD_CACHE_DIR'] = cache_folder

if 'gfx' in gpu_arch: # AMD GPU
self.env_vars['CUPY_INSTALL_USE_HIP'] = '1'
self.env_vars['HCC_AMDGPU_TARGET'] = gpu_arch
Expand All @@ -54,12 +68,12 @@ def validate_test(self):
(r'^\s*model/atmosphere/diffusion/tests/'
r'diffusion/integration_tests/'
r'test_benchmark_diffusion\.py'
r'::test_diffusion_benchmark\s*PASSED'
r'::test_diffusion_benchmark[\s\S]*?PASSED'
), self.stdout)
dycore_granule = sn.assert_found(
(r'^\s*model/atmosphere/dycore/tests/'
r'dycore/integration_tests/test_benchmark_solve_nonhydro\.py'
r'::test_benchmark_solve_nonhydro\[True-False\]\s*PASSED'
r'::test_benchmark_solve_nonhydro\[True-False\][\s\S]*?PASSED'
), self.stdout)
Comment on lines 73 to 77
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern uses [\s\S]? which matches any character (including newlines) in a non-greedy manner. This is a less readable way to match multi-line content. Consider using the re.DOTALL flag (or (?s) flag in the pattern) with .? instead, or ensure the regex pattern is well-tested, as pytest output between the test name and PASSED could vary significantly.

Copilot uses AI. Check for mistakes.

return diffusion_granule and dycore_granule
Expand Down
50 changes: 49 additions & 1 deletion checks/apps/icon4py/src/_install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ unset PYTHONPATH

git clone https://github.com/C2SM/icon4py.git
cd icon4py
git checkout 5485bcacb1dbc7688b1e7d276d4e2e28362c5444 # Commit: Update to GT4Py v1.1.0 (#933)
git checkout 15b7406d9385a189abaaa71b14851d1491b231f1 # Commit: Update to GT4Py v1.1.2 (#969)

# Install uv locally
curl -LsSf https://astral.sh/uv/install.sh | UV_UNMANAGED_INSTALL="$PWD/bin" sh
Expand All @@ -25,6 +25,54 @@ mpi4py_ver=$(uv pip show mpi4py | awk '/Version:/ {print $2}')
uv pip uninstall mpi4py && uv pip install --no-binary mpi4py "mpi4py==$mpi4py_ver"
uv pip install git+https://github.com/cupy/cupy.git

# Patch Gt4Py to avoid cache issues
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Patch Gt4Py" but should consistently use "GT4Py" (all caps) to match the naming convention used throughout the rest of the codebase.

Suggested change
# Patch Gt4Py to avoid cache issues
# Patch GT4Py to avoid cache issues

Copilot uses AI. Check for mistakes.
uv pip uninstall gt4py
git clone --branch v1.1.2 https://github.com/GridTools/gt4py.git
python3 -c '
import sys
from pathlib import Path

file_path = Path("gt4py/src/gt4py/next/otf/stages.py")
Comment on lines +30 to +35
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python script does not validate that the gt4py directory was successfully cloned before attempting to read and patch the file. If the git clone operation fails (e.g., due to network issues), the script will fail with a confusing error about the file not existing rather than a clear indication that the clone operation failed.

Suggested change
git clone --branch v1.1.2 https://github.com/GridTools/gt4py.git
python3 -c '
import sys
from pathlib import Path
file_path = Path("gt4py/src/gt4py/next/otf/stages.py")
if ! git clone --branch v1.1.2 https://github.com/GridTools/gt4py.git; then
echo "Error: failed to clone gt4py repository." >&2
exit 1
fi
python3 -c '
import sys
from pathlib import Path
file_path = Path("gt4py/src/gt4py/next/otf/stages.py")
if not file_path.is_file():
print(f"Error: expected file '{file_path}' does not exist. gt4py repository may not have been cloned correctly.", file=sys.stderr)
sys.exit(1)

Copilot uses AI. Check for mistakes.
lines = file_path.read_text().splitlines()

new_lines = []
iterator = iter(lines)

found = False
for line in iterator:
# 1. Detect the start of the block we want to change
if "program_hash = utils.content_hash(" in line:
found = True
# Insert the NEW pre-calculation line
# We steal the indentation from the current line to be safe
indent = line[:line.find("program_hash")]
new_lines.append(f"{indent}offset_provider_arrays = {{key: value.ndarray if hasattr(value, \"ndarray\") else value for key, value in offset_provider.items()}}")

# Add the modified content_hash call
new_lines.append(f"{indent}program_hash = utils.content_hash(")
new_lines.append(f"{indent} (")
new_lines.append(f"{indent} program.fingerprint(),")
new_lines.append(f"{indent} sorted(offset_provider_arrays.items(), key=lambda el: el[0]),")

# Skip the OLD lines from the iterator until we hit "column_axis"
# We blindly consume lines until we find the one we keep
while True:
skipped_line = next(iterator)
if "column_axis," in skipped_line:
new_lines.append(skipped_line) # Add column_axis line back
break
Comment on lines +59 to +63
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python script uses next(iterator) in a while True loop without handling potential StopIteration exceptions. If the "column_axis," line is not found in the file after detecting the "program_hash" line, this will raise an uncaught StopIteration exception instead of providing a helpful error message.

Suggested change
while True:
skipped_line = next(iterator)
if "column_axis," in skipped_line:
new_lines.append(skipped_line) # Add column_axis line back
break
column_line_found = False
for skipped_line in iterator:
if "column_axis," in skipped_line:
new_lines.append(skipped_line) # Add column_axis line back
column_line_found = True
break
if not column_line_found:
print(
'"column_axis," line not found after program_hash block; '
"patch cannot be safely applied."
)
sys.exit(1)

Copilot uses AI. Check for mistakes.
else:
new_lines.append(line)

if found:
file_path.write_text("\n".join(new_lines) + "\n")
print("Patch applied.")
else:
print("Target line not found.")
sys.exit(1)
'
uv pip install -e ./gt4py
Comment on lines +28 to +74
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline Python script is performing a brittle file patching operation by searching for specific text patterns and modifying source code. This approach is fragile and could easily break if the GT4Py library structure changes slightly (e.g., whitespace changes, code reformatting). Consider using a more robust patching mechanism such as applying a unified diff patch file, or checking if this fix has been upstreamed to GT4Py v1.1.2 or a later version.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +74
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block clones the third-party gt4py repository from GitHub using --branch v1.1.2 and then installs it editable with uv pip install -e ./gt4py, which pins a critical dependency only to a mutable Git ref rather than an immutable commit or verified release artifact. If the GridTools/gt4py repo or its v1.1.2 ref is compromised or retagged, an attacker could inject arbitrary code into this check pipeline and potentially access CI secrets or tamper with produced artifacts. To reduce supply-chain risk, pin gt4py to an immutable commit SHA or a verified release artifact (e.g., a PyPI release with hashes) instead of a branch/tag, and avoid editable installs from mutable Git references in automated environments.

Copilot uses AI. Check for mistakes.

################################################################################
# NVHPC runtime auto-discovery for serialbox (libnvhpcatm.so)
################################################################################
Expand Down
6 changes: 3 additions & 3 deletions checks/apps/icon4py/src/_run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ unset PYTHONPATH
cd icon4py
source .venv/bin/activate

pytest -v \
pytest -sv \
-m continuous_benchmarking \
--benchmark-only \
--benchmark-warmup=on \
Expand All @@ -16,8 +16,8 @@ pytest -v \
--backend=dace_gpu \
--grid=icon_benchmark_regional \
--benchmark-time-unit=ms \
model/atmosphere/diffusion/tests/diffusion/integration_tests/test_benchmark_diffusion.py::test_diffusion_benchmark \
model/atmosphere/dycore/tests/dycore/integration_tests/test_benchmark_solve_nonhydro.py::test_benchmark_solve_nonhydro[True-False]
"model/atmosphere/diffusion/tests/diffusion/integration_tests/test_benchmark_diffusion.py::test_diffusion_benchmark" \
"model/atmosphere/dycore/tests/dycore/integration_tests/test_benchmark_solve_nonhydro.py::test_benchmark_solve_nonhydro[True-False]"
echo

# Cleanup
Expand Down