Bugfix 2428 python embedding with MET_PYTHON_EXE defined #2462
Conversation
…_util.cc & python3_util.h
hsoh-u
changed the title
data/wrappers/read_tmp_point_nc.py
Outdated
| import os | ||
| import sys | ||
|
|
||
| met_base_dir = os.getenv('MET_BASE',None) |
There was a problem hiding this comment.
I don't think these 3 lines are necessary since the path is appended using the relative path of the script below.
There was a problem hiding this comment.
MET_BASE was removed at read_tmp_point_nc.py and write_tmp_point_nc.py
data/wrappers/write_tmp_point_nc.py
Outdated
| import sys | ||
| import importlib.util | ||
|
|
||
| met_base_dir = os.getenv('MET_BASE',None) |
There was a problem hiding this comment.
I don't think these 3 lines are necessary since the path is appended using the relative path of the script below.
|
|
||
| def get_type(self, value): | ||
| return 'string' if isinstance('str') else type(value) | ||
| if not self.is_numpy_array(self.hdr_typ): |
There was a problem hiding this comment.
To clean up this logic, I suggest putting the is_numpy_array check inside the convert_to_numpy function. If it is already an numpy array, the function could just return the input. I believe the variables are passed by reference so the function could simply return if it is already a numpy array or modify the value if it is not instead of returning a value. It would be good to double check that this works.
def convert_to_numpy(self, value_list):
if self.is_numpy_array(value_list):
return
value_list = np.array(value_list)
Then replace:
if not self.is_numpy_array(self.hdr_typ):
self.hdr_typ = self.convert_to_numpy(self.hdr_typ)
with:
self.convert_to_numpy(self.hdr_typ)
There was a problem hiding this comment.
Thank you for the suggestion. I'm not sure it works. Is there a way to pass the reference?
Here are my tests:
def set_to_var(local_var):
local_var = ['111', '222']
print('local_var', local_var)
aaa = ['bb','cc']
set_to_var(aaa)
print(aaa)
result
('local_var', ['111', '222'])
['bb', 'cc']
The content of test.py:
class my_class(object):
def __init__(self):
self.my_var = [ '111', 'bbb']
def update_var(self, var_name):
print(' input at update_var:', var_name)
var_name = [' abc', 'def' ]
print(' updated by update_var:', var_name)
def do_job(self):
self.update_var(self.my_var)
print(' after calling update_var:', self.my_var)
test = my_class()
test.do_job()
python3 ./test.py
input at update_var: ['111', 'bbb']
updated by update_var: [' abc', 'def']
after calling update_var: ['111', 'bbb']
There was a problem hiding this comment.
You are right that it won't work. You are able to change the values of a list or dictionary in a function, but not the entire list itself. Thanks for checking.
def set_to_var(local_var):
local_var[0] = 'a'
print('local_var', local_var)
aaa = ['bb','cc']
set_to_var(aaa)
print(aaa)
Result:
local_var ['a', 'cc']
['a', 'cc']
| print("{p} User Command:\t".format(p=PROMPT) + repr(' '.join(sys.argv[2:]))) | ||
| print("{p} Temporary File:\t".format(p=PROMPT) + repr(sys.argv[1])) | ||
|
|
||
| tmp_filename = sys.argv[1] |
There was a problem hiding this comment.
A lot of this logic looks to be the same as other scripts in the data/wrappers directory. We should consider pulling out duplicate code into a function. This isn't required for this PR and could be done during a bigger refactor of the Python Embedding logic.
|
The test listed in the PR for #2428 does not work for me: |
The case is If I So the error is with the write/read of temporary netCDF file somewhere I think. |
Fixed and removed setting from MET_BASE (suggestion from @georgemccabe). |
|
I am still getting the same error: Can you double check the command I am running is correct after your fix @hsoh-u ? |
There was a problem hiding this comment.
I performed three tests on seneca:
TEST 1: MET_PYTHON_EXE unset
/d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/bin/plot_point_obs "PYTHON_NUMPY=/d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/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 4 | grep -v "Plotting location"
TEST 2: MET_PYTHON_EXE set to a user Python instance (different from compile-time Python instance)
export MET_PYTHON_EXE=/home/dadriaan/.conda/envs/icing/bin/python3
dadriaan@seneca:~$ /d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/bin/plot_point_obs "PYTHON_NUMPY=/d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/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 4 | grep -v "Plotting location"
TEST 3: MET_PYTHON_EXE set to the MET compile-time Python instance
export MET_PYTHON_EXE=/usr/local/met-python3/bin/python3.8
dadriaan@seneca:~$ /d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/bin/plot_point_obs "PYTHON_NUMPY=/d1/personal/hsoh/git/pull_request/MET_bugfix_2428_python_from_env_2/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 4 | grep -v "Plotting location"
All three behave as I expect:
TEST 1: Python embedding uses the Python instance from MET compile-time (NO writing/reading temporary file) and correctly reports it in the logging at verbosity = 4
TEST 2: Python embedding uses the user installed Python instance (writing/reading temporary file) and correctly reports it in the logging at verbosity = 4
TEST 3: Python embedding uses the Python instance from MET compile-time (writing/reading temporary file) and correctly reports it in the logging at verbosity = 4
In addition, I did not have to set MET_BASE to find met_point_obs.py, or set PYTHONPATH, which is extremely desireable.
I reviewed Appendix F and determined these changes don't require any documentation updates, however I made 1 commit to fix a typo. The documentation for Python embedding will be revisited soon in a separate piece of work.
Co-authored-by: johnhg <johnhg@ucar.edu> Co-authored-by: Lisa Goodrich <lisag@seneca.rap.ucar.edu> Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@seneca.rap.ucar.edu> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: Dave Albo <dave@seneca.rap.ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com> Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu> Co-authored-by: Jonathan Vigh <jvigh@ucar.edu> Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: davidalbo <dave@ucar.edu> Co-authored-by: Daniel Adriaansen <dadriaan@ucar.edu> fix 2238 link error (#2255) fix #2271 develop nbrctc (#2272) fix #2309 develop tcmpr (#2310) fix #2306 ascii2nc airnow hourly (#2314) fix_spread_md (#2335) fix #2389 develop flowchart (#2392) 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)
Expected Differences
Same as the bugfix for 11.0
If MET_PYTHON_EXE is defined, the met_point_data is saved as temporary NetCDF file and MET reads the NetCDF file by using compile time python. The log message shows if the temporary file was created.
Without the environment variable MET_PYTHON_EXE
With the environment variable MET_PYTHON_EXE
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
at seneca:
Test from #2428
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
Do these changes include sufficient testing updates? [Yes]
Unittest "python_point2grid_pb2nc_TMP_user_python" was added. It should produce the same output with python_point2grid_pb2nc_TMP (pb2nc_TMP.nc and pb2nc_TMP_user_python.nc).
If yes, describe the new output and/or changes to the existing output:
One more output python/pb2nc_TMP_user_python.nc
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s)
Select: Organization level software support Project or Repository level development cycle Project
Select: Milestone as the version that will include these changes