Feature 2669 proj comp #2710
Feature 2669 proj comp #2710
Conversation
There was a problem hiding this comment.
Unfortunately, I think we need to work some more on this.
The dtcenter/METbaseimage repo uses this compile_MET_all.sh script to create the docker testing environment for MET and METplus. So I tried rebuilding that environment by:
- Cloning METbaseimage:
git clone https://github.com/dtcenter/METbaseimage
cd METbaseimage
- Updating
Dockerfileto pull from your feature branch.
cat Dockerfile | sed 's%/develop/%/feature_2669_proj_comp/%g' > Dockerfile.new
mv Dockerfile.new Dockerfile
- Building the image:
docker build -t met-base:feature_2669_proj_comp .
This errors out with on the compilation of PROJ:
Using python version:
./compile_MET_all.sh: line 397: /usr/local/bin/python3: No such file or directory
...
-- Could NOT find Python (missing: Python_EXECUTABLE Interpreter)
-- PROJ: Configured 'dist' target
-- Configuring incomplete, errors occurred!
Here's an issue...
I believe that python is required to compile sqlite or proj. But in the Dockerfile, we intentionally run compile_MET_all.sh FIRST and compile python SECOND so that we can compile python using the NetCDF library that was compiled by compile_MET_all.sh. So that's a circular dependency.
Currently, the Dockerfile does already install sqlite3 as a package, and it's available in /usr/include/sqlite3.h and /usr/lib/x86_64-linux-gnu/libsqlite3.so. So we could stick with that without relying on compile_MET_all.sh to compile it.
BUT even that is not working.
SQLITE_DIRis NOT currently set in thedevelopment.dockerenvironment file in the MET repo.- If we wanted to set it, we couldn't because it is used as:
-DSQLITE3_INCLUDE_DIR=${SQLITE_DIR}/include
-DSQLITE3_LIBRARY=${SQLITE_DIR}/lib/libsqlite3.so
But that won't actually resolve to the right locations in the Docker environment.
We'd need to use /usr/lib/x86_64-linux-gnu/libsqlite3.so instead of /usr/lib/libsqlite3.so.
- If we leave it unset, then line
compile_MET_all.sh:160sets it to LIB_DIR:
SQLITE_DIR=${LIB_DIR}
- But then
compile_MET_all.sh:403checks if its set:
if [[ -z "$SQLITE_DIR" ]]; then
But of course its set... its always set... by line 160. And because of that, I don't think compile_MET_all.sh would ever actually compile sqlite.
I just don't think this logic is doing what you want it to do. So let's take a look at this together next week and decide how we want the logic to work in METbaseimage, along with HPC's (hera, jet, and so on).
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
|
@JohnHalleyGotway Thank you for remembering to test this for the docker testing environment for MET and METplus and for finding these problems! I'm thinking that the circular dependency won't be a problem for most, if any, and that using the and modifying the following as well: to check for the setting of the two new variables? |
|
@jprestop as we discussed on 10/9/23, this sounds like a good approach. Remember to update line 160 to set a |
|
@JohnHalleyGotway I made the changes we discussed to compile_MET_all.sh. I tested on Jet by setting both which allowed me to retest with these changes. Unfortunately, I received a seemingly unrelated error with the installation of numpy (see below for details). Looking up the error: I came across this page, which indicated that numpy v1.26 is very new (3 weeks and 2 days ago (16 Sep 2023)) so no wheels are available. The same user says that once available, this problem should solve itself. So, in the METbaseimage Dockerfile, I added a specific version for numpy: Details of numpy error: |
|
The run was unsuccessful at first, due to xarray attempting to reinstall numpy version 1.26.0 after already having installed numpy version 1.25.2. I remembered a comment in this Discussion's answer, which said:
So, I modified the Dockerfile to remove @JohnHalleyGotway Do these changes need to be made in METbaseimage, or since we have a successful METbaseimage-3.0 release, do we not need to worry about these issues now? |
There was a problem hiding this comment.
I approve of these changes, but did suggest a minor change to comments in the script.
I'll note that we discussed in Slack that we expect the version-itis issues you uncovered testing in METbaseimage to resolve themselves in the next weeks/months as the python package versions become available in wheel. So we don't need to make any changes in METbaseimage now.
I reviewed the changes and note that you did add entries for SQLITE_INCLUDE_DIR and SQLITE_LIB_DIR to development.docker. Thanks for testing that out.
I did NOT test this script on seneca, namely because we don't actually use it there.
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
|
I just wanted to add a note that the failure in the Check for Differences is unrelated. I am going to proceed with the squash and merge. |
Expected Differences
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:
Installed on Jet and Hera.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Ensure all tests pass and review code changes.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes for now, the documentation is undergoing modifications]
Do these changes include sufficient testing updates? [N/A]
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 [At your convenience before working on modifications to the compilation script for Enhance MET to compile and link against the Atlas and ecKit libraries #2574].
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