Store display outputs in history for %notebook magic

[Darshan808]

Fixes#1433

Summary

Fixes issue where display outputs (plots) were not included when using %notebook magic to save session as .ipynb file.

Changes

• Modified ZMQDisplayPublisher.publish() to store display data in history_manager.outputs
• Added test to verify display outputs are stored in history

[Darshan808]

Would it make sense to bump the IPython version to >9.1.0 (where HistoryOutput was introduced), or should we instead import HistoryOutput in try block?

[krassowski]

I think a conditional block (could be conditional on version) would be preferred, especially because it would be great to backport to 6.x branch.

[krassowski]

Kicking CI

[krassowski]

@ianthomas23 I think the test failures are unrelated. Do you think we could merge this and backport to 6.x? I do not have merge rights here.

[ianthomas23]

@ianthomas23 I think the test failures are unrelated. Do you think we could merge this and backport to 6.x? I do not have merge rights here.

Yes, I will do that. Do you want a 6.30.2 release for this, I cannot tell how important it is?

[krassowski]

Thinking more about this I wonder if we should add this as opt-in behind a traitlet for now and backport to 6.31.

My reasoning is that I would like to avoid increasing memory usage for notebook users who do not need %notebook magic. Still, it is currently impossible to use %notebook magic fully in say jupyter console so I believe we should backport to 6.x. Any other thoughts @Darshan808?

[Darshan808]

Yes, I agree. I believe very few users will use %notebook outside of IPython. So adding this feature behind a traitlet sounds like a good approach.

[ianthomas23]

Could you repeat the test using store_display_history = False to confirm that mock_shell.history_manager.outputs is empty for it? I guess we cannot use @pytest.mark.parametrize as this is unittest, perhaps it could just be a for enable in [False, True] within this test function.

Other than this, I think it is good to merge.

[ianthomas23]

Thanks @Darshan808.

I'll backport to the 6.x branch and aim for a 6.31.0 release in the next few days. Feel free to ping me if I am being too slow.

[krassowski]

Can we or the bot help?

@meeseeksdev please backport to 6.x

[lumberbot-app]

Awww, sorry krassowski you do not seem to be allowed to do that, please ask a repository maintainer.

[ianthomas23]

@meeseeksdev please backport to 6.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout 6.x
git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 b8f5dfc3a35a658c66b85213e60f634bd8a44488

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #1435: Store display outputs in history for `%notebook` magic'

4. Push to a named branch:

git push YOURFORK 6.x:auto-backport-of-pr-1435-on-6.x

5. Create a PR against branch 6.x, I would have named this PR:

"Backport PR #1435 on branch 6.x (Store display outputs in history for %notebook magic)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

[ianthomas23]

Manually backported to 6.x branch in PR #1461.

[ianthomas23]

6.31.0 is released on PyPI (https://pypi.org/project/ipykernel/6.31.0/) containing the backport of this fix.