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

MRG: First draft of jupyter_compat mode #1394

Merged
merged 5 commits into from Jun 21, 2023

Conversation

matthew-brett
Copy link
Contributor

This is a first attempt to replicate the default Jupyter behavior with
Reticulate.

That is:

  • Always display plots
  • Do not display results of code expressions unless at last code line.
  • Display result of final expression unless suppressed with ;

See: gh-1391

@t-kalinowski
Copy link
Member

This LGTM! Thank you!
Can you please add a NEWS entry?

@matthew-brett
Copy link
Contributor Author

I've added a NEWS entry - look ok?

I had two worries:

  • There are currently two ways of preventing code output - the suppress option, that changes the compile_mode, used now for the standard mode, and my post-hoc deletion of the return value for jupyter_compat. Do we need both?
  • I guess one problem will be, that we can only detect Matplotlib plotting when the code uses standalone expressions, rather than statements. Is that right? If so, it would be easy to miss with code like arrs, bins = plt.hist(some_array).

@t-kalinowski
Copy link
Member

t-kalinowski commented Jun 20, 2023

Thanks

  • In the news entry, can you please include a link to this pr, the original issue, and if you'd like, your github handle?
  • I agree, consolidating the code paths makes sense. I'd be OK always parsing in "single" mode, and then handling or discarding the output afterwards, in eng_python_autoprint() or just after it. Additionally, with this change, it might make sense to completely supplant the 'inspect-the-last-value-and-call-plt.show()-if-its-a-matplotlib-object' logic with the new logic based around matplotlib_pending_show. Seems like that's a cleaner solution over-all.
  • We might not even need a jupyter_compat option, and my preference is not to expand the API if it's not needed. If I'm understanding correctly:
    • The repr of a matplotlib plot is by default always suppressed.
    • With this new approach, a plt.show() is always called at the end of a chunk if a matplotlib plot object was returned by any of the individual expressions.
    • The presence or absence of a trailing ; on the last expression of a chunk returning a matplotlib object is then, effectively, a no-op---it makes no difference since the plt.show() will be called regardless, and the repr() is suppressed regardless.

@matthew-brett
Copy link
Contributor Author

I've added the links to the NEWS entry.

For the logic as currently implemented with jupyter_compat

  • Only the final expression gets output displayed.
  • Semi-colon will suppress that final output, if present, but otherwise, yes, it's a no-op.
  • I defered to eng_python_autoprint for the repr for the Matplotlib-returning expressions, so yes, these are currently suppressed regardless. I guess for full Jupyter compatibility, we should not suppress reprs for the final expression (without a semi-colon).

Did you look at the Matplotlib figure manager? I have a vague memory that is where you can look for any new but not-yet-displayed figures.

@t-kalinowski
Copy link
Member

Thank you!

Did you look at the Matplotlib figure manager? I have a vague memory that is where you can look for any new but not-yet-displayed figures.

I have not. I remember having a brief, unsuccessful, look for something like a "pending undisplayed matplotlib plots queue". I'm open to other suggestions for how to to better detect if there is a pending plot, though I think this new approach embodied in this PR seems good.

@t-kalinowski
Copy link
Member

I guess for full Jupyter compatibility, we should not suppress reprs for the final expression (without a semi-colon).

Is this necessary? The repr seems to me output that nobody will want in their polished and rendered report, perhaps excepting someone writing a matplotlib tutorial.

I'm inclined to keep the current behavior.

@matthew-brett
Copy link
Contributor Author

Is this necessary? The repr seems to me output that nobody will want in their polished and rendered report, perhaps excepting someone writing a matplotlib tutorial.

Fair enough - it's certainly niche - although I can imagine a student being surprised they get the output in the Jupyter notebook, and don't see it in the report.

@matthew-brett
Copy link
Contributor Author

I asked for advice on detecting new plots over on the Matplotlib discourse ....

Drop the older method of autoshow, and document the loss of the ability
to suppress plots with semicolons.
@matthew-brett
Copy link
Contributor Author

I've updated the PR with what I think you were suggesting - is it OK?

As you've implied, this is a behavior change - as you can now longer suppress plot output with terminating semicolons - but my guess is that this was very rarely used, intentionally.

The heuristic for detecting plots will miss this:

res = plt.plot(range(10))

but that would need a completely different approach - maybe soneone in the Matplotlib community has an idea. Jupyter has to do this, so it must be possible.

@matthew-brett matthew-brett changed the title WIP: First draft of jupyter_compat mode MRG: First draft of jupyter_compat mode Jun 21, 2023
@matthew-brett
Copy link
Contributor Author

Also tried asking about plot detection over at Jupyter Discourse.

Copy link
Member

@t-kalinowski t-kalinowski left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@t-kalinowski t-kalinowski merged commit 9163ac3 into rstudio:main Jun 21, 2023
12 of 13 checks passed
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.

None yet

2 participants