-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature #2669 proj #2672
Feature #2669 proj #2672
Conversation
…ate all the MET Makefile.in files.
…Also delete development.kiowa since that machine was decommissioned in June 2023.
…compiling the 50 applications that already link to -lvx_grid.
…include the Proj library in the rpath settings.
…r or not MET successfully compiles in that METbaseimage
…a4 to test with the correct base image version
…updating this feature branch to use that image when compiling via GHA.
I notice failures in this GHA run related to python embedding. Will investigate further and debug.
|
These changes failed in this GHA run. All instances of python embedding cause a segfault. I pulled the Docker image and was able to replicate the bad behavior locally:
This is the same issue that RAL-IT encountered when installing MET into /usr/local and @torle1f diagnosed the problem. Manually compiling python 3.10 (as we are doing in METbaseimage) and running pip commands introduces linking problems, as demonstrated by this ldd command:
So Python's NetCDF library doesn't know where to find the needed libraries and defaults to a bad/incompatible choice from /usr/local/lib. The fix is being more careful in the manual compilation of python, using rpath settings to tell it where to find its required libraries. |
… with GCC version provided by Debian 12.
I do note that this apparent linker problem also exists in the Comparing to the successful setup on seneca, I do notice that the custom Python 3.10 installation DOES NOT have a |
All of the unit tests finally passed although some differences were flagged in this GHA run:
In addition, the new output increases the number of derived HPBL values from 392 to 432. I inspected the new values and they look to be reasonable. But I have no good reason to offer why a newer gfortran compiler version would increase the number of derived HPBL obs by 40! That being said, the along/cross track error and HBPL observations are all derived using Fortran code. So I'm pretty confident that these diffs are all due to changing gfortran compiler versions. |
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.
@JohnHalleyGotway Thanks for your patience and perseverance on this task! I scanned through the code changes and looked at others more closely. I saw the failed check in the differences and reviewed your comment on them. It sounds like these are acceptable differences. I approve this request.
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.
These changes look good and the diffs were explained in the comments.
A note about the order of merging of this and the corresponding METbaseimage PR:
To test these changes, we had to create the dtcenter/met-base:v2.1
DockerHub tag that uses the build script from the feature_2669_proj
branch. We will want to update the dtcenter/METbaseimage Dockerfile to no longer reference the feature branch, as it will go away once this PR is merged.
We discussed that we should merge this pull request, then update the dtcenter/METbaseimage#14 branch to use develop
instead of feature_2669_proj
in the URL to the build script. We can retag v2.1
in METbaseimage after it is merged, which will kick off a rebuild of the Docker image. This last step is not mandatory, but would be good to do to ensure that we don't introduce a typo in the METbaseimage PR that we will have to track down later.
…Dockerfile here to pull compile_MET_all.sh from develop rather than the feature_2669_proj feature branch.
Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@seneca.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: Daniel Adriaansen <dadriaan@ucar.edu> Co-authored-by: John and Cindy <halleygotway@Halleys-Mac-mini.local> Co-authored-by: rgbullock <bullock@ucar.edu> Co-authored-by: Randy Bullock <bullock@seneca.rap.ucar.edu> Co-authored-by: Dave Albo <dave@seneca.rap.ucar.edu> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com> Co-authored-by: davidalbo <dave@ucar.edu> Co-authored-by: Lisa Goodrich <lisag@ucar.edu> Co-authored-by: metplus-bot <97135045+metplus-bot@users.noreply.github.com> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jonathan Vigh <jvigh@ucar.edu> Co-authored-by: Tracy Hertneky <39317287+hertneky@users.noreply.github.com> Co-authored-by: David Albo <dave@ucar.edu> Co-authored-by: Dan Adriaansen <dadriaan@ucar.edu> fix 2518 dtypes appf docs (#2519) fix 2531 compilation errors (#2533) fix #2531 compilation_errors_configure (#2535) fix #2514 develop clang (#2563) fix #2575 develop python_convert (#2576) Fix Python environment issue (#2407) fix definitions of G172 and G220 based on comments in NOAA-EMC/NCEPLIBS-w3emc#157. (#2406) fix #2380 develop override (#2382) fix #2408 develop empty config (#2410) fix #2390 develop compile zlib (#2404) fix #2412 develop climo (#2422) fix #2437 develop convert (#2439) fix for develop, for #2437, forgot one reference to the search_parent for a dictionary lookup. fix #2452 develop airnow (#2454) fix #2449 develop pdf (#2464) fix #2402 develop sonarqube (#2468) fix #2426 develop buoy (#2475) fix 2596 main v11.1 rpath compilation (#2614) fix #2514 main_v11.1 clang (#2628) fix #2644 develop percentile (#2647)
Expected Differences
This enhances MET to compile against and link to the proj library. Note that for dtcenter/METbaseimage#12, I pushed the
dtcenter/met-base:v2.1
Docker image which provides an environment for GHA in which these changes compile. The Makefiles have all been updated so that anywhere-lvx_grid
appears,-lproj
is added.Unrelated but also included:
development.kiowa
since kiowa has been decommissioned.test_const_dakota.R
since a machine-specific configuration is no longer required.Note also that the vx_grid library HAS NOT yet been updated to actually use the Proj library. @rgbullock will work on that.
Do these changes introduce new tools, command line arguments, or configuration file options? [No]
If yes, please describe:
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]
If yes, please describe:
Pull Request Testing
Describe testing already performed for these changes:
Tested compilation on seneca and via GHA with v2.1 of the MET base image.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Browse changes and confirm that MET compiles via GHA and no differences to the unit test output are flagged.
A review from @jprestop is required because of the huge implications on installation.
A review from @georgemccabe is optional to keep him in the loop on dependency changes and his knowledge of the METbaseimage setup for GHA.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Added
MET_PROJ
info to the installation chapter which should be easy to incorporate into the overhaul of that chapter that @jprestop and @j-opatz are working on.Do these changes include sufficient testing updates? [Yes]
None needed.
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Please complete this pull request review by [Wed 8/30/23].
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: Coordinated METplus-X.Y Support project for bugfix releases or MET-X.Y.Z Development project for official releases