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

Integrate vl-convert for saving to svg and png #2701

Merged
merged 9 commits into from
Nov 1, 2022

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Oct 22, 2022

This PR adds support for saving Altair charts to SVG and PNG static images using the VlConvert Python package.

See #2662 (opened by @mcp292) for some background on the need for a simpler method for exporting charts as static images.

See https://medium.com/@jonmmease/introducing-vlconvert-c763f0076e89 for background on how VlConvert works, and how it is able to avoid the need for external dependencies like selenium or Node.js.

Example:

$ pip install vl-convert-python
import altair as alt
from vega_datasets import data

chart = alt.Chart(data.cars.url).mark_point().encode(
    x='Horsepower:Q',
    y='Miles_per_Gallon:Q',
    color='Origin:N'
)

chart.save('chart.svg')
chart.save('chart.png', scale_factor=2)

Questions:

  • Should this integration be added directly in Altair like this, or in Altair saver?
  • Should VlConvert take precedence over Altair Saver if they are both installed?

cc @joelostblom @mattijn @daylinmorgan

@joelostblom
Copy link
Contributor

This is exciting! My thoughts on your two questions:

  1. At first glance, I think it makes sense to integrate this here and not rely on the altair saver package. Especially if we think that vl-convert can replace altair saver over time and offer the same functionality with less hassle. On that topic, do you see PDF support being added to vl-convert or is that unfeasible due to some technical limitation (tbh, I don't see much of a use for PDF when svg is working fine, but since altair saver supports it, I thought I would ask)?
  2. If the vl-convert repo becomes adapted as part of the vega organization and it offers a solution that means less issues for end users compared to altair_saver, then I think it makes sense that it takes precedence over altair saver when both are installed.

@jonmmease
Copy link
Contributor Author

Thanks for the notes @joelostblom.

  1. I don't yet see a clear path to dependency free PDF export. There are a few options that could move us in that direction, but none that I'm fully happy with yet. I'll try to open an issue in the VlConvert project soon with some notes on potential approaches. I'd also be interested to understand the main use cases for PDF export. From my POV, the most important thing is having high-quality raster (PNG) and vector (SVG) options, and then people can use tools like Inkscape to edit and convert them to other specific formats (PDF, EPS, EMF, etc.).
  2. VlConvert has already been moved to the vega org! See https://github.com/vega/vl-convert

@joelostblom
Copy link
Contributor

  1. From my POV, the most important thing is having high-quality raster (PNG) and vector (SVG) options, and then people can use tools like Inkscape to edit and convert them to other specific formats (PDF, EPS, EMF, etc.).

I agree with you and personally I don't see much of a use for PDF when svg is working fine. I imagine that there might be some people relying on PDF export in altair since it still is a more familiar format, so we would just need to include a note in the docs/changelog about how to convert SVG to PDF or how to use SVG directly when in need of vector graphics for those not familiar with it (I'm sure there is some existing resource to link to). Especially if we decide to replace altair_saver with vl-convert as the recommend way to save images, which would have my vote based on what I have seen so far and that the repo is now officially part of the Vega org.

@daylinmorgan
Copy link
Contributor

I agree with @joelostblom that this should not rely on altair_saver and should also take precedence over it.

@jonmmease
Copy link
Contributor Author

Ok, thanks for the feedback @joelostblom and @daylinmorgan! I'll fix the lint issue and add some tests soon. In the meantime, let me know if you have feedback on the patch itself.

@jonmmease
Copy link
Contributor Author

To make testing easier, I added an engine kwarg to save/spec_to_mimebundle that can be set to either 'vl-convert' or 'altair_saver'. This way you can toggle between them when both are installed.

It defaults to 'vl-convert' if vl-convert-python is installed and the format is not pdf. Otherwise defaults to 'altair_saver' if altair_saver is installed.

If this approach looks good, I'll update the documentation

@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2022

Hi @jonmmease, nice work! I've a question. I saw this comment in another repo deeplook/svglib#207 (comment).

It uses another approach to convert svg into png. It goes from svg to pdf and then from pdf to png.. Surely cumbersome, but it also has a few things that might be interesting here.

  1. PDF output.
  2. A familiar dpi parameter instead of a scale_factor.
  3. scaling and transparency included (no idea if this an issue here)
  4. It takes less time to save as a png with high scale_factor, which apparently is a bottleneck in resvg-js.

There are also disadvantages, which are working correctly with vl-convert. For example, this issue altair-viz/altair_saver#110 displays correctly as png with vl-convert, but not with the method described above.

In no means I'm saying we should use that option, but are there things in there that we can benefit from?

@daylinmorgan
Copy link
Contributor

Svglib (becasue of reportlab) also doesn't support gradients. daylinmorgan/okab#3 (comment)

@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2022

Lots of excellent info, thanks for sharing @daylinmorgan!

@jonmmease
Copy link
Contributor Author

Thanks for the notes @mattijn,

I've played with svglib+reportlab a bit as an approach to generate PDFs. It worked well for simple figures, but I didn't pursue it further what I saw that it doesn't support gradients.

This may be another good reason to introduce the engine argument as I do in this PR. In the future we could add a 'svglib' engine that would use vl-convert to generate an svg with proper text layout and then svglib+reportlab to generate a PDF, and svglib+reportlab+mupdf to generate a PNG. And the 'svglib' engine could support a dpi argument.

@jonmmease
Copy link
Contributor Author

Another option, that I'm totally fine with, would be to only expose the svg image type to start with. This way we could do some more research on PNG approaches before endorsing one as the default.

@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2022

I'm fine with the current approach. No need to expose svg-only. Thanks for the clarifications!

@jonmmease
Copy link
Contributor Author

I fixed the flake8 lint and updated the image export documentation to include info on vl-convert and the new engine argument to Chart.save.

This is good to go from my side, let me know if there's anything else you'd like to see done here before merging.

@jonmmease jonmmease changed the title Integrate vl-convert-python for saving to svg and png Integrate vl-convert for saving to svg and png Oct 26, 2022
@jonmmease
Copy link
Contributor Author

One thing to note, in 925ca3e I removed the installation of vl-convert in the documentation CI job. It works fine for every example except for the isotype_emoji, because emoji aren't supported by resvg (which vl-convert relies on).

@daylinmorgan
Copy link
Contributor

Do the docs currently use PNGs for the static figures?

@jonmmease
Copy link
Contributor Author

The gallery thumbnails are static images

@mattijn
Copy link
Contributor

mattijn commented Oct 29, 2022

Will the changes in this PR results that the example-specs are tested against both altair_saver and vl_convert (excluding the emoji-spec for vl_convert)? If so, nice. If not, can it?

@jonmmease
Copy link
Contributor Author

The test_vegalite_to_vega_mimebundle tests run on both vl-convert and altair_saver, but not image exports.

It looks like altair/examples/tests/test_examples.py converts all of the examples to PNG and checks that the resulting bytes start with the proper prefix, but they aren't compared to baseline images. I could easily update this test to check both altair_saver and vl-convert. But were you picturing having the test compare the exported images to baseline images?

The vl-convert test suite does this for a handful of vega-lite specifications, and we could certainly do that here as well if we're comfortable committing the collection of baseline PNGs to the repository.

@mattijn
Copy link
Contributor

mattijn commented Oct 29, 2022

If you can change the test_example.py to test against both altair_saver and vl-convert, that would be great. Currently I would stick to the current adopted approach to check for b'\x89PNG' only.
If you think it is beneficial to expand the test suite with a visual change comparison, then you might want to open a new issue for this.

@jonmmease
Copy link
Contributor Author

fa283a6 updates test_example.py to run against both altair_saver and vl-convert. Thanks for the suggestion @mattijn!

@mattijn
Copy link
Contributor

mattijn commented Oct 31, 2022

Thanks for the changes! Your branch is a bit behind with the main repo. Since this PR there have been 8 other commits that were merged. can you synchronise your branch with master?

@jonmmease
Copy link
Contributor Author

Branch updated in a163f8f

@mattijn
Copy link
Contributor

mattijn commented Nov 1, 2022

Thanks! All tests are passing. Merging. While this is in master, you still have sufficient time to work on vega/vl-convert#29 to make it conda/mamba installable. This is great, thanks again!

@joelostblom
Copy link
Contributor

@jonmmease I noticed that there another test where we should probably add vl-convert as an alternative to altair-saver. Would you be able to put up a PR changing this file to also include the appropriate vl-convert test and I can review?

https://github.com/altair-viz/altair/blob/68d197e8783db5a53a49a246fc2c2ce0f0205ef7/altair/vegalite/v5/tests/test_api.py#L256-L277

@jonmmease
Copy link
Contributor Author

sure!

@jonmmease
Copy link
Contributor Author

Done over in #2784

@jonmmease jonmmease deleted the vl_convert_export branch December 30, 2022 13:50
@mcp292
Copy link

mcp292 commented Jan 17, 2023

This is incredible! Thanks for all the hard work everyone! What release is this scheduled for?

@jonmmease
Copy link
Contributor Author

Thanks @mcp292, this will be included in Altair 5 (which doesn't have a firm release timeline unfortunately). But you can start using VlConvert directly in the meantime. See the Altair examples toward the end of https://github.com/vega/vl-convert/blob/main/vl-convert-python/notebooks/convert_vegalite.ipynb.

@mcp292
Copy link

mcp292 commented Jan 18, 2023

Thanks for that @jonmmease, and thank you especially for moving this improvement forward!

I noticed scale in the underlying command: vlc.vegalite_to_png(vl_spec=vl_spec, scale=2), so I'm assuming scale_factor is needed in chart.save() to provide this value. The function description mentions the default value as 1, but I don't see where that default is actually set.

When it is included in Altair 5, will scale_factor be a required parameter for saving PNGs or will there be a default value (chart.save('chart.png', scale_factor=2)chart.save('chart.png'))?


On a separate note, I found it interesting that the library is written in Rust. Is there a form of Altair for Rust? What does it replace pandas with?

@jonmmease
Copy link
Contributor Author

Hi @mcp292, the default of 1 for the scale parameter is applied in the Rust layer:

https://github.com/vega/vl-convert/blob/4d537e0df259c446171c359316726ffaa8b48cc3/vl-convert-rs/src/converter.rs#L673-L682

Scale is optional in vlc.vegalite_to_png and will be optional in chart.save (though for backward compatibility the argument to chart.save is named scale_factor). In terms of something like Altair for Rust, there is https://github.com/procyon-rs/vega_lite_4.rs, though I haven't used it yet.

@mcp292
Copy link

mcp292 commented Jan 18, 2023

That answers all my questions! Thanks again @jonmmease!

@bodacious-bill
Copy link

bodacious-bill commented Feb 21, 2023

Really cool to see an implementation that is fully installed from pypi!

I've finally gotten the node packages brought into my corporate firewalled VM environment for the node implementation of altair_saver, to find out the rendering is pretty slow compared to matplotlib. For my test example (large scatterplot) it was ~8s using altair_saver versus ~3s using matplotlib.

I can't find a whole lot of discussion on it, but it seems that re-svg is quicker at rendering than node-canvas. Has anyone done a speed comparison between this vl-convert implementation and altair_saver?

@jonmmease
Copy link
Contributor Author

Hi @bodacious-bill, unfortunately I don't think resvg is faster that node canvas for the case of scatter plots (it may even be a bit slower). This is listed as a limitation in the vl-convert README.

@bodacious-bill
Copy link

Thanks for the quick response! I suppose then I'll have to just stick to matplotlib for static chart generation.

archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jun 28, 2023
Alternative for saving PNG and SVG
vega/altair#2701
@mcp292
Copy link

mcp292 commented Jun 29, 2023

Updated to Altair 5, this is working perfectly!! Thank you all for the hard work and the clever approach! Works seamlessly.

@jonmmease
Copy link
Contributor Author

Thanks for taking the time to let us know @mcp292, that's great to hear!

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.

6 participants