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

CLN/DEPR: Final panel removal #27101

Merged
merged 14 commits into from
Jul 3, 2019

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Jun 28, 2019

@jorisvandenbossche jorisvandenbossche changed the title Final panel removal CLN/DEPR: Final panel removal Jun 28, 2019
@jorisvandenbossche jorisvandenbossche added this to the 0.25.0 milestone Jun 28, 2019
@jorisvandenbossche
Copy link
Member

That looked easy ;)

@jreback jreback added the Panel label Jun 28, 2019
@jreback
Copy link
Contributor

jreback commented Jun 28, 2019

https://travis-ci.org/pandas-dev/pandas/jobs/551916327

xarray is explicity referencing Panel. not sure if if this is actually failing on the current one though.

@jorisvandenbossche
Copy link
Member

So we will need to keep something like

class Panel:

    def __init__(self, *args, **kwargs):
        raise ImportError("pandas.Panel is removed")

in core/panel.py and still import that in the main pandas namespace to avoid breaking released version of statsmodels and xarray.

@WillAyd
Copy link
Member Author

WillAyd commented Jun 28, 2019

Brief glance at the xarray source this is used for introspection so not sure a dummy class is that useful. Perhaps something more cleanly handled downstream? cc @shoyer

@jreback
Copy link
Contributor

jreback commented Jun 28, 2019

statsmodels is doing the same type of checking ?

cc @jbrockmendel

@jbrockmendel
Copy link
Member

statsmodels is doing the same type of checking ?

Yes. Opened statsmodels/statsmodels#5892 to remove it.

@shoyer
Copy link
Member

shoyer commented Jun 28, 2019 via email

@shoyer
Copy link
Member

shoyer commented Jun 28, 2019

For what it's worth, this is probably an indication that the deprecation notice for Panel was not entirely effective, because it could still be used only for type checks without an error. In the future it would probably make sense to use module level __getattr__ when deprecating classes.

shoyer added a commit to shoyer/xarray that referenced this pull request Jun 28, 2019
It's being removed in the next pandas 0.25 release
(see pandas-dev/pandas#27101).
@jreback
Copy link
Contributor

jreback commented Jun 28, 2019

For what it's worth, this is probably an indication that the deprecation notice for Panel was not entirely effective, because it could still be used only for type checks without an error. In the future it would probably make sense to use module level __getattr__ when deprecating classes.

@shoyer well that's very hard with < 3.7 (we did actually have some machinery to do this but blew it away a while back as it was complex) and not worth the effort IMHO.

@jorisvandenbossche
Copy link
Member

I would "be nice" for now and don't knowingly break other projects. As @shoyer said, it is hard to catch this as the specific use case does not result in a warning message (so there will probably be other projects as well). If we want to release a RC next week, I think it is better if that RC does not break importing statsmodels or xarray.

Or, @shoyer, do you plan to do a xarray bugfix release soonish?

Keeping a dummy class is not that hard (only a couple of lines of code). For Python 3.7 we could maybe actually use the getattribute trick (if it is possible to do that depending on the python version)

@jorisvandenbossche
Copy link
Member

Something like this seems to work (at least on Python 3.7):

diff --git a/pandas/__init__.py b/pandas/__init__.py
index eafb89c52..d1d4cf678 100644
--- a/pandas/__init__.py
+++ b/pandas/__init__.py
@@ -158,3 +158,18 @@ Here are just a few of the things that pandas does well:
     conversion, moving window statistics, moving window linear regressions,
     date shifting and lagging, etc.
 """
+
+if pandas.compat.PY37:
+    def __getattr__(name):
+        if name == 'Panel':
+            import warnings
+            warnings.warn(
+                "The Panel class is removed from pandas. Accessing it from the "
+                "top-level namespace will also be removed in the next version",
+                FutureWarning, stacklevel=2)
+            return None
+        raise AttributeError(
+            "module 'pandas' has no attribute {}".format(name))
+else:
+    class Panel:
+        pass

shoyer added a commit to pydata/xarray that referenced this pull request Jun 30, 2019
@jorisvandenbossche
Copy link
Member

@WillAyd In order to get this merged, are you ok with adding something like the above? (#27101 (comment))

@WillAyd
Copy link
Member Author

WillAyd commented Jul 1, 2019

I would prefer not to add something like that since we have been targeting removal on 0.25 for a while and we still then wouldn't be removing it.

@jorisvandenbossche
Copy link
Member

To be clear, I don't propose to keep the 1000+ lines of panel code. We are removing the actual Panel class. I only propose to add a few lines to give a more gracious deprecation for a use case we have not been raising a warning for.

@jbrockmendel
Copy link
Member

I would prefer not to add something like that since we have been targeting removal on 0.25 for a while and we still then wouldn't be removing it.

Agreed. Especially if it is mainly xarray and statsmodels we're worried about, we can help them get quick bugfix releases out and have this over Once And For All.

@TomAugspurger
Copy link
Contributor

I think a compatibility shim like #27101 (comment) is good to have for a bit.

@WillAyd do you have time to do that? If not, do you mind if I do it sometime tonight?

@shoyer
Copy link
Member

shoyer commented Jul 2, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jul 2, 2019

yeah i think we need this shim.

@WillAyd
Copy link
Member Author

WillAyd commented Jul 2, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jul 3, 2019

@WillAyd I pushed some compat code; let's see.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jul 3, 2019

@jreback I think we should do the getattr check on Python 3.7 as I showed above in #27101 (comment).
That actually gives a deprecation warning for the use case that we are keeping a dummy class for (only if you are using py 3.7, but that is at least something).

@jreback
Copy link
Contributor

jreback commented Jul 3, 2019

@jreback I think we should do the getattr check on Python 3.7 as I showed above in #27101 (comment).
That actually gives a deprecation warning for the use case that we are keeping a dummy class for (only if you are using py 3.7, but that is at least something).

@jorisvandenbossche ok sure

@pep8speaks
Copy link

pep8speaks commented Jul 3, 2019

Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-03 15:44:44 UTC

@WillAyd
Copy link
Member Author

WillAyd commented Jul 3, 2019

Thanks Jeff! Traveling back west today so feel free to move forward on this without me if a blocker. Otherwise can pick up later this evening

@jreback
Copy link
Contributor

jreback commented Jul 3, 2019

@TomAugspurger I think this needs a conditional in pandas/tests/api/tests_api.py where if < py37 then add to the class list (deprecated), otherwise its not there. I can do later today unless you want to push a patch.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jul 3, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jul 3, 2019

doc/source/whatsnew/v0.15.0.rst(734,6): error E999: SyntaxError: invalid syntax
doc/source/whatsnew/v0.11.0.rst(392,12): error E999: SyntaxError: invalid syntax
Bash exited with code '1'.

@TomAugspurger
Copy link
Contributor

Fixing now.

"from the top-level namespace will also be removed in "
"the next version",
FutureWarning, stacklevel=2)
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Downstream are expecting this to be a type so they can do isinstance(thing, pd.Panel). Will make another dummy class Panel here (not outside the getattr, so that we don't pollute the namespace on py37+).

@TomAugspurger
Copy link
Contributor

@jreback the CI failure is unrelated (the categorical ASV we fixed in the other PR).

Should be good to go.

@jorisvandenbossche jorisvandenbossche merged commit 4e185fc into pandas-dev:master Jul 3, 2019
@WillAyd WillAyd deleted the remove-panel branch January 16, 2020 00:35
rs2 added a commit to rs2/pandas that referenced this pull request Jan 21, 2021
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.

CLN: final Panel cleanup
7 participants