-
Notifications
You must be signed in to change notification settings - Fork 456
EAMxx: do not reset yaml file list in eamxx-prod testmod #8004
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.
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
Conversation
Other testmods existing BEFORE eamxx-prod may have already added yaml files
mahf708
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable
|
|
||
| # set the output yaml files | ||
| output_yaml_files=$(find ${cime_root}/../components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/prod/yaml_outs/ -maxdepth 1 -type f) | ||
| $atmchange -b output_yaml_files="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to start adding them with = instead of +=? If not, I think this should be resolved at the atmchange level, i.e., if a user uses += it should default to start a fresh new list if a list doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such thing as "non-existing" entry (you can't atmchange an entry that doesn't exist). If the array is empty, it simply adds one entry and generate a length-1 array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, well, this PR may introduce issues in that this testmod will take all sorts of random files it wasn't meant to take then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only acts on the yaml files introduced by the prod testmod:
output_yaml_files=$(find ${cime_root}/../components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/prod/yaml_outs/ -maxdepth 1 -type f)Other files are not touched, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think what the testmod was doing before the latlon PR was wrong:
if [ "${file}" == "${output_yaml_files[0]}" ]; then
# First file, reset output list
$atmchange -b output_yaml_files="./$(basename ${file})"
else
# Append to output list
$atmchange -b output_yaml_files+="./$(basename ${file})"
fi The testmod assumed that all yaml files in the atm came from eamxx-prod, which is not right. It was basically nuking any existing yamlfile. I am not sure how the eamxx-output-preset-1--eamxx-prod testmods on mappy managed to work. Most likely, the ne4 res causes the 1st file to be skipped, which means the if statement above never triggered.
Anyhow, I think this PR restores things to a more consistent state.
Other testmods existing BEFORE eamxx-prod may have already added yaml files. [BFB]
Other testmods existing BEFORE eamxx-prod may have already added yaml files.
[BFB]
PR #7985 introduced this bug, causing more DIFFs than expected on cdash. I did not catch this, as the eamxx CI does not run eamxx-prod combined with other output-producing testmods.