Skip to content
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

Bugfix: Fix the MET vx_pointdata_python library to handle MET_PYTHON_EXE for python embedding of point observations #2428

Closed
22 tasks
DanielAdriaansen opened this issue Jan 31, 2023 · 7 comments · Fixed by #2443, #2462 or #2467
Assignees
Labels
MET: Python Embedding priority: high High Priority reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: NCAR/RAL NCAR Research Applications Laboratory type: bug Fix something that is not working

Comments

@DanielAdriaansen
Copy link
Contributor

DanielAdriaansen commented Jan 31, 2023

Describe the Problem

When a user runs a point data tool plot_point_obs, point_stat, etc. with the environment variable MET_PYTHON_EXE set, MET does not override the default Python with the value from MET_PYTHON_EXE.

Expected Behavior

When a user sets MET_PYTHON_EXE, the MET tools should override the default Python executable and use the users Python executable. The example below under "To Reproduce" reports this:

DEBUG 3: Initializing python: /usr/local/met-python3/bin/python3.8

but I would expect it to say this:

DEBUG 3: Initializing python: /home/met_test/.conda/envs/metplus_base.v5/bin/python3

Environment

Testing on seneca on Linux using the MET develop nightly build which reports the Python executable being used.

To Reproduce

On seneca:

export MET_PYTHON_EXE=/home/met_test/.conda/envs/metplus_base.v5/bin/python3

/d1/projects/MET/MET_regression/develop/NB20230130/MET-develop/bin/plot_point_obs "PYTHON_NUMPY=/d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_point.py /d1/projects/MET/MET_releases/MET-11.0.0/data/sample_obs/ascii/sample_ascii_obs.txt" test.ps -v 3

Relevant Deadlines

List relevant project deadlines here or state NONE.

Funding Source

2792542

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Select component(s)
  • Select priority
  • Select requestor(s)

Projects and Milestone

  • Select Organization level Project for support of the current coordinated release
  • Select Repository level Project for development toward the next official release or add alert: NEED PROJECT ASSIGNMENT label
  • Select Milestone as the next bugfix version

Define Related Issue(s)

Consider the impact to the other METplus components.

Bugfix Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of main_<Version>.
    Branch name: bugfix_<Issue Number>_main_<Version>_<Description>
  • Fix the bug and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Push local changes to GitHub.
  • Submit a pull request to merge into main_<Version>.
    Pull request: bugfix <Issue Number> main_<Version> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Development issue
    Select: Organization level software support Project for the current coordinated release
    Select: Milestone as the next bugfix version
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Complete the steps above to fix the bug on the develop branch.
    Branch name: bugfix_<Issue Number>_develop_<Description>
    Pull request: bugfix <Issue Number> develop <Description>
    Select: Reviewer(s) and Development issue
    Select: Repository level development cycle Project for the next official release
    Select: Milestone as the next official version
  • Close this issue.
@DanielAdriaansen DanielAdriaansen added type: bug Fix something that is not working requestor: NCAR/RAL NCAR Research Applications Laboratory alert: NEED MORE DEFINITION Not yet actionable, additional definition required alert: NEED ACCOUNT KEY Need to assign an account key to this issue alert: NEED CYCLE ASSIGNMENT Need to assign to a release development cycle MET: Python Embedding priority: high High Priority labels Jan 31, 2023
@DanielAdriaansen DanielAdriaansen added this to the MET 11.0.1 (bugfix) milestone Jan 31, 2023
@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Jan 31, 2023

Run this script python_commands.sh.txt on seneca to demonstrate. And then diff the resulting log files.

MET_PYTHON_EXE works as expected for plot_data_plane:

diff plot_data_plane_UNSET.log plot_data_plane_SET.log 
4c4,6
< DEBUG 3: Running user's python script (/d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_numpy).
---
> DEBUG 3: Calling /usr/local/met-python3/bin/python3 to run user's python script (/d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_numpy).
> DEBUG 4: Writing temporary Python dataplane file:
> DEBUG 4: 	/usr/local/met-python3/bin/python3 /d1/projects/MET/MET_regression/develop/NB20230130/MET-develop/share/met/wrappers/write_tmp_dataplane.py /tmp/tmp_met_nc_2660_0 /d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_numpy /d1/projects/MET/MET_releases/MET-11.0.0/data/python/fcst.txt FCST
5a8
> DEBUG 4: Reading temporary Python dataplane file: /tmp/tmp_met_nc_2660_0
31c34
< DEBUG 1: Finish plot_data_plane by johnhg(6088) at 2023-01-31 18:04:39Z
---
> DEBUG 1: Finish plot_data_plane by johnhg(6088) at 2023-01-31 18:04:41Z

But it does NOT work for plot_point_obs:

diff plot_point_obs_UNSET.log plot_point_obs_SET.log 
21c21
< DEBUG 1: Finish plot_point_obs by johnhg(6088) at 2023-01-31 18:04:39Z
---
> DEBUG 1: Finish plot_point_obs by johnhg(6088) at 2023-01-31 18:04:40Z

There are no real differences in the plot_point_obs log files, when there should be!

Ideally, fix this by migrating MET_PYTHON_EXE logic from ascii2nc into library code some that can be accessed by all the point observation tools, such as point_stat, ensemble_stat, plot_point_obs, point2grid, and so on.

While fixing this, recommend updating log messages as follows:

  • With MET_PYTHON_EXE set:
DEBUG 3: Initializing MET compile time python instance: /usr/local/met-python3/bin/python3.8
...
DEBUG 3: Running user-specified python instance (MET_PYTHON_EXE=/usr/local/met-python3/bin/python3) to run user's python script (/d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_numpy).
  • Without MET_PYTHON_EXE set:
DEBUG 3: Initializing MET compile time python instance: /usr/local/met-python3/bin/python3.8
...
DEBUG 3: Running MET compile time python instance (usr/local/met-python3/bin/python3) to run user's python script (/d1/projects/MET/MET_releases/MET-11.0.0/scripts/python/read_ascii_numpy).

@TaraJensen TaraJensen added reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project and removed alert: NEED ACCOUNT KEY Need to assign an account key to this issue labels Feb 3, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 14, 2023
hsoh-u pushed a commit that referenced this issue Feb 15, 2023
hsoh-u pushed a commit that referenced this issue Feb 15, 2023
@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Feb 20, 2023

@hsoh-u and @DanielAdriaansen, merging PR #2443 introduced a problem in main_v11.0. The automated testing for that PR actually failed when it introduced differences in the output.

  1. Adding a new output file is fine:
ERROR: folder /data/output/met_test_truth missing 1 files
    python/pb2nc_TMP_user_python.nc
  1. But introducing diffs for an existing unit test is not:
COMPARING python/precip24_2010010112.ps
file1: /data/output/met_test_truth/python/precip24_2010010112.ps
file2: /data/output/met_test_output/python/precip24_2010010112.ps
ERROR: diff error:

The problem is basically that using python embedding to serve up obs identified by their GRIB code via python embedding no longer works. Here's a command to demonstrate:

bin/plot_point_obs \
'PYTHON_NUMPY=share/met/python/read_ascii_point.py data/sample_obs/ascii/precip24_2010010112.ascii' \
precip24_2010010112.ps \
-config internal/test_unit/config/PlotPointObsConfig \
-title "Precip24 from python embedding" -gc 61 \
-v 3

This should plot 822 locations, but now plots 0:

DEBUG 3: For point data group 1, plotting 0 locations for 0 observations.

I tried to restore the "GRIB" logic in met_point_obs.py, but that didn't fix it:

     if 'csv' == input_type:
         csv_point_data = csv_point_obs(point_data)
+        if csv_point_data.is_grib_code():
+            csv_point_data.use_var_id = False
         csv_point_data.check_csv_point_data(check_all_records)
         tmp_point_data = csv_point_data.get_point_data()
     else:

When I run this through the debugger, I see that use_var_id is set to True when it should be set to False.

@hsoh-u can you please fix this ASAP on Tuesday morning. We need to include this in the MET-11.0.1-rc1 release if at all possible.

hsoh-u pushed a commit that referenced this issue Feb 21, 2023
hsoh-u pushed a commit that referenced this issue Feb 21, 2023
hsoh-u pushed a commit that referenced this issue Feb 21, 2023
@JohnHalleyGotway JohnHalleyGotway changed the title Bugfix: The MET vx_pointdata_python libcode does not respect MET_PYTHON_EXE Bugfix: Fix the MET vx_pointdata_python library to handle MET_PYTHON_EXE for python embedding of point observations Feb 21, 2023
@hsoh-u
Copy link
Collaborator

hsoh-u commented Feb 22, 2023

If user defined MET_PYTHON_EXE is the same as the MET_PYTHON_BIN_EXE (python at compile time), is it OK not to write/read the temporay NetCDF file?

@DanielAdriaansen
Copy link
Contributor Author

If user defined MET_PYTHON_EXE is the same as the MET_PYTHON_BIN_EXE (python at compile time), is it OK not to write/read the temporay NetCDF file?

I don't know the technical details from a MET side of things, but if MET_PYTHON_EXE==MET_PYTHON_BIN_EXE, then I think we can ignore MET_PYTHON_EXE and operate the way we would if it was unset, which means no writing/reading of the temporary NetCDF file.

But maybe @JohnHalleyGotway or @georgemccabe have a different thinking?

@hsoh-u
Copy link
Collaborator

hsoh-u commented Feb 22, 2023

execution times from unit tests (the first: without MET_PYTHON_EXE and the second: with MET_PYTHON_EXE)

TEST: python_point2grid_pb2nc_TMP                       - pass -   2.128 sec
TEST: python_point2grid_pb2nc_TMP_user_python           - pass -   3.401 sec

@georgemccabe
Copy link
Collaborator

I recall an issue a while back that was resolved by setting MET_PYTHON_EXE explicitly to the same version of Python, but I don't know if it is still an issue.

From what I remember, there was a conflict between the HDF5 library used to install MET and the HDF5 library that is obtained when installing the h5py python package. It failed when MET_PYTHON_EXE was unset but succeeded when MET_PYTHON_EXE was set. It looks like the /usr/local/met-python3 version does not have the h5py package installed, so we would have to build MET with another Python to test.

I may not be remembering the details correctly and this may have been resolved in changes to the Python Embedding logic long ago. I lean towards writing the temp NetCDF file if MET_PYTHON_EXE is set by the user regardless of the value. Yes, it takes longer to run, but checking and forcing behavior based on this value may prevent a viable work around to potential issues.

hsoh-u pushed a commit that referenced this issue Feb 22, 2023
hsoh-u pushed a commit that referenced this issue Feb 22, 2023
hsoh-u pushed a commit that referenced this issue Feb 22, 2023
hsoh-u pushed a commit that referenced this issue Feb 22, 2023
hsoh-u pushed a commit that referenced this issue Feb 22, 2023
hsoh-u pushed a commit that referenced this issue Feb 22, 2023
@hsoh-u hsoh-u linked a pull request Feb 23, 2023 that will close this issue
15 tasks
hsoh-u pushed a commit that referenced this issue Feb 23, 2023
hsoh-u pushed a commit that referenced this issue Feb 23, 2023
hsoh-u pushed a commit that referenced this issue Feb 24, 2023
@hsoh-u hsoh-u linked a pull request Feb 24, 2023 that will close this issue
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MET: Python Embedding priority: high High Priority reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: NCAR/RAL NCAR Research Applications Laboratory type: bug Fix something that is not working
Projects
Status: Done
6 participants