DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup by neutrinoceros · Pull Request #16354 · astropy/astropy

neutrinoceros · 2024-04-29T08:07:02Z

Description

In the same vein as #16353, but specifically for IPython since this one requires a "bump" (effectively I'm just declaring as minimal the first version that supports Python 3.10, which is still a little bit shy of SPEC 0's current recommendation at 7.39).

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-04-29T08:07:28Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2024-04-29T08:07:30Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-04-29T08:13:02Z

astropy/utils/console.py

-
-            try:
+            if HAS_IPYTHON:
                from ipykernel.iostream import OutStream


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

astropy/utils/console.py

pllim · 2024-04-30T16:15:07Z

astropy/utils/console.py

-            cls._OutStream = None
-            try:
-                cls.get_ipython()
-            except NameError:


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!

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

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!

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 ?

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?

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

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).

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?

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

should we get #16369 in first?

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

…l and ipywidgets as optional deps, cleanup

…pykernel as optional dep, cleanup

pllim

Thanks. LGTM now!

…l and ipywidgets as optional deps, cleanup (#16354) * DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup * fixup! DEP: bump minimal supported IPython version to 7.32, declare ipykernel as optional dep, cleanup

github-actions bot added utils installation labels Apr 29, 2024

neutrinoceros commented Apr 29, 2024

View reviewed changes

neutrinoceros mentioned this pull request Apr 29, 2024

build: improve opt deps #16340

Merged

1 task

neutrinoceros marked this pull request as ready for review April 29, 2024 08:31

neutrinoceros force-pushed the dep/bump_ipython branch from 79c297b to 8b6918d Compare April 29, 2024 10:01

neutrinoceros marked this pull request as draft April 29, 2024 10:07

neutrinoceros force-pushed the dep/bump_ipython branch from 8b6918d to 81ddc4d Compare April 29, 2024 10:12

neutrinoceros changed the title ~~DEP: bump minimal supported IPython version to 7.32, cleanup~~ DEP: bump minimal supported IPython version to 7.32, declare ipykernel as optional dep, cleanup Apr 29, 2024

neutrinoceros force-pushed the dep/bump_ipython branch from 81ddc4d to 2126ef2 Compare April 29, 2024 10:13

eerovaher reviewed Apr 29, 2024

View reviewed changes

astropy/utils/console.py Outdated Show resolved Hide resolved

neutrinoceros force-pushed the dep/bump_ipython branch from 2126ef2 to 3ef8d9b Compare April 29, 2024 17:38

neutrinoceros marked this pull request as ready for review April 29, 2024 17:38

pllim added this to the v7.0.0 milestone Apr 30, 2024

pllim reviewed Apr 30, 2024

View reviewed changes

neutrinoceros mentioned this pull request May 2, 2024

RFC: improve readability of IPython-specific code blocks #16369

Merged

1 task

neutrinoceros added 2 commits May 2, 2024 12:36

DEP: bump minimal supported IPython version to 7.32, declare ipykerne…

74b304c

…l and ipywidgets as optional deps, cleanup

fixup! DEP: bump minimal supported IPython version to 7.32, declare i…

677a81a

…pykernel as optional dep, cleanup

neutrinoceros force-pushed the dep/bump_ipython branch from 3ef8d9b to 677a81a Compare May 2, 2024 10:36

neutrinoceros changed the title ~~DEP: bump minimal supported IPython version to 7.32, declare ipykernel as optional dep, cleanup~~ DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup May 2, 2024

pllim approved these changes May 2, 2024

View reviewed changes

pllim added Refactoring no-changelog-entry-needed labels May 2, 2024

pllim merged commit d898950 into astropy:main May 2, 2024

neutrinoceros deleted the dep/bump_ipython branch May 2, 2024 20:53

Uh oh!

Conversation

neutrinoceros commented Apr 29, 2024

Description

Uh oh!

github-actions bot commented Apr 29, 2024

Uh oh!

github-actions bot commented Apr 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neutrinoceros May 2, 2024 •

edited

Loading