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
patch plt.show() and use AGG beneath #1023
Conversation
e4cc6a0
to
718fc92
Compare
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.
Left a comment and a suggestion.
Is this a hard thing to patch upstream? Just wondering if it should be fixed there as well and then we can remove this code 🤔
We may need to add a test to confirm that the graph is shown correctly 😄
pyscriptjs/src/main.ts
Outdated
@@ -203,6 +203,21 @@ export class PyScriptApp { | |||
// XXX: maybe the following calls could be parallelized, instead of | |||
// await()ing immediately. For now I'm using await to be 100% | |||
// compatible with the old behavior. | |||
|
|||
// needed for patch inside pyscript.py | |||
await runtime.installPackage("matplotlib"); |
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.
I'm slightly concerned about installing matplotlib all the time even if we don't need it.
What do you think about adding the patch into a try/pass instead?
pyscriptjs/src/python/pyscript.py
Outdated
os.environ["MPLBACKEND"] = "AGG" | ||
|
||
import matplotlib.pyplot # noqa: E402 | ||
|
||
_old_show = matplotlib.pyplot.show | ||
assert _old_show, "matplotlib.pyplot.show" | ||
|
||
|
||
def show(*, block=None): | ||
buf = io.BytesIO() | ||
matplotlib.pyplot.savefig(buf, format="png") | ||
buf.seek(0) | ||
display(matplotlib.pyplot) | ||
matplotlib.pyplot.clf() | ||
|
||
|
||
matplotlib.pyplot.show = show |
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.
Perhaps we could do something like:
from contextlib import suppress
with suppress(ImportError):
import matplotlib.pyplot
_old_show = ...
That way if we can't import matplotlib we just pass.
Hi @FabioRosado But your concerns are valid. PS -- Jupyterlite also uses a similar kind of hack |
Ah apologies, I just looked at the title and didn't read the main PR text |
718fc92
to
4b77b6a
Compare
Closing, since we don't really need this. |
This is kinda WIP and may require improvements
But showcases how things like #983 can be fixed.