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

Add Kaleido image export support #2613

Merged
merged 31 commits into from
Jul 14, 2020
Merged

Add Kaleido image export support #2613

merged 31 commits into from
Jul 14, 2020

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Jul 3, 2020

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
Copy link
Contributor Author

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.

@nicolaskruchten
Copy link
Contributor

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

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.

@jonmmease
Copy link
Contributor Author

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.

@jonmmease
Copy link
Contributor Author

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 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
@jonmmease
Copy link
Contributor Author

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
Copy link
Contributor Author

@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.

@jonmmease
Copy link
Contributor Author

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?

@nicolaskruchten
Copy link
Contributor

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

@nicolaskruchten
Copy link
Contributor

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?

@nicolaskruchten
Copy link
Contributor

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 :)

@jonmmease
Copy link
Contributor Author

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.

@nicolaskruchten
Copy link
Contributor

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
Copy link
Contributor Author

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 jonmmease changed the title [WIP] Add Kaleido image export preview behind the kaleido_export future flag Add Kaleido image export support Jul 13, 2020
@jonmmease
Copy link
Contributor Author

@nicholas-esterer @emmanuelle @nicholas-esterer

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

README.md Outdated

### Static Image Export with Orca

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

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

Choose a reason for hiding this comment

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

Update in 00987c0

@@ -34,13 +34,65 @@ jupyter:
---

### 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

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

Choose a reason for hiding this comment

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

Update in a25f5b2

##### 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

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

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

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

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)

CHANGELOG.md Outdated Show resolved Hide resolved
@nicolaskruchten
Copy link
Contributor

💃 modulo the documentation feedback

@nicholas-esterer nicholas-esterer self-requested a review July 14, 2020 15:51
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 👍

##### 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

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, ...".

##### 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

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.


```
$ pip install psutil requests
```
<!-- #endregion -->

### Create a Figure
Copy link
Contributor

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

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
files.download('fig1.svg')
files.download('fig1.png')
```
<!-- #endregion -->

### Create a Figure
Now let's create a simple scatter plot with 100 random points of variying color and size.
Copy link
Contributor

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

Choose a reason for hiding this comment

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

done

@@ -207,4 +259,4 @@ In addition to the `executable` property, the `plotly.io.orca.config` object can

Copy link
Contributor

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

Choose a reason for hiding this comment

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

Fixed in 8731ecfe3402002d1eac53e74e7d7ce821b2c821

@jonmmease
Copy link
Contributor Author

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

@jonmmease jonmmease merged commit 25a77f3 into master Jul 14, 2020
@nicolaskruchten nicolaskruchten deleted the kaleido branch July 20, 2020 14:35
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

4 participants