maint(fix): Add deprecation warning for accidental imports #19316

oscarbenjamin · 2020-05-14T22:31:33Z

In #18245 a number of modules were
removed from the list of names imported by

from sympy import *

This adds those names back although with most of them wrapped in an
object that emits DeprecationWarning each time it is accessed.

References to other Issues or PRs

See the discussion on the mailing list:
https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sympy/HYo616T3EwQ/g-z6ZrNKAgAJ

Brief description of what is fixed or changed

Other comments

Release Notes

NO ENTRY

In sympy#18245 a number of modules were removed from the list of names imported by from sympy import * This adds those names back although with most of them wrapped in an object that emits DeprecationWarning each time it is accessed.

sympy-bot · 2020-05-14T22:31:42Z

✅

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

No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

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

In https://github.com/sympy/sympy/pull/18245 a number of modules were
removed from the list of names imported by

   from sympy import *

This adds those names back although with most of them wrapped in an
object that emits DeprecationWarning each time it is accessed.

<!-- 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

See the discussion on the mailing list:
https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sympy/HYo616T3EwQ/g-z6ZrNKAgAJ

#### Brief description of what is fixed or changed

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. 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 -->
NO ENTRY
<!-- END RELEASE NOTES -->

sympy-bot · 2020-05-14T22:31:44Z

🟠

Hi, I am the SymPy bot (v158). 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 add new files:

858a968:
- sympy/deprecated/tests/test_deprecated_imports.py

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

codecov · 2020-05-15T00:07:29Z

Codecov Report

Merging #19316 into 1.6 will decrease coverage by 0.037%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##               1.6    #19316       +/-   ##
=============================================
- Coverage   75.652%   75.615%   -0.038%     
=============================================
  Files          651       651               
  Lines       169528    169549       +21     
  Branches     40021     40022        +1     
=============================================
- Hits        128253    128205       -48     
- Misses       35654     35717       +63     
- Partials      5621      5627        +6

oscarbenjamin · 2020-05-15T00:09:43Z

Does anyone want to review this?

CC @moorepants @asmeurer

asmeurer · 2020-05-15T04:23:27Z

Is the idea when the deprecation ends to continue to also have top-level modules be removed as well? I think they should be.

asmeurer · 2020-05-15T04:24:30Z

sympy/__init__.py

+#=======================================================================
+
+
+class DeprecatedImportModule:


This should subclass types.ModuleType.

What difference does subclassing ModuleType make?

It makes certain things that operate on modules work correctly. For example, help() gives special output on modules (actually that is wrong anyway, because this doesn't inherit the docstring properly).

I also wonder if this could be an issue

>>> from sympy import ode as ode1 >>> from sympy.solvers import ode as ode2 >>> ode1 == ode2 False

It might or might not be an issue, I'm not sure. I guess if this turns out to cause problems, we can do a patch release that removes it.

I think I'd prefer the help to be a message saying that you haven't imported the module in the right way (if there is any need for help at all).

The purpose of providing these deprecated module imports is just to keep code that possibly depends on importing this way chugging along but with warnings. I don't see any need to provide any other functionality.

moorepants · 2020-05-15T04:33:14Z

sympy/__init__.py

+    'multipledispatch',
+    'ntheory',
+    'parsing',
+    # 'physics',


Maybe remove if this wasn't imported before?

moorepants · 2020-05-15T04:41:39Z

sympy/__init__.py

+# __init__.py file. The plan is to remove them but for now they remain
+# importable but will give a deprecation warning when used.
+#
+#=======================================================================


I don't understand the purpose of have this comment block repeated. It refers to the names below, but there is nothing below?

There's a bug list of names in _DEPRECATED_IMPORTS below. The difference between these and those that are listed above is that these are deprecated and will give a deprecation warning. The ones above (in this PR currently) are not deprecated and will give no warning when used.

moorepants · 2020-05-15T05:02:04Z

As I understand this, some subset of all sympy modules were imported with from sympy import *. Those modules were then available in the namespace and functions, classes, and variables could be accessed with module.function() for example. So, with this warning you can still import and use the modules but you get a warning when doing so. I don't quite understand how you've implemented it, but the result seems very good.

It also seems you could do submodule star imports:

from sympy.core import *
add.Add()

I assume those will not get deprecation warnings with this?

asmeurer · 2020-05-15T05:20:06Z

I think they don't but the mechanism could be added. However, my opinion is that submodule star imports should really be supported except for submodules that aren't already included in the main sympy/__init__.py.

oscarbenjamin · 2020-05-15T09:30:20Z

Is the idea when the deprecation ends to continue to also have top-level modules be removed as well? I think they should be.

I don't see any way to do that with a deprecation period so if we're removing them them from star import we might as well just do it now.

oscarbenjamin · 2020-05-15T09:44:06Z

I assume those will not get deprecation warnings with this?

This only gives deprecation warnings for modules that were previously importable from top level.

For example in sympy 1.5 you can do:

>>> from sympy.combinatorics import *
>>> prufer
<module 'sympy.combinatorics.prufer' from '/Users/enojb/current/sympy/sympy/sympy/combinatorics/prufer.py'>

With 1.6 the name prufer would not be imported. This PR does not change that.

asmeurer · 2020-05-15T20:38:19Z

I don't see any way to do that with a deprecation period so if we're removing them them from star import we might as well just do it now.

If we are going to do a deprecation I think it is better to remove them together, even if we can't issue a warning for some of the cases.

This commit clarifies the comments about what will or will not be importable in future.

Clarify comments in __init__.py add docstring for DeprecatedImportModule

oscarbenjamin · 2020-05-17T00:00:32Z

@moorepants @asmeurer does this look good?

If so I'll merge it and along with a fix for #19326 I can put out a second release candidate.

moorepants · 2020-05-17T00:04:35Z

I'm good with this, as long as it gives a warning on the most likely cases. Thank you for addressing my concern.

These names were importable before sympy 1.6 by accident. They were replaced with deprecated import objects to give a warning in sympy#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.

oscarbenjamin added this to the SymPy 1.6 milestone May 14, 2020

oscarbenjamin added the Uncategorised Doesn't fit other labels... label May 14, 2020

asmeurer reviewed May 15, 2020

View reviewed changes

moorepants reviewed May 15, 2020

View reviewed changes

sympy/__init__.py Outdated

'multipledispatch',

'ntheory',

'parsing',

# 'physics',

Copy link

Member

moorepants May 15, 2020

Choose a reason for hiding this comment

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

Maybe remove if this wasn't imported before?

moorepants reviewed May 15, 2020

View reviewed changes

oscarbenjamin added 2 commits May 15, 2020 23:39

maint(fix): Add deprecation warning for accidental imports

4924845

This commit clarifies the comments about what will or will not be importable in future.

maint(fix): Add deprecation warning for accidental imports

1e565a7

Clarify comments in __init__.py add docstring for DeprecatedImportModule

oscarbenjamin merged commit 7d63577 into sympy:1.6 May 18, 2020

oscarbenjamin deleted the pr_deprecate_imports branch May 31, 2020 22:38

oscarbenjamin mentioned this pull request Jun 14, 2020

Docs build fails due to warnings after sphinx update #19551

Closed

oscarbenjamin mentioned this pull request Jun 14, 2020

maint(sympy): Remove deprecated imports #19554

Merged

oscarbenjamin mentioned this pull request Jul 9, 2020

Final flake8 PR... #18245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maint(fix): Add deprecation warning for accidental imports #19316

maint(fix): Add deprecation warning for accidental imports #19316

oscarbenjamin commented May 14, 2020

sympy-bot commented May 14, 2020

sympy-bot commented May 14, 2020 •

edited

codecov bot commented May 15, 2020 •

edited

oscarbenjamin commented May 15, 2020

asmeurer commented May 15, 2020

asmeurer May 15, 2020

oscarbenjamin May 15, 2020

asmeurer May 15, 2020

oscarbenjamin May 15, 2020

moorepants May 15, 2020

moorepants May 15, 2020

oscarbenjamin May 15, 2020

moorepants commented May 15, 2020

asmeurer commented May 15, 2020

oscarbenjamin commented May 15, 2020

oscarbenjamin commented May 15, 2020

asmeurer commented May 15, 2020

oscarbenjamin commented May 17, 2020

moorepants commented May 17, 2020

		#=======================================================================


		class DeprecatedImportModule:

maint(fix): Add deprecation warning for accidental imports #19316

maint(fix): Add deprecation warning for accidental imports #19316

Conversation

oscarbenjamin commented May 14, 2020

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented May 14, 2020

sympy-bot commented May 14, 2020 • edited

🟠

codecov bot commented May 15, 2020 • edited

Codecov Report

oscarbenjamin commented May 15, 2020

asmeurer commented May 15, 2020

asmeurer May 15, 2020

Choose a reason for hiding this comment

oscarbenjamin May 15, 2020

Choose a reason for hiding this comment

asmeurer May 15, 2020

Choose a reason for hiding this comment

oscarbenjamin May 15, 2020

Choose a reason for hiding this comment

moorepants May 15, 2020

Choose a reason for hiding this comment

moorepants May 15, 2020

Choose a reason for hiding this comment

oscarbenjamin May 15, 2020

Choose a reason for hiding this comment

moorepants commented May 15, 2020

asmeurer commented May 15, 2020

oscarbenjamin commented May 15, 2020

oscarbenjamin commented May 15, 2020

asmeurer commented May 15, 2020

oscarbenjamin commented May 17, 2020

moorepants commented May 17, 2020

sympy-bot commented May 14, 2020 •

edited

codecov bot commented May 15, 2020 •

edited