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

[FIX] Default to using scipy v1.8.0+ API #646

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

matthewfeickert
Copy link
Contributor

@matthewfeickert matthewfeickert commented Jun 25, 2024

Types of changes

As most users will be installing the latest scipy release when they install POT, default to using the scipy v1.8.0+ API and catch the ModuleNotFoundError in the rare case in which scipy v1.7.3 or older is required. (scipy/scipy#14966 is in scipy v1.8.0+)

In all situations,import from the scipy.optimize.{_}linesearch API as the newest version of scipy that supports Python 3.7 is v1.7.3 which does not support from scipy.optimize import scalar_search_armijo, just as scipy v1.14.0+ does not.

Motivation and context / Related issue

Amends PR #642

How has this been tested (if it applies)

$ docker run --rm -ti python:3.8 /bin/bash
root@62c4a7db4ec9:/# python -m pip --quiet install uv
root@62c4a7db4ec9:/# uv venv
Using Python 3.8.19 interpreter at: usr/local/bin/python3
Creating virtualenv at: .venv
root@62c4a7db4ec9:/# . .venv/bin/activate
(.venv) root@62c4a7db4ec9:/# uv pip install 'scipy==1.8.0'
Resolved 2 packages in 327ms
Prepared 2 packages in 1.10s
Installed 2 packages in 27ms
 + numpy==1.24.4
 + scipy==1.8.0
(.venv) root@62c4a7db4ec9:/# python -c 'from scipy.optimize._linesearch import scalar_search_armijo; print(scalar_search_armijo)'
<function scalar_search_armijo at 0x7536b52294c0>
(.venv) root@62c4a7db4ec9:/#

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
    • Codecov is giving 0% coverage to the diff, but that is only because the ImportError raising branch isn't ever being taken as the version of scipy to raise it is old.
  • [N/A] I have added the PR and Issue fix to the RELEASES.md file.

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.65%. Comparing base (14bbebf) to head (56efb61).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   96.66%   96.65%   -0.02%     
==========================================
  Files          85       85              
  Lines       16936    16936              
==========================================
- Hits        16372    16370       -2     
- Misses        564      566       +2     

@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Jun 25, 2024

CI is passing, so this is now ready for review.

@matthewfeickert matthewfeickert changed the title [MNT] Default to using scipy v1.8.0+ API [FIX] Default to using scipy v1.8.0+ API Jun 25, 2024
@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Jun 25, 2024

This actually ends up being required to not break Python 3.7:

POT/setup.py

Line 87 in 14bbebf

python_requires=">=3.7",

$ docker run --rm -ti python:3.7 /bin/bash
root@1c521122a4c7:/# python -m venv venv && . venv/bin/activate
(venv) root@1c521122a4c7:/# python -m pip --quiet install --upgrade pip setuptools wheel
(venv) root@1c521122a4c7:/# python -m pip install --upgrade scipy
Collecting scipy
  Downloading scipy-1.7.3-cp37-cp37m-manylinux_2_12_x86_64.manylinux2010_x86_64.whl.metadata (2.2 kB)
Collecting numpy<1.23.0,>=1.16.5 (from scipy)
  Downloading numpy-1.21.6-cp37-cp37m-manylinux_2_12_x86_64.manylinux2010_x86_64.whl.metadata (2.1 kB)
Downloading scipy-1.7.3-cp37-cp37m-manylinux_2_12_x86_64.manylinux2010_x86_64.whl (38.1 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 38.1/38.1 MB 45.2 MB/s eta 0:00:00
Downloading numpy-1.21.6-cp37-cp37m-manylinux_2_12_x86_64.manylinux2010_x86_64.whl (15.7 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 15.7/15.7 MB 53.4 MB/s eta 0:00:00
Installing collected packages: numpy, scipy
Successfully installed numpy-1.21.6 scipy-1.7.3
(venv) root@1c521122a4c7:/# apt update && apt install -y vi
(venv) root@1c521122a4c7:/# cat fail.py 
# https://github.com/PythonOT/POT/blob/14bbebf1b6c5a3acef91e07247388945fd7a9014/ot/optim.py#L19-L22
try:
    from scipy.optimize import scalar_search_armijo
except ImportError:
    from scipy.optimize._linesearch import scalar_search_armijo
print(scalar_search_armijo)
(venv) root@1c521122a4c7:/# python fail.py 
Traceback (most recent call last):
  File "fail.py", line 3, in <module>
    from scipy.optimize import scalar_search_armijo
ImportError: cannot import name 'scalar_search_armijo' from 'scipy.optimize' (/venv/lib/python3.7/site-packages/scipy/optimize/__init__.py)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "fail.py", line 5, in <module>
    from scipy.optimize._linesearch import scalar_search_armijo
ModuleNotFoundError: No module named 'scipy.optimize._linesearch'
(venv) root@1c521122a4c7:/# cat pass.py 
# https://github.com/PythonOT/POT/pull/646
try:
    from scipy.optimize._linesearch import scalar_search_armijo
except ModuleNotFoundError:
    from scipy.optimize.linesearch import scalar_search_armijo
print(scalar_search_armijo)
(venv) root@1c521122a4c7:/# python pass.py 
<function scalar_search_armijo at 0x7dd6dc6767a0>
(venv) root@1c521122a4c7:/# 

fail.py.txt

pass.py.txt

* As most users will be installing the latest scipy release when they install
  POT, default to using the scipy v1.8.0+ API and catch the ModuleNotFoundError in
  the rare case in which scipy v1.7.3 or older is required, such as Python 3.7.
* Always import from the scipy.optimize.{_}linesearch API as the newest version
  of scipy that supports Python 3.7 is v1.7.3 which does not support
  'from scipy.optimize import scalar_search_armijo', just as scipy v1.14.0+ does
  not.
from scipy.optimize._linesearch import scalar_search_armijo
except ModuleNotFoundError:
# scipy<1.8.0
Copy link
Contributor Author

@matthewfeickert matthewfeickert Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is added only to make it easier to know when this try except block can be removed when the scipy install_requires lower bound gets raised eventually.

@rflamary rflamary merged commit 5c9c70a into PythonOT:master Jun 26, 2024
17 of 18 checks passed
@matthewfeickert matthewfeickert deleted the mnt/default-to-modern-api branch June 26, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants