-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Support Kaleido v1 in Plotly.py #5062
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
base: main
Are you sure you want to change the base?
Conversation
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 few comments and a question, happy to read through again as soon as you want.
plotly/io/_kaleido.py
Outdated
- 'webp' | ||
- 'svg' | ||
- 'pdf' | ||
- 'eps' (Requires the poppler library to be installed and on the PATH) |
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.
do we still handle EPS?
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.
@gvwilson Following up on this — Kaleido v1 does not support EPS yet. So either we drop support for EPS entirely, or document that EPS is only available with Kaleido v0 and add an informative error message.
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.
please do the latter - thanks
- "kaleido": Use Kaleido for image export | ||
- "orca": Use Orca for image export | ||
- "auto" (default): Use Kaleido if installed, otherwise use orca | ||
engine (deprecated): str |
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.
do we print a deprecation warning if this argument is used?
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.
Not yet, assuming we are in agreement about removing this argument, I'll add one
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 in agreement that we should remove this argument
def check_image(path_or_buffer, size=(700, 500), format="PNG"): | ||
if format == "PDF": | ||
img = PdfReader(path_or_buffer) | ||
# TODO: There is a conversion factor needed here |
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.
why?
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.
Part of the TODO is for me to educate myself on how Plotly currently determines PDF size when writing to PDF. :)
But the size
argument is measured in pixels (at least for raster file types) and PDFs have no concept of pixels so I wouldn't expect the PDF to report the same "size" as passed in the argument necessarily.
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 will also take a looksy on that issue, lets say for this and the above tomorrow. I am flying today.
cc @ayjayt |
Do we need to maintain backwards compatibility with older Kaleido versions here? |
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.
Are there percy tests that use kaleido to make sure that the actual images generated are correct?
… compatible kaleido version
…into upgrade-kaleido
plotly/io/_defaults.py
Outdated
def __init__(self): | ||
self.default_format = "png" | ||
self.default_width = 700 | ||
self.default_height = 500 | ||
self.default_scale = 1 | ||
|
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.
Will there be a way to set mathjax
, topojson
, and mapbox_access_token
, the other export settings available in v0?
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.
mapbox_access_token
isn't relevant anymore since Mapbox traces are no longer supported; the other two I guess we should expose. I'll add a TODO.
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.
topojson
has been added but I have not tested it yet; mathjax
is added in defaults
but not functional quite yet
Support both the Kaleido v0 and v1 APIs in Plotly.py.
fig.write_image()
,fig.to_image()
,pio.write_image()
, andpio.to_image()
to work with both Kaleido versionsengine
argument toto_image()
andwrite_image()
kaleido.scope.*
plotly.io.defaults.*
. This can be used in place ofkaleido.scope.*
and supports all the same settings:default_format
,default_height
,default_width
,default_scale
,mathjax
, andtopojson
pio.write_images()
function for generating multiple images at a timewrite_image()
in a looppio.get_chrome()
function which wrapskaleido.get_chrome_sync()
, and add a CLI entry pointplotly_get_chrome
for installing Chrome from the command linetests/test_optional/test_kaleido/test_kaleido.py
tests to work with either Kaleido v0 or v1. They now also actually check that an image of the right format and dimensions has been generated, as opposed to using mocking to check that the right arguments are being passed to Kaleido.pio.write_images()
TODO
engine
argument towrite_image
/to_image
mathjax
andtopojson