maint(sympy): Remove deprecated imports

[oscarbenjamin]

Fixes #19551.

These names were importable before sympy 1.6 by accident. They were
replaced with deprecated import objects to give a warning in
#19316

The warnings coming from these are now seen while building the docs and
are breaking the docs build in CI so this commit removes those names
meaning that they will have been giving a deprecation warning in sympy
1.6 and will be removed in sympy 1.7.

• core
  • Modules and names that were accidentally importable from sympy are no longer importable. In sympy 1.6 importing and using these names would give a deprecation warning. In sympy 1.7 these names are removed. As an example from sympy import add would need to be from sympy.core import add or import sympy.core.add as add.

[sympy-bot]

✅

Hi, I am the SymPy bot (v161). 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:

• core
  • Modules and names that were accidentally importable from sympy are no longer importable. In sympy 1.6 importing and using these names would give a deprecation warning. In sympy 1.7 these names are removed. As an example from sympy import add would need to be from sympy.core import add or import sympy.core.add as add. (#19554 by @oscarbenjamin)

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

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

Fixes #19551.

These names were importable before sympy 1.6 by accident. They were
replaced with deprecated import objects to give a warning in
https://github.com/sympy/sympy/pull/19316

The warnings coming from these are now seen while building the docs and
are breaking the docs build in CI so this commit removes those names
meaning that they will have been giving a deprecation warning in sympy
1.6 and will be removed in sympy 1.7.

<!-- BEGIN RELEASE NOTES -->
* core
    * Modules and names that were accidentally importable from sympy are no longer importable. In sympy 1.6 importing and using these names would give a deprecation warning. In sympy 1.7 these names are removed. As an example `from sympy import add` would need to be `from sympy.core import add` or `import sympy.core.add` as `add`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[sympy-bot]

🟠

Hi, I am the SymPy bot (v161). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits delete files:

• 47789b1:
  • sympy/deprecated/tests/test_deprecated_imports.py

If these files were added/deleted on purpose, you can ignore this message.

[oscarbenjamin]

It seems that the sphinx build still fails because it gets confused about e.g. the ask function which is defined in the sympy.assumptions.ask module i.e.:

In [1]: import sympy                                                                                                                                          

In [2]: sympy.assumptions.ask                                                                                                                                 
Out[2]: <function sympy.assumptions.ask.ask(proposition, assumptions=True, context=AssumptionsContext())>

In [3]: sympy.assumptions.ask.ask                                                                                                                             
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-8a32f6c8c56a> in <module>
----> 1 sympy.assumptions.ask.ask

AttributeError: 'function' object has no attribute 'ask'

In [4]: from sympy.assumptions.ask import ask 

The warnings are e.g.

/home/travis/build/sympy/sympy/sympy/plotting/plot.py:1411: RuntimeWarning: invalid value encountered in double_scalars
  cos_theta = dot_product / (vector_a_norm * vector_b_norm)
/home/travis/build/sympy/sympy/sympy/plotting/plot.py:1411: RuntimeWarning: invalid value encountered in double_scalars
  cos_theta = dot_product / (vector_a_norm * vector_b_norm)
WARNING: autodoc: failed to import function 'ask.ask' from module 'sympy.assumptions'; the following exception was raised:
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.8.0/lib/python3.8/site-packages/sphinx/util/inspect.py", line 324, in safe_getattr
    return getattr(obj, name, *defargs)
AttributeError: 'function' object has no attribute 'ask'

[oscarbenjamin]

This sphinx change might be related: sphinx-doc/sphinx#7463

[sylee957]

Should we rather consider pinning down the sphinx version and contacting sphinx team whether this is an issue?

[oscarbenjamin]

I have asked on the sphinx mailing list (no response yet):
https://groups.google.com/d/msg/sphinx-users/mx83xLsDjBs/Qc3jW-z7AQAJ

I think that removing the deprecated imports here is still something we should do in any case. Looking at this I realise that things like sympy.matrices.Matrix don't work as they should.

For now I agree that pinning the sphinx version can keep the CI going.

[sylee957]

It had not passed a month that the 1.6 was released and it is too early to remove the deprecation warning.

[oscarbenjamin]

It will be more than one month after the 1.6 release when 1.7 gets released. I didn't think there was a need for a deprecation in the first place.

[sylee957]

When I take a look at the test log, this doesn't seem like solving the sphinx errors.

[oscarbenjamin]

This PR solved most of the warnings that were originally failing the build. After that the build fails because of other things like detailed here:
https://groups.google.com/forum/#!msg/sphinx-users/mx83xLsDjBs/Qc3jW-z7AQAJ

Specifically the problem is having a function with the same name as a module that is accessible as an attribute of the module's parent so anywhere we have a top-level function/class name that matches a top-level package name e.g. sympy.simplify means the simplify package an an import context but getattr(sympy, 'simplify') returns the simplify function. Same goes for series. It can also happen at a deeper level e.g. sympy.assumptions.ask is on the one hand a module but also a function (called ask) defined in that module.

It's probably a mistake to make a module have the exact same name as the function defined in the module or to have a submodule with the same name as its containing package as it will lead to these kinds of ambiguities. I've seen that mypy gets confused about what simplify is:

from sympy.simplify.simplify import simplify

[codecov]

Codecov Report

Merging #19554 into master will increase coverage by 3.900%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##            master    #19554       +/-   ##
=============================================
+ Coverage   75.660%   79.561%   +3.900%     
=============================================
  Files          654       674       +20     
  Lines       169941    215660    +45719     
  Branches     40065     60450    +20385     
=============================================
+ Hits        128578    171582    +43004     
- Misses       35745     37955     +2210     
- Partials      5618      6123      +505     

[sylee957]

How would python distinguish the ambiguity at the first place?

I see duplicate names are chosen because there are often no way to name them better.

And requiring every sympy modules, classes, and functions to have unique names can be too much.

[oscarbenjamin]

Part of the problem is the way that in sympy everything is imported down the chain through every __init__.py file and then everything is exposed at the top-level though from sympy import x and sympy.x.

Those things that are not importable in that way just have to be imported from whichever module they happen to be defined in so there is minimal separation between the internal organisation of code and the import interface provided to users.

Ideally we would divide the sympy namespace up so that users import things from the relevant submodules e.g. from sympy.simplification import simplify. Those submodules should just be stubs that import the relevant functions and classes according to a designed import structure that is documented and advertised to users. Then the actual definitions of the functions/classes should live in some clearly internal module that users are not expected to import from like sympy._implementation.simplify or something. Then we would be free to move code around internally without breaking imports for users.

[asmeurer]

What would be the benefit of doing that? The usual touted benefit, that you can import only the submodules you need, mostly doesn't apply to SymPy because the SymPy modules are so circular that importing one thing would force everything else to be imported anyway. Unless you really think such a separation is possible (I'm sceptical).

[oscarbenjamin]

The benefit would be that we could make it clear both to users and contributors what is public (and where to import it from) and what is internal. We would really be free to move things around in the internal part. In the public part we could organise APIs into submodules in a way that makes sense to users rather than for the organisation of code within the implementation. The docs could be organised along the lines of the public API so that they would also seem coherent to users.

Also I think that almost all of the circular imports are not necessary. It would be difficult to remove them now but very few of them needed to be added in the first place.

I still think it would be good to remove the circular imports but there is so little high-level coordination in the codebase that the only way we could maintain something like that would be with tests that reject non-conformant code. As a starting point we could have a test that rejects a top-level imports like from sympy import ... within library code. Those are always problematic, always unnecessary and highly likely to create import problems. Afterwards we'd need to be disciplined about which packages are allowed to import which others.

Doing this properly would require having a documented and widely understood model for the relationships between the different parts of the codebase. For example, is the ntheory module a module of low-level sub-symbolic functions that can be used by the core in building up the basic symbolic routines? If yes, then it's fine for the core to import from it. On the other hand is it a library of symbolic classes that is built on top of the core? If yes, then the core should not import from ntheory because it needs to import from core. In fact ntheory is both of these things and so there are circular imports but it wouldn't be hard to organise things differently.

[asmeurer]

The benefit would be that we could make it clear both to users and contributors what is public (and where to import it from) and what is internal. We would really be free to move things around in the internal part. In the public part we could organise APIs into submodules in a way that makes sense to users rather than for the organisation of code within the implementation. The docs could be organised along the lines of the public API so that they would also seem coherent to users.

I guess I'm not following how exactly that makes it more clear to users what is public. Right now, it should (hopefully) be very clear that anything importable from the top-level is public.

What's less clear is when something isn't importable from the top-level, if it is public or not. For example, a lot of stuff in ntheory isn't importable from the top-level. I currently use sympy.ntheory.modular.crt (Chinese Remainder Theorem) in another library, but it isn't clear to me if it's actually supposed to be public or not. The Polys are another example where it's not obvious what's public. Is it only the top layer, or are lower layers public too? I think the only reliable signal for most Python developers is that if they see an underscore, either in the function name or the module name, then they know that it is not public. So should we carefully make all non-public functions either start with an underscore or be in a module that starts with an underscore? For the examples I just gave, I don't even know if we have really decided ourselves if they are public or not.

I still think it would be good to remove the circular imports but there is so little high-level coordination in the codebase that the only way we could maintain something like that would be with tests that reject non-conformant code. As a starting point we could have a test that rejects a top-level imports like from sympy import ... within library code. Those are always problematic, always unnecessary and highly likely to create import problems. Afterwards we'd need to be disciplined about which packages are allowed to import which others.

I'm not convinced we can so easily avoid circular imports. For example, right now, sympy.solvers.solvers imports simplify (because solve has a simplify flag) and sympy.simplify.simplify imports solve (because some simplification algorithms use solve). Right now, it looks like the simplify import is at the top-level of solvers.py and the solve import is inside a function definition in simplify.py. What's the canonically best way to deal with this, and can it be generalized to the rest of SymPy?

The above example is actually fine because you can easily move an import inside a function definition. Things get worse when you need to import a base class, because that has to go at the module level.

A world where from sympy.submodule.submodule import function only imports a subset of from sympy import *, where function is included in from sympy import * seems very unlikely to achieve. If you look at both simplify.py or solvers.py, for example, they import stuff from all over SymPy.

Afterwards we'd need to be disciplined about which packages are allowed to import which others.

But if function x needs function y to work, then it has to import it. Or do you mean only certain imports are allowed at the top module level and the rest have to go inside of function definitions?

Doing this properly would require having a documented and widely understood model for the relationships between the different parts of the codebase. For example, is the ntheory module a module of low-level sub-symbolic functions that can be used by the core in building up the basic symbolic routines? If yes, then it's fine for the core to import from it. On the other hand is it a library of symbolic classes that is built on top of the core? If yes, then the core should not import from ntheory because it needs to import from core. In fact ntheory is both of these things and so there are circular imports but it wouldn't be hard to organise things differently.

Well take sympy.functions for example. These are clearly symbolic implementations of functions. But the way SymPy is implemented, a lot of the logic for functions is implemented on the class itself. This logic can do almost anything, in terms of what other submodules it needs to import from. solvers, ntheory, assumptions, and so on are all submodules that are reasonably needed for certain _eval_ methods. Right now, for the most part, this is done by moving the imports inside of the methods. But I don't see how this would be done differently via code reorganization. The only thing I can see is if we moved from a model where code is on a class object to a model where it is separate, e.g., with multipledispatch. That may or may not have merits in its own right, but I'm not sure if it's worth changing how our code works in such a drastic way just for the sake of making circular imports easier to deal with.

I'm worried that solving circular imports in SymPy by refactoring code is nearly impossible task, and even if we could rearrange things to that they work now, it doesn't guarantee that that arrangement will continue to make sense for future code.

[oscarbenjamin]

Right now, it looks like the simplify import is at the top-level of solvers.py and the solve import is inside a function definition in simplify.py. What's the canonically best way to deal with this, and can it be generalized to the rest of SymPy?

There isn't a single way to approach all of these situations. I think that the first thing is that top-level API functions for users like solve and simplify should probably be separated from the lower-level functions that implement them which don't actually need to be in the same module. For example we can have sympy/toplevel.py which defines or imports the top level functions. Internally we would use something like _simplify or _solve which at least would have a stricter interface.

I would judge that solve is downstream from simplify so probably simplify should not import solve. At the same time solve should be built out of primitives that can e.g. solve a linear system etc which could be used by simplify. There are 3 calls to solve in the simplify package (outside of tests / docstrings).

There's a call to solve in ratsimpmodprime:

sympy/sympy/simplify/ratsimp.py

Line 162 in 586a432

sol = solve(S, Cs + Ds, particular=True, quick=True)

If I understand correctly this is always solving a linear system with poly-type coefficients. This could be handled by a specific low-level linear equation solver perhaps one that is actually based on Poly or the polys domains (we have this in sympy/polys/solvers.py).

There's a call to solve in hyperexpand.py:

sympy/sympy/simplify/hyperexpand.py

Line 786 in 586a432

n0, = solve(expr.xreplace(repl0) - target, _n)

As far as I can tell this always solves an univariate linear equation with Rational coefficients. There could be a much more efficient primitive function for handling that.

Then there's this one in _nthroot_solve:

sympy/sympy/simplify/simplify.py

Line 940 in 586a432

sols = solve(f, x)

That's always finding the roots of an univariate polynomial so it should be using roots rather than solve.

The fact these places call solve is most likely because we don't have well-defined primitives or a clear idea of which functions should (not) be used in which place or even which are internal and which are for end-users.

[czgdp1807]

@oscarbenjamin Is this good to go? Or if someone else can take this over?

[oscarbenjamin]

This PR is finished. If @sylee957 and @asmeurer are happy with it then it can be merged.

The question is whether or not it is okay to remove the deprecated feature so quickly (after only one release). Personally I think that we could have skipped deprecation altogether in the first place for this. There would have been some breakage but there always is in a sympy release and we know that this change was necessary to reduce breakage in the future. I know of only two cases where downstream libraries have reported seeing this warning. In each case the fix was straight-forward.

If we don't remove this by the next release then we should at least improve the message given in the deprecation warning.

[oscarbenjamin]

@sylee957 @asmeurer the 1.7 release is approaching. What do you think of this PR?

The deprecated import modules are causing warnings during normal operation like e.g. tab-completion in isympy. A couple of downstream issues showed up early on as a result of the warnings but I haven't noticed anything since.

[oscarbenjamin]

I'm re-running the tests in preparation for merging this. @sylee957 @asmeurer please say if you have any thoughts because otherwise I will merge this for the sympy 1.7.