fix: do not crash when using IPython but no kernel is available by maartenbreddels · Pull Request #17683 · astropy/astropy

maartenbreddels · 2025-01-28T15:23:02Z

Description

This pull request is to address a crash of astropy when using it in combination with solara (see widgetti/solara#965)

When _WITHIN_IPYTHON was true, it was assumed a kernel would exists. However, looking at
https://github.com/ipython/ipython/blob/637e037d668db7aebe61ae04261ebfc5cb7f32c5/IPython/core/interactiveshell.py#L788
This only means an interactive shell is available, so we should also check if get_ipython() returns a kernel object.

Fixes widgetti/solara#965

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 · 2025-01-28T15:23:31Z

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.

neutrinoceros · 2025-01-28T15:40:14Z

Hi @maartenbreddels, thanks for your work
It's not yet clear to me that this should be fixed on the astropy side. Can you elaborate on why get_ipython returns None when solora is installed, even with __IPYTHON__ being defined ?

neutrinoceros · 2025-01-28T16:01:35Z

More accurately, could you point to where in solara an IPython shell is created and explain why this is seemingly run automatically through pytest ?

pllim · 2025-01-28T18:20:50Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

as other because there is no logger section

pllim · 2025-01-28T18:25:13Z

I think you could have a Jupyter session active with no active kernel if you go into the option and says "kills all kernels". Solara is being used in Jdaviz, which I am also working on. Since this is a blocker for us downstream, I am going to approve. But I will wait to merge in case @astrofrog wants to review too or @neutrinoceros objects to this.

Thanks!

neutrinoceros · 2025-01-28T18:29:41Z

I'll probably end up accepting this without change, but since it fixes a mistake I made I'd like to make sure I understand the approach so I don't do it again 🥲

dhomeier · 2025-01-28T20:11:33Z

astropy/logger.py

    _WITHIN_IPYTHON = False
 else:
-    _WITHIN_IPYTHON = True
+    from IPython import get_ipython


making the imports in L321,341,380 redundant – this might be the more elegant way anyway

Good catch! I took the liberty to add a commit to remove them.

that are no longer necessary

dhomeier · 2025-01-28T20:54:53Z

I think you could have a Jupyter session active with no active kernel if you go into the option and says "kills all kernels".

I think in Jupyter this does not happen, as even after killing all kernels calling get_ipython() (it [re-]importing interactiveshell.InteractiveShell, or in fact already on referencing __IPYTHON__) seems to immediately launch a new instance. May be more specific to Solara then...

pllim · 2025-01-28T20:54:58Z

codecov/patch — 0.00% of diff hit (target 91.87%)

I think we can ignore? 😅

dhomeier

I don't understand the full repercussions of running this in the Solara environment either, but since get_ipython is needed and used anyway, don't see any drawbacks to this change.

pllim · 2025-01-28T23:11:14Z

@neutrinoceros , are you okay with merging this? Please advise. Thanks!

neutrinoceros · 2025-01-29T08:13:12Z

Ok I think I get it now. The name __IPYTHON__ being defined is indeed insufficient to know wether we are in an IPython interactive shell; it not being defined is however still sufficient proof that this isn't the case, so my approach in #16369 was partially incorrect, and this patch corrects it. However, there are other places (easy to review from #16369) with the same logic problem, so one could say this PR is incomplete at the moment. I would prefer that we address all such bugs in one go for posterity. @maartenbreddels, could you expand your PR to fix all of them ? Thanks !

We do this with all the other methods, so it makes sense to do it here as well. This should fix #965 and is an alternative to astropy/astropy#17683

maartenbreddels · 2025-01-29T09:19:46Z

@neutrinoceros

thank you for being critical. While trying to explain why this PR was needed, I discovered I made a mistake in my reasoning. I misremembered that get_ipython should return the kernel, but it should return the shell ( https://ipython.readthedocs.io/en/8.27.0/api/generated/IPython.core.getipython.html )
Solara patched get_ipython and returned None (when not in a virtual kernel), which was inconsistent with what we do to other functions we patch (like InteractiveShell.instance).
widgetti/solara#990 should fix this behaviour, and I think what you do here was actually correct.

…#990) We do this with all the other methods, so it makes sense to do it here as well. This should fix #965 and is an alternative to astropy/astropy#17683

fix: do not crash when using IPython but no kernel is available

6fc6f4f

github-actions bot added the logging label Jan 28, 2025

pllim added this to the v7.0.1 milestone Jan 28, 2025

pllim added Bug backport-v7.0.x labels Jan 28, 2025

pre-commit-ci bot and others added 2 commits January 28, 2025 18:21

[pre-commit.ci] auto fixes from pre-commit.com hooks

1b71e7e

for more information, see https://pre-commit.ci

Add change log

542a3fe

as other because there is no logger section

pllim approved these changes Jan 28, 2025

View reviewed changes

dhomeier reviewed Jan 28, 2025

View reviewed changes

Remove extraneous get_ipython imports

301fdf5

that are no longer necessary

dhomeier approved these changes Jan 28, 2025

View reviewed changes

maartenbreddels mentioned this pull request Jan 29, 2025

fix: get_ipython should dispatch to original if not in virtual kernel widgetti/solara#990

Merged

maartenbreddels closed this Jan 29, 2025

dhomeier mentioned this pull request Jan 29, 2025

Use last focused/resized view to determine shape of BqplotImageView glue-viz/glue-jupyter#475

Merged

1 task

Uh oh!

Conversation

maartenbreddels commented Jan 28, 2025

Description

Uh oh!

github-actions bot commented Jan 28, 2025

Uh oh!

neutrinoceros commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Jan 28, 2025

Uh oh!

pllim commented Jan 28, 2025

Uh oh!

pllim commented Jan 28, 2025

Uh oh!

neutrinoceros commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhomeier Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

pllim Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

dhomeier commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jan 28, 2025

Uh oh!

dhomeier left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 28, 2025

Uh oh!

neutrinoceros commented Jan 29, 2025

Uh oh!

maartenbreddels commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neutrinoceros commented Jan 28, 2025 •

edited

Loading

neutrinoceros commented Jan 28, 2025 •

edited

Loading

dhomeier commented Jan 28, 2025 •

edited

Loading