Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@jonmmease jonmmease commented Jul 3, 2020 •

edited

Loading

This PR adds preview support for performing static image export using Kaleido instead of Orca. See the Kaleido README for discussion of project motivations and how it compares to Orca. There has not yet been a stable release of Kaleido, so the API may still evolve a little.

With this PR, Kaleido support is enabled by installing the kaleido package from PyPI...

$ pip install kaleido

and setting the new engine kwarg in plotly.io.to_image and plotly.io.write_image to "kaleido". "kaleido" is also the default value when kaleido is installed.

TODO:

Add engine kwarg to image renderers to make it possible to override default engine
Add tests

Original Approach

With this PR, Kaleido support is enabled by installing the kaleido package from PyPI...

$ pip install kaleido

and importing the kaleido_export future flag before importing plotly

from _plotly_future_ import kaleido_export
import plotly.io as pio
pio.to_image

 <function plotly.io._kaleido.to_image(fig, format=None, width=None, height=None, scale=None, validate=True)>

When Kaleido mode is enabled, the to_image and write_image functions use Kaleido rather than Orca.

There is no change in default behavior if the kaleido_export future flag is not imported

There are still open questions about how we will roll out the transition from Orca to Kaleido. For example, do we wait for a version 5 for the switch? Do we support both Orca and Kaleido for a time and design a mechanism to dynamically select between them? What should we do with the plotly.io.orca module after orca support is dropped? Should we remove it, or remap compatible options to plotly.io.kaleido even though only a subset of the options are compatible?

This PR doesn't make any assertions about how we should do that going forward except that the Kaleido approach will still be invoked using the current plotly.io.to_image and plotly.io.write_image functions. I'd like to release this preview support in version 4.9.0 so we can start asking people to try it out and gathering feedback (especially when people have problems getting Orca working).

jonmmease added 2 commits

July 3, 2020 14:34


 Add kaleido image export option behind the kaleido_export future flag

1c18fe4


 Add CHANGELOG entry

Copy link

Contributor Author

jonmmease commented Jul 5, 2020

As of kaleido==0.0.1rc5, Kaleido supports EPS and EMF formats using poppler (for EPS) and inkscape (for EMF) the same way Orca does.

This brings Kaleido to feature parity with Orca, which would make it more feasible to do a complete Orca to Kaleido switchover in a version 5.

emmanuelle

emmanuelle reviewed

Jul 6, 2020

packages/python/plotly/plotly/io/_kaleido.py Show resolved Hide resolved

Copy link

Contributor

nicolaskruchten commented Jul 6, 2020

How about we add a new engine flag to to_image and write_image which defaults to auto in which case it uses Kaleido if installed, otherwise Orca. People could then force it to orca if they happen to have both installed and prefer the old way? I would feel comfortable doing this without bumping to 5.x :)

@emmanuelle

Copy link

Contributor

emmanuelle commented Jul 6, 2020

Would it be possible that fig.write_image? displays the docstring of the corresponding pio function, mentioning kaleido? This is not the case at the moment if I'm not mistaken.

Copy link

Contributor Author

jonmmease commented Jul 6, 2020

Would it be possible that fig.write_image? displays the docstring of the corresponding pio function, mentioning kaleido? This is not the case at the moment if I'm not mistaken.

@emmanuelle I'm not sure I understand what you mean. Are you talking about auto-generating the docstrings? We used to do this, but I removed it during the import optimization work because there was a non-trivial import delay.

Copy link

Contributor Author

jonmmease commented Jul 6, 2020

How about we add a new engine flag to to_image and write_image which defaults to auto in which case it uses Kaleido if installed, otherwise Orca.

This works for to_image, but there would also need to be a solution for the image renderers. We could add an engine option to the image renderers as well I suppose. I'll take a look.

jonmmease added 3 commits

July 7, 2020 05:59


 Add eps/emf formats

b546fcc


 Replace future flag with engine kwarg

7f65f5f


 emf only available with kaleido

556ef96

@jonmmease jonmmease changed the title ~~(削除) Add Kaleido image export preview behind the kaleido_export future flag (削除ここまで)~~ (追記) [WIP]Add Kaleido image export preview behind the kaleido_export future flag (追記ここまで)

Jul 7, 2020

@jonmmease jonmmease changed the title ~~(削除) [WIP]Add Kaleido image export preview behind the kaleido_export future flag (削除ここまで)~~ (追記) [WIP] Add Kaleido image export preview behind the kaleido_export future flag (追記ここまで)

Jul 7, 2020

Copy link

Contributor Author

jonmmease commented Jul 7, 2020

Ok, I removed the future flag and added an engine kwarg to the plotly.io.to_image and plotly.io.write_image functions. Still need to add engine to the image renderes, but I think this approach will work well. I also still need to port some of the image tests from orca to run with Kaleido as well.

jonmmease added 8 commits

July 8, 2020 05:35


 import kaleido submodule when plotly.io is imported

ed71d6a


 Update Image renderers with engine parameter

da4cb83

Don't auto-start orca when image renderer is activated because now
we might not end up using orca.


 Added kaleido engine tests

3e1cbd4


 Merge remote-tracking branch 'origin/master' into kaleido

299165d


 Python 2.7 mock compatibility

411ffb4


 Python 2.7 mock compatibility

de44623


 Update CHANGELOG.md

c5f4e48


 Add kaleido test that actually calls Kaleido and checks that the

4c5fe76

resulting bytes are reasonable

Copy link

Contributor Author

jonmmease commented Jul 8, 2020

@nicolaskruchten and @emmanuelle this is ready for another review. I removed the future flag, added an engine kwarg to the to_image and write_image functions, and to the Image renderer subclasses.

For testing, I took a different approach than with orca. I'm basically just using mocks to make sure that the PlotlyScope.transform method is called with the expected arguments in various situations.I think this is a bit cleaner since testing against image baselines is going to be handled by the kaleido project.

Copy link

Contributor Author

jonmmease commented Jul 8, 2020

The python-2.7-optional tests are failing due to a compatibility issue in kaleido. I'm building a new version that should be compatible with 2.7.

The python-3.7-orca test is failing due to that dendrogram error. Has anyone worked out the cause of that?

Copy link

Contributor

nicolaskruchten commented Jul 8, 2020

The 3.7 orca test failure is logged as #2618 and I should get to it this week :)

Copy link

Contributor

nicolaskruchten commented Jul 8, 2020

Thanks for adding the engine stuff! So if you removed the future flag, the default is "Kaleido if it's present" basically right? I kind of thought we would keep the future flag to control the polarity of the "auto" setting but I'm on the fence. In the short run, no one will have Kaleido installed for any reason other than to use it with Plotly, but in the future, there may be some situation we can't yet foresee wherein folks have Kaleido installed for some other library support and yet there's some issue such that they want to use Orca for Plotly and they'd consider this a breaking change.

I think I'm ok with that though... thoughts?

Copy link

Contributor

nicolaskruchten commented Jul 8, 2020

Are you planning on updating the docs in this PR as well? I think I'd like that if possible, including changes to the README and getting-started.md please :)

Copy link

Contributor Author

jonmmease commented Jul 8, 2020

I think I'd be fine bringing the future flag back to control the default. But when would you picture changing the default, not till a v5?

Yeah, I'll do the docs and README. So far I've held off because I wasn't sure how much to present Kaleido as experimental with Orca as stable, vs Kaleido as recommended and Orca as legacy.


 Remove broken EMF format as option

011ec72

Copy link

Contributor

nicolaskruchten commented Jul 8, 2020

I think I'd be fine bringing the future flag back to control the default. But when would you picture changing the default, not till a v5?

I'm not dead-set on adding the future flag... just wanted to bring up/discuss this outside chance of some future where someone is annoyed about this. I think I'm OK with running this risk.

jonmmease added 3 commits

July 8, 2020 18:53


 Merge remote-tracking branch 'origin/master' into kaleido

b5fc3db


 Remove broken EMF format as option

f8bff56


 Update image export documentation to recommend and describe Kaleido

827af91

Copy link

Contributor Author

jonmmease commented Jul 9, 2020

I updated the README and image export documentation to recommend Kaleido, while still describing how to use orca.

Note that the documentation for installing Kaleido using conda is dependent on the acceptance of conda-forge/staged-recipes#12093 into conda-forge.

I'll merge in the dendrogram fix as soon as #2627 is merged, but otherwise this is ready for review.

jonmmease added 4 commits

July 9, 2020 15:11


 Add engine docstring to figure image export methods

ef6e68d


 Change kaleido conda channel to plotly since it most likely won't be ...

fc7d856

...available on conda-forge initially


 Conda package renamed from kaleido -> python-kaleido

aee0e0d


 Merge remote-tracking branch 'origin/master' into kaleido

32f6e7f

@jonmmease jonmmease changed the title ~~(削除) [WIP] Add Kaleido image export preview behind the kaleido_export future flag (削除ここまで)~~ (追記) Add Kaleido image export support (追記ここまで)

Jul 13, 2020

@nicholas-esterer @emmanuelle @nicholas-esterer

Copy link

Contributor Author

jonmmease commented Jul 13, 2020

Tests are passing now and this is ready for re-review.

nicolaskruchten reviewed

README.md Outdated

### Static Image Export with Orca

While Kaleido is now the recommended approach, image export can also be supported

Copy link

Contributor

@nicolaskruchten nicolaskruchten Jul 14, 2020

Choose a reason for hiding this comment

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

I would mention that Orca is the older, harder to install option.

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

Update in 00987c0

nicolaskruchten reviewed

doc/python/orca-management.md Outdated

### Overview

This section covers the lower-level details of how plotly.py uses orca to perform static image generation. Please refer to the [Static Image Export](/python/static-image-export/) section for general information on creating static images from plotly.py figures.

This section covers the lower-level details of how plotly.py can use orca to perform static image generation. Please refer to the [Static Image Export](/python/static-image-export/) section for general information on creating static images from plotly.py figures.

Copy link

Contributor

@nicolaskruchten nicolaskruchten Jul 14, 2020

Choose a reason for hiding this comment

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

I would add a section above this saying "Orca is no longer the recommended way to do static export, and we now recommend Kaleido" with a link back to the static image export page.

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

Update in a25f5b2

nicolaskruchten reviewed

doc/python/static-image-export.md Outdated

##### conda

Using the [conda](https://conda.io/docs/) package manager, you can install these dependencies in a single command:

#### Install Dependencies (Kaleido)

Static image generation requires the [`kaleido`](https://github.com/plotly/Kaleido) package which can be installed using pip...

Copy link

Contributor

@nicolaskruchten nicolaskruchten Jul 14, 2020

Choose a reason for hiding this comment

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

strictly speaking this isn't correct, it requires "either Kaleido (recommended) or Orca (deprecated)" or something like that, no? Just to avoid confusing people who already know Orca? (same comment applies in other places in this PR where we've said Kaleido is required)

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

It's not in the diffs but under ### Write Image File only orca is referred to so maybe it these could be changed to "kaleido or orca" or "the image export system in use" or something like that. For example, in this section it says: "Orca can output figures to several raster image formats including PNG, ...".

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

I can't select it but on line 40 JEPG should be JPEG.

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

Thanks:

JPEG change (a033df0)
"Write Image File" changes (2bdcfe2)
Merged install dependency sections and better explain that orca is legacy and kaleido is new (d1fc9a8)

nicolaskruchten reviewed

CHANGELOG.md Outdated Show resolved Hide resolved

Copy link

Contributor

nicolaskruchten commented Jul 14, 2020

💃 modulo the documentation feedback

@nicholas-esterer nicholas-esterer self-requested a review

July 14, 2020 15:51

nicholas-esterer

nicholas-esterer reviewed

Copy link

Contributor

@nicholas-esterer nicholas-esterer left a comment

Choose a reason for hiding this comment

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

just some typos, otherwise very slick 👍

doc/python/static-image-export.md Outdated

##### conda

Using the [conda](https://conda.io/docs/) package manager, you can install these dependencies in a single command:

#### Install Dependencies (Kaleido)

Static image generation requires the [`kaleido`](https://github.com/plotly/Kaleido) package which can be installed using pip...

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

doc/python/static-image-export.md Outdated

##### conda

Using the [conda](https://conda.io/docs/) package manager, you can install these dependencies in a single command:

#### Install Dependencies (Kaleido)

Static image generation requires the [`kaleido`](https://github.com/plotly/Kaleido) package which can be installed using pip...

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

I can't select it but on line 40 JEPG should be JPEG.

doc/python/static-image-export.md

```

### Create a Figure

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

line 61 should be "Now let's create a simple scatter plot with 100 random points of varying color and size."

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

Fixed in 70c11ac

doc/python/static-image-export.md Outdated Show resolved Hide resolved

doc/python/orca-management.md Outdated Show resolved Hide resolved

doc/python/orca-management.md Outdated

### Create a Figure

Now let's create a simple scatter plot with 100 random points of variying color and size.

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

Suggested change

Now let's create a simple scatter plot with 100 random points of variying color and size.

Now let's create a simple scatter plot with 100 random points of varying color and size.

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

done

doc/python/orca-management.md

- **`topojson`**: Location of the topojson files needed to render choropleth traces. Defaults to a CDN location. If fully offline export is required, set this to a local directory containing the [Plotly.js topojson files](https://github.com/plotly/plotly.js/tree/master/dist/topojson).

- **`mapbox_access_token`**: Mapbox access token required to render `scattermapbox` traces.

- **`use_xvfb`**: Whether to call orca using [Xvfb](https://www.x.org/releases/X11R7.6/doc/man/man1/Xvfb.1.xhtml) on Linux. Xvfb is needed for orca to work in a Linux environment if an X11 display server is not available. By default, plotly.py will automatically use Xvfb if it is installed, and no active X11 display server is detected. This can be set to `True` to force the use of Xvfb, or it can be set to `False` to disable the use of Xvfb.

Copy link

Contributor

@nicholas-esterer nicholas-esterer Jul 14, 2020

Choose a reason for hiding this comment

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

line 253 should be - **default_scale**: The default image scale factor applied on image export.

Copy link

Contributor Author

@jonmmease jonmmease Jul 14, 2020

Choose a reason for hiding this comment

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

Fixed in 8731ecfe3402002d1eac53e74e7d7ce821b2c821

jonmmease and others added 10 commits

July 14, 2020 15:08


 in README: indicate that Keleido is new and improved and orca is legacy

00987c0


 Add Kaleido note to orca-management section

a25f5b2


 JPEG typo

a033df0


 Merge "Install Dependency" sections and better explain that Kaleido i...

d1fc9a8

...s new and recommended


 Replace Orca with plotly.py when discussing supported image export fo...

2bdcfe2

...rmats


 varying type

70c11ac


 factor typo [ci skip]

e350025

@jonmmease @nicolaskruchten


 Update CHANGELOG.md

967b728

Co-authored-by: Nicolas Kruchten <nicolas@plot.ly>

@jonmmease @nicholas-esterer


 Update doc/python/static-image-export.md

81a0bc7

Co-authored-by: NickE <64649603+nicholas-esterer@users.noreply.github.com>

@jonmmease @nicholas-esterer


 Update doc/python/orca-management.md

74c4274

Co-authored-by: NickE <64649603+nicholas-esterer@users.noreply.github.com>

Copy link

Contributor Author

jonmmease commented Jul 14, 2020

Thanks for the reviews @nicholas-esterer and @nicolaskruchten, updates applied. Merging!

@jonmmease jonmmease merged commit 25a77f3 into master