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.

Sign up for GitHub

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

Jump to bottom

DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup #16354

Merged
merged 2 commits into from
May 2, 2024
Merged

DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup #16354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74b304c
DEP: bump minimal supported IPython version to 7.32, declare ipykerne…
neutrinoceros Apr 29, 2024
677a81a
fixup! DEP: bump minimal supported IPython version to 7.32, declare i…
neutrinoceros May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions astropy/utils/compat/optional_deps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
"fsspec",
"h5py",
"html5lib",
"ipykernel",
"IPython",
"ipywidgets",
"jplephem",
"lxml",
"matplotlib",
Expand Down
64 changes: 18 additions & 46 deletions astropy/utils/console.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
_CAN_RESIZE_TERMINAL = False

from astropy import conf
from astropy.utils.compat.optional_deps import (
HAS_IPYKERNEL,
HAS_IPYTHON,
HAS_IPYWIDGETS,
)

from .decorators import classproperty, deprecated
from .misc import isiterable
Expand All @@ -47,44 +52,27 @@
class _IPython:
"""Singleton class given access to IPython streams, etc."""

@classproperty
def get_ipython(cls):
try:
from IPython import get_ipython
except ImportError:
pass
return get_ipython

@classproperty
def OutStream(cls):
if not hasattr(cls, "_OutStream"):
cls._OutStream = None
try:
cls.get_ipython()
except NameError:
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what this NameError was for but looks like handling it has been dropped in this PR. Can you please clarify? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The get_ipython method was oddly written in such a way that its return variable was only bound if IPython was installed, meaning it was effectively relying on a NameError being raised otherwise. Since I also rewrote the method in a more conventional manner, this handling isn't needed any more

Copy link
Member

Choose a reason for hiding this comment

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

Still, looking at code in main, the original design looks like it never intended to throw exception if IPython is not installed, but this PR does. Can we refactor in a way that does not throw exception? Thanks for your patience!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused

original design looks like it never intended to throw exception if IPython is not installed

if you mean the get_ipython class method, it seems to me that it was actually designed to throw an exception since that's how it's been used through except NameError (albeit a quirky exception to catch in your own code).

but this PR does

I don't understand, where else would an exception be raised in my version ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually the NameError on main makes no sense to me. When I try to emulate the implementation, I get UnboundLocalError. But that is neither here nor there...

The point I was trying to make is that get_ipython on main defers the exception handling to OutStream. But in this PR, you explicitly throws ModuleNotFoundError from within get_ipython if IPython is not installed. I think that is a subtle behavior change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it was easy enough that I just went for it: #16369

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I figured that this _IPython.get_ipython method was not actually needed, so I refactored it. To make it easier for you to follow after I rebased, I've kept this one change as a separate commit (that should eventually get squashed).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, does look much cleaner now. But I still don't know what is the point of this whole _IPython class now that I dig into the code. I can't seem to see it being used anywhere and it is "hidden".

Anyways, should we get #16369 in first?

Copy link
Member

Choose a reason for hiding this comment

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

Ah nvm, found it. Just used internally here for isatty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we get #16369 in first?

these PRs are actually completely independent as it turns out; they can be merged in any order

return None

try:
if HAS_IPYKERNEL:

Check warning on line 58 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L58 

Added line #L58 was not covered by tests
from ipykernel.iostream import OutStream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IPython.zmq was removed in 2013 in ipython/ipython#4121 and this functionality was moved to IPython.kernel, which was itself deprecated in IPython 4.0.0 and removed in 8.0.0, see ipython/ipython#13386

except ImportError:
try:
from IPython.zmq.iostream import OutStream
except ImportError:
return None

cls._OutStream = OutStream
cls._OutStream = OutStream

Check warning on line 61 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L61 

Added line #L61 was not covered by tests
else:
cls._OutStream = None

Check warning on line 63 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L63 

Added line #L63 was not covered by tests

return cls._OutStream

@classproperty
def ipyio(cls):
if not hasattr(cls, "_ipyio"):
try:
if HAS_IPYTHON:

Check warning on line 70 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L70 

Added line #L70 was not covered by tests
from IPython.utils import io
except ImportError:
cls._ipyio = None
else:

cls._ipyio = io
else:
cls._ipyio = None

Check warning on line 75 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L75 

Added line #L75 was not covered by tests
return cls._ipyio


Expand All @@ -108,26 +96,7 @@
if _IPython.OutStream is None or (not isinstance(file, _IPython.OutStream)):
return False

# File is an IPython OutStream. Check whether:
# - File name is 'stdout'; or
# - File wraps a Console
if getattr(file, "name", None) == "stdout":
return True

if hasattr(file, "stream"):
# FIXME: pyreadline has no had new release since 2015, drop it when
# IPython minversion is 5.x.
# On Windows, in IPython 2 the standard I/O streams will wrap
# pyreadline.Console objects if pyreadline is available; this should
# be considered a TTY.
try:
from pyreadline.console import Console as PyreadlineConsole
except ImportError:
return False

return isinstance(file.stream, PyreadlineConsole)

return False
return getattr(file, "name", None) == "stdout"

Check warning on line 99 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L99 

Added line #L99 was not covered by tests


@deprecated("6.1", alternative="shutil.get_terminal_size")
Expand Down Expand Up @@ -586,9 +555,12 @@
"""
# Create and display an empty progress bar widget,
# if none exists.

if not hasattr(self, "_widget"):
# Import only if an IPython widget, i.e., widget in iPython NB
_IPython.get_ipython()
if not HAS_IPYWIDGETS:
raise ModuleNotFoundError("ipywidgets is not installed")

Check warning on line 562 in astropy/utils/console.py

View check run for this annotation

Codecov / codecov/patch

astropy/utils/console.py#L561-L562 

Added lines #L561 - L562 were not covered by tests

from ipywidgets import widgets

self._widget = widgets.FloatProgress()
Expand Down
8 changes: 6 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ test = [
test_all = [
"astropy[test]", # installs the [test] dependencies
"objgraph",
"ipython>=4.2",
"ipykernel",
"ipython>=7.32",
"ipywidgets",
"coverage[toml]",
"skyfield>=1.20",
"sgp4>=2.3",
Expand Down Expand Up @@ -88,7 +90,9 @@ all = [
"mpmath",
"asdf-astropy>=0.3",
"bottleneck",
"ipython>=4.2",
"ipykernel",
"ipython>=7.32",
"ipywidgets",
"pytest>=7.0",
"fsspec[http]>=2023.4.0",
"s3fs>=2023.4.0",
Expand Down