Add support for passing options to vega CLI utils via `vega_cli_options` kwarg #52

boydgreenfield · 2020-03-27T00:48:10Z

My primary use case is to be able to render interactive charts with multiple renderers (alt.renderers.enable("altair_saver", fmts=["html", "svg"]) without getting warnings about the window coming back and getting rendered into a notebook or other nbconvert export.

Closes #51. @jakevdp Would love your review and happy to tweak as needed – this was my quick and dirty approach to pass the arguments through, but if there's a more elegant way of doing so, please let me know!

…ns` kwarg

jakevdp

This looks good – a few comments.

One larger question: do the CLI commands all accept the same flags? Is a single list applied to all commands the best approach here?

altair_saver/_core.py

altair_saver/savers/_html.py

altair_saver/savers/_selenium.py

boydgreenfield · 2020-03-27T04:36:11Z

This looks good – a few comments.

One larger question: do the CLI commands all accept the same flags? Is a single list applied to all commands the best approach here?

They have the same interface, so I think it's worth keeping this simple? I don't really think you'd want to vary any of the params per renderer except for maybe --scale:

These are the options for vg2pdf, vg2svg, and vg2png

Options:
  -b, --base        Base directory for data loading. Defaults to the directory
                    of the input spec.                                  [string]
  -l, --loglevel    Level of log messages written to stderr. One of "error",
                    "warn" (default), "info", or "debug".               [string]
  -c, --config      Vega config object. Either a JSON file or a .js file that
                    exports the config object.                          [string]
  -f, --format      Number format locale descriptor. Either a JSON file or a .js
                    file that exports the locale object.                [string]
  -t, --timeFormat  Date/time format locale descriptor. Either a JSON file or a
                    .js file that exports the locale object.            [string]
  -s, --scale       Output resolution scale factor.        [number] [default: 1]
  --seed            Seed for random number generation.                  [number]
  --help            Show help                                          [boolean]
  --version         Show version number                                [boolean]

boydgreenfield · 2020-03-27T05:28:00Z

@jakevdp I fixed the typo (oops!) and added a commit to only pass vega_cli_options to NodeSaver. But that's causing a failure for tests on GH Actions (that pass locally on 3.7 for me and are frustrating my debugging). Maybe something about my renderers/environment config.

I don't really love this solution though. It seems like it'd be cleaner to pass along to NodeSaver only and have a subclassed __init__ method for it or keep it at the Saver level and pass it down (which I did originally just because that's how embed_options works and is not used in all of the Saver subclasses either).

jakevdp · 2020-03-27T05:56:58Z

Thanks – I think we should avoid special-casing for subclasses within the base class. Probably a better approach is to let the subclasses accept arbitrary kwargs and ignore any unrecognized/unsupported ones.

boydgreenfield · 2020-03-27T15:51:29Z

Thanks – I think we should avoid special-casing for subclasses within the base class. Probably a better approach is to let the subclasses accept arbitrary kwargs and ignore any unrecognized/unsupported ones.

OK, I've refactored to have all of the subclasses take a kwargs and then this is only being added to (1) _node.py for NodeSaver and the related vg2* functions and documented in the render and save methods in _core.py.

Let me know if this looks good!

jakevdp · 2020-03-27T17:17:41Z

Looks good! Last issue is to add some tests. I think we want two basic tests of this new behavior:

a new test in test_node.py that ensures that keywords passed to NodeSaver are properly propagated to the CLI functions
a new test in test_core.py that ensures that keywords passed to save() and render() make it to the NodeSaver

This will probably involve some mocking; if you're not familiar with mocking in pytest, search for monkeypatch within this repo to see a few examples of relevant approaches.

boydgreenfield · 2020-03-27T17:55:01Z

Looks good! Last issue is to add some tests. I think we want two basic tests of this new behavior:

a new test in test_node.py that ensures that keywords passed to NodeSaver are properly propagated to the CLI functions

a new test in test_core.py that ensures that keywords passed to save() and render() make it to the NodeSaver

This will probably involve some mocking; if you're not familiar with mocking in pytest, search for monkeypatch within this repo to see a few examples of relevant approaches.

Added some tests. Tried to mimic the style and mocking set up you have already, but let me know if they need tweaks (and also do feel free to just tweak directly if easier). A couple of the tests felt a little awkward/verbose but needed extra runtime checks because of MyPy.

jakevdp · 2020-03-27T18:22:52Z

Thanks - in the new test within test_core, it doesn't appear that you're validating that the cli_options argument is being correctly passed to NodeSaver: in other words, if save()/render() did nothing with that argument, the test would still pass.

altair_saver/savers/_node.py

boydgreenfield · 2020-03-27T21:59:36Z

Thanks - in the new test within test_core, it doesn't appear that you're validating that the cli_options argument is being correctly passed to NodeSaver: in other words, if save()/render() did nothing with that argument, the test would still pass.

These do shell out to vg2* – I've added a test to capture the stderr and ensure that, e.g., we get DEBUG logging when passing in the debug loglevel.

I think this addresses all of your comments @jakevdp. Let me know if you need anything else to merge.

jakevdp

A bunch of minor comments; once the tests are passing I think this is good to go!

Thanks so much for your work on this.

jakevdp · 2020-03-27T23:54:08Z

altair_saver/_core.py

@@ -162,7 +165,7 @@ def save(
        embed_options = alt.renderers.options.get("embed_options", None)

    Saver = _select_saver(method, mode=mode, fmt=fmt, fp=fp)
-    saver = Saver(spec, mode=mode, embed_options=embed_options, **kwargs)
+    saver = Saver(spec, mode=mode, embed_options=embed_options, **kwargs,)


remove the stray comma

jakevdp · 2020-03-27T23:54:22Z

altair_saver/_core.py

@@ -242,7 +248,7 @@ def render(

    for fmt in fmts:
        Saver = _select_saver(method, mode=mode, fmt=fmt)
-        saver = Saver(spec, mode=mode, embed_options=embed_options, **kwargs)
+        saver = Saver(spec, mode=mode, embed_options=embed_options, **kwargs,)


remove comma

jakevdp · 2020-03-27T23:55:01Z

altair_saver/savers/_node.py

@@ -69,6 +72,28 @@ class NodeSaver(Saver):
        "vega": ["pdf", "png", "svg"],
        "vega-lite": ["pdf", "png", "svg", "vega"],
    }
+    _vega_cli_options: List


jakevdp · 2020-03-27T23:56:04Z

altair_saver/savers/_node.py

+        embed_options: Optional[JSONDict] = None,
+        vega_version: str = alt.VEGA_VERSION,
+        vegalite_version: str = alt.VEGALITE_VERSION,
+        vegaembed_version: str = alt.VEGAEMBED_VERSION,


We can remove embed_options and the three versions, I believe, as they are not used by NodeSaver. kwargs will handle them if present.

jakevdp · 2020-03-27T23:56:20Z

altair_saver/savers/_node.py

+            embed_options=embed_options,
+            vega_version=vega_version,
+            vegalite_version=vegalite_version,
+            vegaembed_version=vegaembed_version,


pass kwargs

jakevdp · 2020-03-27T23:59:19Z

altair_saver/savers/_selenium.py

@@ -167,6 +167,7 @@ def __init__(
        webdriver: Optional[Union[str, WebDriver]] = None,
        offline: bool = True,
        scale_factor: Optional[float] = 1,
+        **kwargs,


pass kwargs to super(), below.

jakevdp · 2020-03-28T00:00:09Z

altair_saver/savers/tests/test_node.py

+@pytest.mark.parametrize("mode,fmt", get_modes_and_formats())
+@pytest.mark.parametrize("vega_cli_options", [None, ["--loglevel", "error"]])
+def test_node_mimebundle(
+    name: str, data: Any, mode: str, fmt: str, vega_cli_options: Optional[List]


Optional[List[str]]

jakevdp · 2020-03-28T00:01:19Z

altair_saver/tests/test_core.py

+@pytest.mark.parametrize("fmt", ["html", "svg"])
+@pytest.mark.parametrize("vega_cli_options", [None, ["--loglevel", "debug"]])
+def test_save_w_vega_cli_options(
+    capsys: Any, chart: alt.TopLevelMixin, fmt: str, vega_cli_options: Optional[List]


capsys: SysCapture (use from _pytest.capture import SysCapture)
vega_cli_options: Optional[List[str]]

jakevdp · 2020-03-28T00:02:22Z

altair_saver/tests/test_core.py

+
+@pytest.mark.parametrize("vega_cli_options", [None, ["--loglevel", "debug"]])
+def test_render_w_vega_cli_options(
+    capsys: Any, chart: alt.TopLevelMixin, vega_cli_options: Optional[List]


types, same as above.

jakevdp · 2020-03-28T00:03:03Z

altair_saver/tests/test_core.py

+    bundle = render(chart, fmts=["html", "svg"], vega_cli_options=vega_cli_options)
+    assert len(bundle) == 2
+    for mimetype, content in bundle.items():
+        fmt = mimetype_to_fmt(mimetype)


assert content is not None (for mypy)

boydgreenfield · 2020-03-28T17:27:35Z

@jakevdp All the above comments have been addressed and tests are passing. I think between this and #53, most of issues problems highlighted by #52 should be addressed or solvable via passing CLI args. Thanks again!

jakevdp · 2020-03-28T19:43:53Z

Awesome, thanks for the contribution!

Add support for passing options to vega CLI utils via `vega_cli_optio…

5cb9cfa

…ns` kwarg

jakevdp reviewed Mar 27, 2020

View reviewed changes

altair_saver/_core.py Outdated Show resolved Hide resolved

altair_saver/savers/_html.py Outdated Show resolved Hide resolved

altair_saver/savers/_selenium.py Outdated Show resolved Hide resolved

Review: Fix typo

ce89914

jakevdp mentioned this pull request Mar 27, 2020

Add option for passing additional logging options to vega (e.g., vg2svg) #51

Closed

jakevdp reviewed Mar 27, 2020

View reviewed changes

altair_saver/savers/_node.py Outdated Show resolved Hide resolved

Only pass vega_cli_options through to the NodeSaver, add tests

f189b7e

jakevdp mentioned this pull request Mar 27, 2020

Suppress window warnings in node #53

Merged

jakevdp reviewed Mar 28, 2020

View reviewed changes

Add further mypy types, clean up Saver kwargs passing

edae222

jakevdp merged commit fbcb7d4 into altair-viz:master Mar 28, 2020

boydgreenfield deleted the ng-pass-opts-to-vega branch March 29, 2020 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for passing options to vega CLI utils via `vega_cli_options` kwarg #52

Add support for passing options to vega CLI utils via `vega_cli_options` kwarg #52

boydgreenfield commented Mar 27, 2020

jakevdp left a comment

boydgreenfield commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020 •

edited

jakevdp commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020

jakevdp commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020

jakevdp commented Mar 27, 2020 •

edited

boydgreenfield commented Mar 27, 2020

jakevdp left a comment

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 27, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 28, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 28, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 28, 2020

boydgreenfield Mar 28, 2020

jakevdp Mar 28, 2020

boydgreenfield Mar 28, 2020

boydgreenfield commented Mar 28, 2020

jakevdp commented Mar 28, 2020

Add support for passing options to vega CLI utils via vega_cli_options kwarg #52

Add support for passing options to vega CLI utils via vega_cli_options kwarg #52

Conversation

boydgreenfield commented Mar 27, 2020

jakevdp left a comment

Choose a reason for hiding this comment

boydgreenfield commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020 • edited

jakevdp commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020

jakevdp commented Mar 27, 2020

boydgreenfield commented Mar 27, 2020

jakevdp commented Mar 27, 2020 • edited

boydgreenfield commented Mar 27, 2020

jakevdp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boydgreenfield commented Mar 28, 2020

jakevdp commented Mar 28, 2020

Add support for passing options to vega CLI utils via `vega_cli_options` kwarg #52

Add support for passing options to vega CLI utils via `vega_cli_options` kwarg #52

boydgreenfield commented Mar 27, 2020 •

edited

jakevdp commented Mar 27, 2020 •

edited