Feature 2718 use python diff #3189
Conversation
…dirs containig bin
…apper script and removed comp_dir.sh--wrapper not necessary
make this branch current with develop, in case any updates affect tests
|
Created issue #3190 to ensure availability of latest METplus develop version on seneca for nightly build tests |
|
The MET tests may occasionally fail due to a diff on the 7th significant figure. This was observed in the comparison of tc_stat/ALAL2010_stat.out on values 103.88155 and 103.88154. Created issue dtcenter/METplus#3042 to improve numeric diff logic in METplus. If this error comes up frequently and consistently from these same values in these files, then it could be worth implementing at least a short-term work around to handle this particular case (independent of or prior to completion of above issue, which is broader and vaguer in scope.) |
There was a problem hiding this comment.
This looks good to me. I made a suggested change that isn't required if everything works as expected.
I plan to bring up how to obtain METplus develop to get the diff utility for nightly runs on seneca. Using what is available in /home/met_test/METplus works for now, but we may want to automate pulling the latest changes from develop or clone the repo fresh for each nightly run. Thanks for making these changes!
| import os | ||
| import sys | ||
|
|
||
| module_path = os.path.join(os.getenv('LOCAL_METPLUS_DIR'), '.github', 'jobs') |
There was a problem hiding this comment.
If we want to be consistent with the METplus GHA scripts, I used GITHUB_WORKSPACE instead of LOCAL_METPLUS_DIR (see https://github.com/dtcenter/METplus/blob/main_v6.0/.github/jobs/setup_and_run_diff.py for an example).
Actually, looking at that script, if you are only setting the path to access docker_utils, I don't think you need to set the sys.path since docker_utils.py is in the same directory as this script. Unless there are other imports from other directories, I think you could remove these lines.
There was a problem hiding this comment.
The script is being run from a MET directory, and it is importing a module from METplus (that was cloned into the working directory)
There was a problem hiding this comment.
Ah, that makes sense. I thought it was also in the MET directory. Thanks for clarifying.
Expected Differences
Do these changes introduce new tools, command line arguments, or configuration file options? [No]
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]
Pull Request Testing
Describe testing already performed for these changes:
I've run the GHA testing workflow multiple times. I've also tested comp_dir.py() on unit test output files from nightly builds in my personal environment on seneca.
I have not tested in the MET test environment on seneca.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
You can repeat similar tests to what I did.
This should also be tested in the MET test environment by running the test_nightly.sh script.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes or No]
Did not update any documentation, don't think it's needed.
Do these changes include sufficient testing updates? [Yes or No]
Will this PR result in changes to the MET test suite? [Yes]
If yes, describe the new output and/or changes to the existing output:
The diff test output (logs) will look a little different from how it previously did in MET but will be consistent with METplus.
Will this PR result in changes to existing METplus Use Cases? [No]
Do these changes introduce new SonarQube findings? [No]
Please complete this pull request review by [Fill in date].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: METplus-X.Y Support project for bugfix releases or MET-X.Y Development project for the next coordinated release