-
Notifications
You must be signed in to change notification settings - Fork 101
python: bypass plotnine auto-closing comms #7657
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
Conversation
E2E Tests 🚀 |
extensions/positron-python/python_files/posit/positron/positron_ipkernel.py
Show resolved
Hide resolved
wip for feedback, but i'll add a test before we merge this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! This branch seems to fix the weirdness in the linked issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great for me! This feels about as surgical as it could get in this case!
extensions/positron-python/python_files/posit/positron/positron_ipkernel.py
Show resolved
Hide resolved
extensions/positron-python/python_files/posit/positron/positron_ipkernel.py
Outdated
Show resolved
Hide resolved
extensions/positron-python/python_files/posit/positron/positron_ipkernel.py
Outdated
Show resolved
Hide resolved
extensions/positron-python/python_files/posit/positron/positron_ipkernel.py
Show resolved
Hide resolved
…n_ipkernel.py Co-authored-by: Wasim Lorgat <[email protected]> Signed-off-by: Isabel Zimmerman <[email protected]>
self.figure.set_layout_engine("tight") | ||
try: | ||
# plotnine plots use their own layout engine, do not override | ||
if self.figure._layout_engine.__module__ == "plotnine._mpl.layout_engine": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the retina problems were stemming from us overriding plotnine
's layout manager! updated so we don't override it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Can we use the public method self.figure.get_layout_engine()
? Might also need to check that it isn't None
. And it might be safer to do something like .startswith('plotnine')
in case they move it around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we think of always leaving the layout engine alone if it is not None? I understand the sizing won't be exact, but it might be a simple escape hatch to respect user's settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a better approach! We'd just need to double-check that it doesn't break any existing behavior. Might be covered by e2e tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic! what a bug
Release Notes
New Features
Bug Fixes
plotnine
plots throw: "Error rendering plot to 'Auto' size: RPC timed out..." #6393 Fixes plotnine bug where every plot gives RPC timeout errorQA Notes
pip install plotnine
Should create 1 plot with no errors. You can also check that in the logs, the plot comms are being used, rather than
display_data
@:plots