Allow numpy to be import under pypy #23461

eagleoflqj · 2022-05-04T02:22:01Z

References to other Issues or PRs

Please review commit by commit (e.g. checkout the first commit, reproduce the result I get, then checkout the next commit). I'll amend and force-push when needed.

The 1st commit removes the import restriction

My local environment is Ubuntu 22.04 x64, pypy3 from apt.
I can't install scipy (build fail), so I test with numpy only.

$ pypy3 -m pytest sympy/utilities/tests/test_lambdify.py
...
FAILED sympy/utilities/tests/test_lambdify.py::test_integral - NameError: name 'scipy' is not defined
FAILED sympy/utilities/tests/test_lambdify.py::test_double_integral - NameError: name 'scipy' is not defined

It is strange since I have no scipy locally, why it tries to import scipy? If I run the test_integral only, it gives another error:

$ pypy3 -m pytest sympy/utilities/tests/test_lambdify.py::test_integral
...
FAILED sympy/utilities/tests/test_lambdify.py::test_integral - NameError: name 'Integral' is not defined

So the previous error is dependent with tests ran before.

The 2nd commit fixes the scipy problem

A test runs before test_integral tries to import scipy. At line 149 it executes import numpy first, which succeed and updates namespace, which is the global dict SCIPY.
After that, test_integral tries to import scipy. As the namespace is modified, at line 132 the condition becomes True and the function successfully returns, giving a false conclusion that scipy is available.
So this commit re-arranges the order of imports. If scipy is not available, SCIPY isn't modified anymore, and tests run consistently.

sympy/sympy/utilities/lambdify.py

Lines 132 to 150 in ed4e06e

    
           if namespace != namespace_default: 
        
               # The namespace was already generated, don't do it again if not forced. 
        
               if reload: 
        
                   namespace.clear() 
        
                   namespace.update(namespace_default) 
        
               else: 
        
                   return 
        
           for import_command in import_commands: 
        
               if import_command.startswith('import_module'): 
        
                   module = eval(import_command) 
        
                   if module is not None: 
        
                       namespace.update(module.__dict__) 
        
                       continue 
        
               else: 
        
                   try: 
        
                       exec(import_command, {}, namespace) 
        
                       continue

Now testing test_lambdify.py will also lead to the undefined Integral exception.

The 3rd commit skips the 2 failing tests when numpy is installed but scipy isn't

The 4th commit test optional dependencies for pypy

gmpy2, symengine, llvmlite and numba cannot be built. Skip them.

The 5th commit fix the test error in the previous PR

Explanation can be found at https://stackoverflow.com/a/7396043.

Release Notes

utilities
- It is now possible to use lambdify with NumPy while running under PyPy.

Pypy has evolved greatly, it might be a good idea to remove the import ban.

sympy-bot · 2022-05-04T02:22:11Z

✅

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

utilities
- It is now possible to use lambdify with NumPy while running under PyPy. (#23461 by @eagleoflqj and @extraymond)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

Fixes #14658
Revive #14664

Please review commit by commit (e.g. checkout the first commit, reproduce the result I get, then checkout the next commit). I'll amend and force-push when needed.
## The 1st commit removes the import restriction
My local environment is Ubuntu 22.04 x64, pypy3 from apt.
I can't install scipy (build fail), so I test with numpy only.
```sh
$ pypy3 -m pytest sympy/utilities/tests/test_lambdify.py
...
FAILED sympy/utilities/tests/test_lambdify.py::test_integral - NameError: name 'scipy' is not defined
FAILED sympy/utilities/tests/test_lambdify.py::test_double_integral - NameError: name 'scipy' is not defined
```
It is strange since I have no scipy locally, why it tries to import scipy? If I run the `test_integral` only, it gives another error:
```sh
$ pypy3 -m pytest sympy/utilities/tests/test_lambdify.py::test_integral
...
FAILED sympy/utilities/tests/test_lambdify.py::test_integral - NameError: name 'Integral' is not defined
```
So the previous error is dependent with tests ran before.
## The 2nd commit fixes the scipy problem
A test runs before `test_integral` tries to import scipy. At line 149 it executes `import numpy` first, which succeed and updates `namespace`, which is the global dict `SCIPY`.
After that, `test_integral` tries to import scipy. As the `namespace` is modified, at line 132 the condition becomes `True` and the function successfully returns, giving a false conclusion that scipy is available.
So this commit re-arranges the order of imports. If scipy is not available, `SCIPY` isn't modified anymore, and tests run consistently.
https://github.com/sympy/sympy/blob/ed4e06eff860e001fd7c4da7cf17fd8c1cac8c6a/sympy/utilities/lambdify.py#L132-L150
Now testing `test_lambdify.py` will also lead to the undefined `Integral` exception.
## The 3rd commit skips the 2 failing tests when numpy is installed but scipy isn't
## The 4th commit test optional dependencies for pypy
gmpy2, symengine, llvmlite and numba cannot be built. Skip them.
## The 5th commit fix the test error in the previous [PR](https://github.com/sympy/sympy/pull/14664#issuecomment-591618494)
Explanation can be found at https://stackoverflow.com/a/7396043.

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* utilities
  * It is now possible to use `lambdify` with NumPy while running under PyPy.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

eagleoflqj · 2022-05-04T04:22:51Z

It took more than 20 min to build packages, and gmpy2 is incompatible with pypy. Skip that.

eagleoflqj · 2022-05-04T22:16:28Z

Symengine should also be skipped.

github-actions · 2022-05-05T05:36:54Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [77f1d79c]       [96ecc0c5]
     <sympy-1.10.1^0>                 
+        92.2±5ms        177±0.8ms     1.92  sum.TimeSum.time_doit

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

.mailmap

asmeurer · 2022-05-05T18:12:38Z

The changes here all make sense to me. For the CI, I don't know if it's necessary to have a separate optional dependencies build just for pypy. We can install the optional dependencies that work with pypy in the normal pypy build. The point of the optional dependencies build is to make sure we test everything without any optional dependencies installed in the normal tests, but I don't think that's important to do for pypy specifically.

eagleoflqj · 2022-05-06T04:50:04Z

I make a separate CI job for this because

Pypy tests are much slower than their cpython counterpart. We split test1 and test2 even for cpython because running as a whole is too long.
We can compare optional-dependencies-pypy with optional-dependencies, so that we know what dependencies are disabled for pypy, and try adding a package back in the future if it's supported by pypy.

Let test cases covered by test1+test2 be set A, test cases covered by test_optional_dependencies.py be set B. I don't know the relationship between set A and B. If B is a subset of A, then we may run A with optional dependencies. If not, we need this new CI job.

oscarbenjamin · 2022-05-07T11:45:01Z

I think it's reasonable to add this as an extra job.

asmeurer · 2022-05-10T18:04:59Z

Adding another job is fine if it makes more sense to do so. Splitting jobs isn't as big of a deal as it used to be because GitHub Actions doesn't seem to have the same sort of parallel jobs limit like Travis used to, although we should still avoid splitting more than necessary because it makes the checks harder to read and because each job has some overhead for setup time.

eagleoflqj · 2022-05-14T21:45:48Z

I'm going to merge it in order to fix the issue. Feel free to remove the job if it becomes a burden.

oscarbenjamin · 2022-05-14T22:02:02Z

sympy/utilities/lambdify.py

@@ -100,7 +100,7 @@
    "math": (MATH, MATH_DEFAULT, MATH_TRANSLATIONS, ("from math import *",)),
    "mpmath": (MPMATH, MPMATH_DEFAULT, MPMATH_TRANSLATIONS, ("from mpmath import *",)),
    "numpy": (NUMPY, NUMPY_DEFAULT, NUMPY_TRANSLATIONS, ("import numpy; from numpy import *; from numpy.linalg import *",)),
-    "scipy": (SCIPY, SCIPY_DEFAULT, SCIPY_TRANSLATIONS, ("import numpy; import scipy; from scipy import *; from scipy.special import *",)),
+    "scipy": (SCIPY, SCIPY_DEFAULT, SCIPY_TRANSLATIONS, ("import scipy; import numpy; from scipy import *; from scipy.special import *",)),


Why is this change needed?

See my description for the 2nd commit. If the environment has numpy but doesn't have scipy, test_lambdify.py produces strange behavior. It's the case for both cpython and pypy.

oscarbenjamin · 2022-05-14T22:05:47Z

.github/workflows/runtests.yml

+  # -------------------- Optional dependency tests for pypy ----------------- #
+
+  optional-dependencies-pypy:
+    needs: code-quality


Can this be added using matrix: rather than as a separate listing?

The only difference with the job above is that pip doesn't install quite as many things but we could guard that with a conditional.

I'll read the GitHub Actions doc to see how to achieve that.

eagleoflqj · 2022-05-15T01:16:48Z

@oscarbenjamin Required tests should be changed.

extraymond and others added 3 commits May 3, 2022 20:16

Allow numpy to be import under pypy

9084484

Pypy has evolved greatly, it might be a good idea to remove the import ban.

when trying scipy, import scipy first

31092d8

skip integral test for numpy without scipy

1cb49a9

eagleoflqj marked this pull request as draft May 4, 2022 02:22

eagleoflqj force-pushed the pypy_numpy branch 2 times, most recently from 14bb564 to 624866b Compare May 5, 2022 04:10

eagleoflqj marked this pull request as ready for review May 5, 2022 04:17

eagleoflqj requested review from oscarbenjamin and asmeurer May 5, 2022 13:12

oscarbenjamin reviewed May 5, 2022

View reviewed changes

.mailmap Show resolved Hide resolved

oscarbenjamin added the Alternate Python Related to an alternate Python implementation such as PyPy or Jython. label May 7, 2022

oscarbenjamin reviewed May 14, 2022

View reviewed changes

eagleoflqj added 3 commits May 14, 2022 19:49

test optional dependencies for pypy

3a104b1

fix file not closed bug for pypy

5e3997d

add extraymond to .mailmap

9018d2d

eagleoflqj force-pushed the pypy_numpy branch from 624866b to 9018d2d Compare May 14, 2022 23:51

oscarbenjamin merged commit a6e12bb into sympy:master May 15, 2022

eagleoflqj deleted the pypy_numpy branch May 15, 2022 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Allow numpy to be import under pypy #23461

Allow numpy to be import under pypy #23461

Uh oh!

eagleoflqj commented May 4, 2022 •

edited by oscarbenjamin

Loading

Uh oh!

sympy-bot commented May 4, 2022 •

edited

Loading

Uh oh!

eagleoflqj commented May 4, 2022

Uh oh!

eagleoflqj commented May 4, 2022

Uh oh!

github-actions bot commented May 5, 2022 •

edited

Loading

Uh oh!

Uh oh!

asmeurer commented May 5, 2022

Uh oh!

eagleoflqj commented May 6, 2022

Uh oh!

oscarbenjamin commented May 7, 2022

Uh oh!

asmeurer commented May 10, 2022

Uh oh!

eagleoflqj commented May 14, 2022

Uh oh!

oscarbenjamin May 14, 2022

Uh oh!

eagleoflqj May 14, 2022

Uh oh!

oscarbenjamin May 14, 2022

Uh oh!

eagleoflqj May 14, 2022

Uh oh!

eagleoflqj commented May 15, 2022

Uh oh!

Uh oh!

	if namespace != namespace_default:
	# The namespace was already generated, don't do it again if not forced.
	if reload:
	namespace.clear()
	namespace.update(namespace_default)
	else:
	return

	for import_command in import_commands:
	if import_command.startswith('import_module'):
	module = eval(import_command)

	if module is not None:
	namespace.update(module.__dict__)
	continue
	else:
	try:
	exec(import_command, {}, namespace)
	continue

Uh oh!

Allow numpy to be import under pypy #23461

Allow numpy to be import under pypy #23461

Uh oh!

Conversation

eagleoflqj commented May 4, 2022 • edited by oscarbenjamin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

The 1st commit removes the import restriction

The 2nd commit fixes the scipy problem

The 3rd commit skips the 2 failing tests when numpy is installed but scipy isn't

The 4th commit test optional dependencies for pypy

The 5th commit fix the test error in the previous PR

Release Notes

Uh oh!

sympy-bot commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eagleoflqj commented May 4, 2022

Uh oh!

eagleoflqj commented May 4, 2022

Uh oh!

github-actions bot commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

asmeurer commented May 5, 2022

Uh oh!

eagleoflqj commented May 6, 2022

Uh oh!

oscarbenjamin commented May 7, 2022

Uh oh!

asmeurer commented May 10, 2022

Uh oh!

eagleoflqj commented May 14, 2022

Uh oh!

oscarbenjamin May 14, 2022

Choose a reason for hiding this comment

Uh oh!

eagleoflqj May 14, 2022

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin May 14, 2022

Choose a reason for hiding this comment

Uh oh!

eagleoflqj May 14, 2022

Choose a reason for hiding this comment

Uh oh!

eagleoflqj commented May 15, 2022

Uh oh!

Uh oh!

eagleoflqj commented May 4, 2022 •

edited by oscarbenjamin

Loading

sympy-bot commented May 4, 2022 •

edited

Loading

github-actions bot commented May 5, 2022 •

edited

Loading