Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions src/model_config_tests/config_tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,18 @@ def pytest_configure(config):
"markers",
"repro_determinism_restart: mark tests that check determinism restart",
)
config.addinivalue_line(
"markers",
"repro_payu_setup: mark tests that check payu setup reproducibility",
)
config.addinivalue_line(
"markers",
"manifests: mark tests that check payu setup does not change manifests files or md5",
)
config.addinivalue_line(
"markers",
"manifests_unchanged: mark tests that check payu setup does not change manifests files",
)
config.addinivalue_line("markers", "slow: mark tests that are slow to run")
config.addinivalue_line(
"markers",
Expand Down
31 changes: 30 additions & 1 deletion src/model_config_tests/config_tests/test_bit_reproducibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import pytest

from model_config_tests.exp_test_helper import Experiments, ExpTestHelper
from model_config_tests.exp_test_helper import Experiments, ExpTestHelper, setup_exp
from model_config_tests.util import DAY_IN_SECONDS, HOUR_IN_SECONDS

# Names of shared experiments
Expand Down Expand Up @@ -328,3 +328,32 @@ def test_repro_determinism_restart(
)

assert produced == expected


@pytest.mark.repro
@pytest.mark.manifests
@pytest.mark.repro_payu_setup
def test_payu_setup_repro_flag(control_path, output_path):
"""
Test payu setup with `--repro` flag which errors if md5 of any files in payu manifests are changed.
"""
experiment = setup_exp(control_path, output_path, exp_name="repro_payu_setup")
try:
experiment.setup_reproduce()
except Exception as error:
pytest.fail(f"{error}")


@pytest.mark.manifests
@pytest.mark.manifests_unchanged
def test_manifests_unchanged(control_path, output_path):
"""
Test payu setup with `git diff` which errors if any files in payu manifests are changed.
"""
experiment = setup_exp(
control_path, output_path, exp_name="setup_unchanged_manifests"
)
try:
experiment.setup_manifests_unchanged()
except Exception as error:
pytest.fail(f"{error}")
80 changes: 80 additions & 0 deletions src/model_config_tests/exp_test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,86 @@ def has_run(self):
"""
return self.model.output_exists()

def setup_reproduce(self):
"""
Run payu setup with `--repro` flag to check if md5 hashes have changed in the manifests.
"""
owd = Path.cwd()
# Change to experiment directory and run.
os.chdir(self.control_path)

try:
setup_command = [
"payu",
"setup",
"--lab",
str(self.lab_path),
"--reproduce",
]
print(f"Running payu setup command: {setup_command}")
result = sp.run(setup_command, capture_output=True, text=True)
finally:
# Change back to original working directory
os.chdir(owd)

if result.returncode != 0:
raise RuntimeError(
f"Failed to run payu setup with --reproduce. Error: {result.stderr}\n"
f"Full output: {result.stdout}"
)

def setup_manifests_unchanged(self):
"""
Run payu setup command and check if manifests files have been changed with `git diff`.
"""
owd = Path.cwd()
# Change to experiment directory and run.
os.chdir(self.control_path)

try:
setup_command = ["payu", "setup", "--lab", str(self.lab_path)]
print(f"Running payu setup command: {setup_command}")
setup_result = sp.run(setup_command, capture_output=True, text=True)
if setup_result.returncode != 0:
raise RuntimeError(
f"Error during payu setup. \n"
f"{'='*10}STDOUT{'='*10}\n {setup_result.stdout}\n"
f"{'='*10}STDERR{'='*10}\n {setup_result.stderr}\n"
)

result = sp.run(
["git", "diff", "--name-only", "manifests/"],
capture_output=True,
text=True,
)
finally:
# Change back to original working directory
os.chdir(owd)

if result.stdout != "":
# Collect and display the top 10 lines of the diff for each modified file
files = result.stdout.strip().split("\n")
error_message = "Modifications are detected in file:\n"
error_message += "\n".join(" - " + file for file in files) + "\n"
error_message += "\nIf md5 hashes have changed, this indicates file contents being different."
error_message += """
If binhashes/paths have changed but md5's are the same,
this will mean the configuration can reproduce the manifests
but `payu setup` will take longer to run as it needs to re-calculate all the md5 hashes.
"""
for file in files:
diff_details = sp.run(
["git", "-C", str(self.control_path), "diff", f"{file}"],
capture_output=True,
text=True,
)
diff_lines = diff_details.stdout.splitlines()
top_lines = "\n".join(diff_lines[2:12])
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could get an index error if len(diff_lines) < 12 here

I am wondering whether it should just show the full git diff here? If it did show the full error, it should show the all the names of modified files at the top.

Changes detected in manifest file(s):
 - manifests/input.yml

If md5 hashes have changed, this indicates file contents being different.
If binhashes/paths have changed but md5's are the same, this will mean the configuration can reproduce the manifests but `payu setup` will take longer to run as it needs to re-calculate all the md5 hashes. 

Full git diff:
...

Also if you prefer the truncated sections, feel free to keep it too! Could start with truncated diff then if someone asks for the full diff, then it'll be easy enough to add it later

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could get an index error if len(diff_lines) < 12 here

I asked myself the same question before. So I tested the top_lines = "\n".join(diff_lines[2:12]) when diff_lines are less than 12 rows. It reads in all lines without raising error.
I even took an extreme test top_lines = "\n".join(diff_lines[2:300]). It reads in all output of git diff, no add-in empty lines and still didn't raise error. IndexError should not be an issue in this case.

I do agree that it should list all file names at the top, and then details of each file underneath. This is implemented in the latest commit.

if len(diff_lines) > 12:
top_lines += "\n... (truncated)"
error_message += f"\n{'='*10} Diff for {file} {'='*10}\n{top_lines}\n"
raise RuntimeError(f"{error_message}")

def setup_for_test_run(self):
"""
Various config.yaml settings need to be modified in order to run in the
Expand Down
73 changes: 72 additions & 1 deletion tests/test_exp_test_helper.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import shutil
import subprocess
from pathlib import Path
from unittest.mock import Mock, patch
from unittest.mock import MagicMock, Mock, patch

import pytest
import yaml
Expand Down Expand Up @@ -556,3 +556,74 @@ def test_experiments_check_experiment_error(tmp_path):
)
with pytest.raises(RuntimeError, match=error_msg):
exps.check_experiment("error_exp")


@patch("subprocess.run")
def test_setup_reproduce_error(mock_run, exp):
"""Test that payu setup --repro fails raises an error and return to original work directory"""
# Mock the payu setup --repro to fail
mock_result = MagicMock()
mock_result.returncode = 1
mock_result.stderr = "MD5 mismatch"
mock_result.stdout = "Check manifest"
mock_run.return_value = mock_result

with pytest.raises(RuntimeError) as excinfo:
exp.setup_reproduce()

assert (
f"Failed to run payu setup with --reproduce. Error: {mock_result.stderr}"
in str(excinfo.value)
)
assert f"Full output: {mock_result.stdout}" in str(excinfo.value)

# assert returning to the original work directory
assert Path.cwd() == exp.control_path


@patch("subprocess.run")
def test_setup_manifests_unchanged_fail_setup(mock_run, exp):
"""Test that an error is raised when payu setup fails in setup_manifests_unchanged()"""
# Mock the payu setup --repro to fail with unchanged manifests
mock_result = MagicMock()
mock_result.returncode = 1
mock_result.stderr = "Setup failed"
mock_result.stdout = "Payu setup output"
mock_run.return_value = mock_result

with pytest.raises(RuntimeError) as excinfo:
exp.setup_manifests_unchanged()

assert "Error during payu setup." in str(excinfo.value)
assert f"{'='*10}STDOUT{'='*10}\n {mock_result.stdout}\n" in str(excinfo.value)

# assert returning to the original work directory
assert Path.cwd() == exp.control_path


@patch("subprocess.run")
def test_setup_manifests_unchanged_show_changes(mock_run, exp):
"""Test that when manifests are changed, the `git diff` results are printed to stdout"""
# Mock the `payu setup` succeed first
setup_success = MagicMock(returncode=0, stdout="Payu setup succeeded")

# Then mock the `git diff --name-only` to show which files are changed
git_diff_name_only = MagicMock(returncode=1, stdout="manifests/input.yaml")

# Mock the `git diff` to show the detailed changes in the file
git_diff_output = (
"--- a/manifests/input.yaml\n+++ b/manifests/input.yaml\n+new line\n-old line"
)
git_diff_run = MagicMock(returncode=0, stdout=git_diff_output)

# Run these mocks in sequence
mock_run.side_effect = [setup_success, git_diff_name_only, git_diff_run]

with pytest.raises(RuntimeError) as excinfo:
exp.setup_manifests_unchanged()

assert "Modifications are detected in file:\n" in str(excinfo.value)
assert git_diff_output in str(excinfo.value)

# assert returning to the original work directory
assert Path.cwd() == exp.control_path
Loading