Bugfix #3219 develop python #3222

JohnHalleyGotway · 2025-08-12T23:41:22Z

Expected Differences

These are the same basic changes to fix #3219 that described in PR #3220 for the main_v12.1 branch. However, these changes for develop are extended in the following ways:

Change the GlobalPython::set_args() signature to replace the Wchar_Argv argument with StringArray to consolidate more common code. Also update the logic of set_args to automatically add the directory that contains the Python script to the system path to consolidate more code.
Simplify data2d_python.cc to pass the args as a StringArray object rather than first parsing them into argc and argv. This avoid allocating/deallocating memory.
Use consistent function names throughout where met_python_...() indicates that we're running MET's compile-time Python instance (previously called straight_) and user_python_...() indicates that we're running a user-specified version of Python. Also write consistent log messages throughout to indicate which type of Python embedding is being performed.
In python3_util.h, I renamed wrappers_dir to pyembed_dir and replaced those references throughout. The wrappers_dir doesn't actually exist but those scripts were moved to the pyembed_dir location a while ago. I'm not sure that it's necessary to include them in the Python system path, but figured that including it is safer than not.
Generally, apply more consistent formatting of whitespace. I didn't make it the same across all files, but generally made it consistent within each one.

Opportunities for further consolidation and refinement of this code likely still exist, but I see this as incremental progress.

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:

Ran unit test commands locally with Python 3.10 to confirm they run well.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Confirm the GHA workflow runs for Python 3.12 and flags no meaningful differences.
Find this code compiled with Python 3.10 in seneca /d1/projects/MET/MET_pull_requests/met-12.2.0/rc1/MET-bugfix_3219_develop_python. The log from running all the unit tests can be found in internal/test_unit/run_unit_test_py310.log. And the log from diffing this output with that of the nightly build can be found in internal/test_unit/run_comp_dir_py310.log.

No differences found in any files
Finished comparing directories

Review code changes.

Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
None needed. The discussion of Python versions in Appendix F Python Embedding do not REQUIRE any change.
Do these changes include sufficient testing updates? [No]
Will this PR result in changes to the MET test suite? [No]

If yes, describe the new output and/or changes to the existing output:
Will this PR result in changes to existing METplus Use Cases? [No]

If yes, create a new Update Truth METplus issue to describe them.
Do these changes introduce new SonarQube findings? [No]

If yes, please describe:
The SonarQube GHA fails because one existing code smell about "Cognitive Complexity" is flagged as New Code. But the overall number of code smells are reduced by 12 (from 15,241 in develop to 15,229 for this PR).
Please complete this pull request review by [Fri Aug 15, 2025].

Pull Request Checklist

See the METplus Workflow for details.

Review the source issue metadata (required labels, projects, and milestone).
Complete the PR definition above.
Ensure the PR title matches the feature or bugfix branch name.
Define the PR metadata, as permissions allow.
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
After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
Close the linked issue and delete your feature or bugfix branch from GitHub.

…ath prior to calling Py_InitializeFromConfig() and then restore the Python path afterwards. For Python 3.10, the call to Py_InitializeFromConfig() resets the Python path while in Python 3.12 it does not.

…tion names, and log messages.

…orrection. I'm not positive we need it in the python path, but figure its safer to correct its location than it is to remove it.

…P.initialize() already adds it.

…on_pointdata.cc

…bedding.

hsoh-u · 2025-08-13T22:48:01Z

The two files are different. Is this expected?

tc_diag/sal092022_gfso_doper_2022092400_diag.nc
- variables: SHR_MAG, SHR_HDG, 850TANG, 850VORT, and 850TANG
  - all missing values from PR
  - some missing values from Nightly build
tc_diag/sal092022_gfso_doper_2022092400_diag.dat

Compared directories:

/d1/personal/hsoh/git/pull_request/MET_bugfix_3219_develop_python/unit_test_output/tc_diag directory
/d1/projects/MET/MET_regression/develop/NB20250813/MET-develop/test_output/tc_diag.
The python version in the tc_diag:

$ ldd /d1/personal/hsoh/git/pull_request/MET_bugfix_3219_develop_python/bin/tc_diag | grep python
        libpython3.10.so.1.0 => /nrit/ral/met-python3.10/lib/libpython3.10.so.1.0 (0x00007f1460ce7000)

JohnHalleyGotway · 2025-08-14T16:04:37Z

@hsoh-u thanks for looking closely and comparing the outputs. Here's a screenshot of the differences you noted:

Basically, the issue is that these derived diagnostics are not being computed, including shear and vorticity. I do recall this detail coming up during development for the TC-Diag tool.

I believe this Python import statement is the issue.

from scipy import interpolate

The TC-Diagnostics library requires scipy to compute certain diagnostics, including these. But we made that a "soft requirement". If scipy is available, it'll be used to compute real values for those. If not, you'll just get bad data.

But I see that this is NOT MENTIONED in the MET User's Guide. I'll add a note about to the TC-Diag chapter and the Python appendix, and include that on this PR.

Can you please test on your end with the following:

TEST THE USER PYTHON EMBEDDING:

Launch the Python interpreter manually and confirm that import scipy does NOT work. But do NOT actually install scipy yet.
Set export MET_PYTHON_EXE=/nrit/ral/met-python3.10/bin/python3.10 and then rerun the TC-Diag unit test cd internal/test_unit; python/unit.py xml/unit_tc_diag.xml. And then check the .dat output file to see if those diagnostics have been computed.

TEST THE MET PYTHON EMBEDDING:

Unset that env var unset MET_PYTHON_EXE.
Install scipy for the version of Python you're running (maybe with pip or in mamba?).
Re-run the TC-Diag unit test python/unit.py xml/unit_tc_diag.xml and check the .dat output file to see if those diagnostics have been computed.

Thanks!

hsoh-u · 2025-08-14T17:06:12Z

@hsoh-u thanks for looking closely and comparing the outputs. Here's a screenshot of the differences you noted:

Basically, the issue is that these derived diagnostics are not being computed, including shear and vorticity. I do recall this detail coming up during development for the TC-Diag tool.

I believe this Python import statement is the issue.
from scipy import interpolate
The TC-Diagnostics library requires scipy to compute certain diagnostics, including these. But we made that a "soft requirement". If scipy is available, it'll be used to compute real values for those. If not, you'll just get bad data.

But I see that this is NOT MENTIONED in the MET User's Guide. I'll add a note about to the TC-Diag chapter and the Python appendix, and include that on this PR.

Can you please test on your end with the following:

TEST THE USER PYTHON EMBEDDING:

Launch the Python interpreter manually and confirm that import scipy does NOT work. But do NOT actually install scipy yet.

Set export MET_PYTHON_EXE=/nrit/ral/met-python3.10/bin/python3.10 and then rerun the TC-Diag unit test cd internal/test_unit; python/unit.py xml/unit_tc_diag.xml. And then check the .dat output file to see if those diagnostics have been computed.

TEST THE MET PYTHON EMBEDDING:

Unset that env var unset MET_PYTHON_EXE.

Install scipy for the version of Python you're running (maybe with pip or in mamba?).

Re-run the TC-Diag unit test python/unit.py xml/unit_tc_diag.xml and check the .dat output file to see if those diagnostics have been computed.

Thanks!

The difference was disappeared after installing the scipy package locally.

This was before installing the scipy package.
I found a warning message during running tc_diag unit test.

/d1/personal/hsoh/git/pull_request/MET_bugfix_3219_develop_python/share/met/python/tc_diag/diag_lib/cylindrical_grid.py:11: UserWarning: Scipy not installed, some functionality in cylindrical_grid module may not be available.
  warnings.warn("Scipy not installed, some functionality in cylindrical_grid module may not be available.")

(base) hsoh@seneca:~$ which python3
/var/autofs/mnt/linux-amd64/debian/bookworm/local/anaconda3-20231025/bin/python3

(base) hsoh@seneca:~$ python3 --version
Python 3.10.13
(base) hsoh@seneca:~$ python3 -c "from scipy import interpolate"
(base) hsoh@seneca:~$ echo $MET_PYTHON_BIN_EXE
/nrit/ral/met-python3.10/bin/python3.10
(base) hsoh@seneca:~$ $MET_PYTHON_BIN_EXE --version
Python 3.10.13
(base) hsoh@seneca:~$ $MET_PYTHON_BIN_EXE -c "from scipy import interpolate"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'scipy'

hsoh-u

The difference was caused by missing scipy python package.
Thank you for refactoring codes and better naming

… computing diagnostics.

JohnHalleyGotway · 2025-08-14T17:33:52Z

Thanks for the review @hsoh-u. FYI, I just added this note to the TC-Diag chapter of the MET User's Guide:

I'll just go ahead and push through this PR based on your previous review because this is a doc-only update.

JohnHalleyGotway added 9 commits August 12, 2025 17:55

Per #3219, add GlobalPython:set_args() function to store the Python p…

881fb62

…ath prior to calling Py_InitializeFromConfig() and then restore the Python path afterwards. For Python 3.10, the call to Py_InitializeFromConfig() resets the Python path while in Python 3.12 it does not.

Per #3219, update logic to eliminate duplicated log messages.

6b33508

Per #3219, working toward consistency in python embedding logic, func…

2f87445

…tion names, and log messages.

Per #3219, rename wrappers_dir as pyembed_dir and make its location c…

e791221

…orrection. I'm not positive we need it in the python path, but figure its safer to correct its location than it is to remove it.

Per #3219, don't need ascii2nc to add pyembed_dir to the path since G…

00b0bb4

…P.initialize() already adds it.

Per #3219, make logic, function names, and logging consistent in pyth…

b64fe73

…on_pointdata.cc

Per #3219, more consistency in function names and log messages.

c619e8e

work in progress

b98746b

Per #3219, fix met_python_point_data() Python import command.

2c00e86

JohnHalleyGotway added this to METplus-6.2 Development Aug 12, 2025

github-project-automation bot moved this to 🩺 Needs Triage in METplus-6.2 Development Aug 12, 2025

JohnHalleyGotway moved this from 🩺 Needs Triage to 🔎 In review in METplus-6.2 Development Aug 12, 2025

JohnHalleyGotway added this to the MET-12.2.0 milestone Aug 12, 2025

JohnHalleyGotway linked an issue Aug 12, 2025 that may be closed by this pull request

Bugfix: Python embedding does not work for MET version 12.1.0 with Python version 3.10 #3219

Closed

23 tasks

Per #3219, fix problem with preparing arguments for TC-Diag Python em…

9003038

…bedding.

JohnHalleyGotway requested review from georgemccabe and hsoh-u August 13, 2025 14:49

JohnHalleyGotway marked this pull request as ready for review August 13, 2025 14:57

JohnHalleyGotway removed the request for review from georgemccabe August 13, 2025 17:49

hsoh-u previously approved these changes Aug 14, 2025

View reviewed changes

Per #3219, add note to the TC-Diag chapter about the role of SciPy in…

b0fb725

… computing diagnostics.

JohnHalleyGotway dismissed hsoh-u’s stale review via b0fb725 August 14, 2025 17:31

JohnHalleyGotway merged commit 506381b into develop Aug 14, 2025
6 of 7 checks passed

github-project-automation bot moved this from 🔎 In review to 🏁 Done in METplus-6.2 Development Aug 14, 2025

georgemccabe mentioned this pull request Aug 28, 2025

Use MET bugfix version to fix python error when using python 3.10 conda-forge/metplus-feedstock#3

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bugfix #3219 develop python #3222

Bugfix #3219 develop python #3222

Uh oh!

JohnHalleyGotway commented Aug 12, 2025 •

edited

Loading

Uh oh!

hsoh-u commented Aug 13, 2025

Uh oh!

JohnHalleyGotway commented Aug 14, 2025

Uh oh!

hsoh-u commented Aug 14, 2025

Uh oh!

hsoh-u left a comment

Uh oh!

JohnHalleyGotway commented Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

Bugfix #3219 develop python #3222

Bugfix #3219 develop python #3222

Uh oh!

Conversation

JohnHalleyGotway commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Expected Differences

Pull Request Testing

Pull Request Checklist

Uh oh!

hsoh-u commented Aug 13, 2025

Uh oh!

JohnHalleyGotway commented Aug 14, 2025

Uh oh!

hsoh-u commented Aug 14, 2025

Uh oh!

hsoh-u left a comment

Choose a reason for hiding this comment

Uh oh!

JohnHalleyGotway commented Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

JohnHalleyGotway commented Aug 12, 2025 •

edited

Loading