Add repro payu setup check - this should pick up changes to manifests#190
Add repro payu setup check - this should pick up changes to manifests#190
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
===========================================
+ Coverage 68.39% 85.43% +17.04%
===========================================
Files 14 22 +8
Lines 829 1298 +469
===========================================
+ Hits 567 1109 +542
+ Misses 262 189 -73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thinking about this more, it could just check if any Originally I thought it is not a good idea as it'll pick up changes to |
4eea17d to
f16581b
Compare
|
Thanks @jo-basevi for the previous work and suggestions! In this case, empty manifests will not raise an issue. Also test I am not sure if this is what we want. Happy to chat and change it. |
jo-basevi
left a comment
There was a problem hiding this comment.
Thanks for adding this change in!!
Thinking about this more, I am wondering whether this should be two tests:
test_setup_reproduce_manifests
- Uses
payu setup --repro(will need to fix payu failing on empty manifests first...). This will pick up if md5 hashes have changed (so the files are different).
test_setup_unchanged_manifests
- Catches if paths or binhashes have changed using the git diff logic you have added. If the
md5's are the same, it will just need the manifests to be updated withpayu setup. This wouldn't be as large of an issue as the configurations are still using the same files but the initialpayu setupwould take longer as it needs to recalculate all the md5 hashes.
For scheduled tests for tags, only test_setup_reproduce_manifests should run, as if it fails, it will indicate something on NCI has changed.
But for PRs or scheduled tests for branches, both manifest tests should run so the manifests are always kept up to date.
| 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 or "error" in setup_result.stderr.lower(): |
There was a problem hiding this comment.
If there are exceptions or sys.exit(1) should result in non-zero code. If there is a zero code, and there's an "error" in setup_result.stderr.lower(), I wonder if it would've coming from a warning or similar but not an error that would prevent the payu run. Probably fine to leave this, but I'm not 100% sure we should fail the test for that case.
There was a problem hiding this comment.
Good point. I agree that we shouldn't let exception fail the test. In the latest commit, the code will
- raise error when payu setup returns non-zero code,
- print a warning when
erroris in stderr and payu setup exits with zero.
| text=True, | ||
| ) | ||
| diff_lines = diff_details.stdout.splitlines() | ||
| top_lines = "\n".join(diff_lines[2:12]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| for file in files: | ||
| error_message += f"Modifications are detected in {file}:\n" | ||
| diff_details = sp.run( | ||
| ["git", "diff", f"{file}"], |
There was a problem hiding this comment.
This git diff may run from a different directory, so would be safer to use git -C <path/to/repo> e.g.
"git diff -C {self.control_path} {file}"
There was a problem hiding this comment.
Thanks for pointing this out! Good point. It is implemented in the latest commit.
I agree that it is good to split |
499cd8b to
f277c4c
Compare
This PR adds a "repro_payu_setup" test marker, which selects a test that runs "payu setup" with "--reproduce" flag - This is equivalent to setting the following in
config.yaml:So setup will raise an error if any full hashes in the generated manifests do not match.
Tested this with running
model-config-tests -m repro_payu_setupon a configuration with unchanged manifests, and with a changed restart manifest (removed values from a binhash and md5), the error is:An issue is that Payu raises an error when restart manifests are empty, e.g. ACCESS-OM2:
I wonder if payu could not error here and only error if there was no pre-existing manifest and a new restart manifest was generated?
Related to #9