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

Support saving to PDF with VlConvert 1.0 #3244

Merged
merged 8 commits into from
Nov 3, 2023
Merged

Conversation

jonmmease
Copy link
Contributor

VlConvert 1.0 was just released and it includes support for saving charts to PDF. This PR bumps the minimum version of vl-convert-python to 1.0.0 and adds support for saving to PDF with the vl-convert engine.

This brings vl-convert to feature parity with altair_saver, so I added a deprecation warning when the altair_saver engine is used. I'm thinking we could drop altair_saver support in Altair 5.3.0.

@mattijn
Copy link
Contributor

mattijn commented Oct 30, 2023

Thanks for this PR @jonmmease, quite awesome to see all of this progress around vl-convert!
I run into a few issues with this PR. When I run:

import altair as alt
import pandas as pd

source = pd.DataFrame({
    'a': ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I'],
    'b': [28, 55, 43, 91, 81, 53, 19, 87, 52],
})

chart = alt.Chart(source).mark_bar().encode(x='a', y='b')
chart.save('chart.pdf')

I get the following error:

ValueError: Saving charts in 'pdf' format requires the altair_saver package: see http://github.com/altair-viz/altair_saver/

Which surprises me. I was expecting an error coming from the import_vl_convert:

alt.utils._importers.import_vl_convert()
ImportError: The vl-convert-python package must be version 1.0.0 or greater. Found version 0.14.0

But if I do

import vl_convert as vlc
print(vlc.__version__)
vlc.vegalite_to_pdf(chart.to_json())

I get a successful pdf:

0.14.0
b'%PDF-1.7...'

We should probably be raising an importlib.metadata.PackageNotFoundError around here and there, so this ImportError of a too low package version is properly raised and not caught in the try..except clause.

And based on the vl-convert v0.14.0 release page, I'm wondering if the bump to vl-convert 1.0 is already required part of this PR?

@mattijn
Copy link
Contributor

mattijn commented Nov 1, 2023

I added a commit: 07ae53a. I think a RuntimeError is probably better over a ImportError when a version of a soft dependency is detected which is too low. This will presents the correct error with above example as well.

@jonmmease
Copy link
Contributor Author

Thanks for the review and for trying it out @mattijn! I'm happy with change you made. Are you ready for this to be merged?

And based on the vl-convert v0.14.0 release page, I'm wondering if the bump to vl-convert 1.0 is already required part of this PR?

Yeah, for PDF vl-convert-python 0.14.0 would be ok (though there was at least one PDF bug fix since 0.14.0), but I want to open a PR with offline HTML support after this and that will require 1.0.0.

@mattijn
Copy link
Contributor

mattijn commented Nov 3, 2023

Yes, fine with merging. In a follow up PR will also try to improve the current error message if a too low version is detected. I like the part that you get some info how to upgrade using pip or conda, but currently you only get to see this if you don't have the soft dependency installed. Also mentioning that it is a soft dependency is helpful I think.
Thanks @jonmmease! Looking forward to offline support🥳

@mattijn mattijn merged commit e5fb1f0 into main Nov 3, 2023
20 checks passed
This was referenced Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Static image export
Development

Successfully merging this pull request may close these issues.

2 participants