Update hephaestus with all the power it brings - #537
Conversation
teunbrand
left a comment
teunbrand
left a comment
There was a problem hiding this comment.
Partial review fussing over docs, will continue tomorrow
| # Stable is the default toolchain. It is *not* the MSRV: `rust-version` | ||
| # in /Cargo.toml is 1.86 (CRAN's), which is what points clippy's | ||
| # MSRV-aware lints at 1.86 — so running clippy here still guards it. | ||
| # The default cannot be 1.86 itself, because the default writers pull | ||
| # `parley`, which declares 1.88, and the `adbc` test path needs 1.88 | ||
| # for real. |
There was a problem hiding this comment.
Maybe we should have a claude.md at the workflow level? I feel like this comment about MSRVs is scattered throughout workflows and it might be better to have a single place to explain how we dance to CRAN's tunes
There was a problem hiding this comment.
Looks great in general. I haven't gone through the jupyter/vscode/wasm integrations.
One overarching thing that I noticed is with the ggsql viewand .hep output. The ggsql view window output fits the mould of the other writers so well that a separation into a separate CLI command feels like an extravagance. For the .hep output, I don't think (or may have missed) there is user-facing instruction on what to do with these files, in particular how to show them. We might be able to kill two birds with one stone if we could manage something like ggsql exec --writer=hep "..." > ggsql view where ggsql view is just a command to view a hep file, rather than a command that orchestrates a whole plot cycle.
|
I do think the window mode is a different thing than writing an output to somewhere. It may matter little now, but in an interactive future it retains the link to the running process allowing interactions - going through a .hep intermediary kinda breaks this link so I don't agree on your suggestion to make it a pipe I agree that .hep output feels a bit onesided at the moment and I'm not planning on promoting it heavily before the other side (the rendering) has been built out more. On the flip side I see no reason not to include it |
georgestagg
left a comment
georgestagg
left a comment
There was a problem hiding this comment.
The Jupyter and Wasm stuff looks broadly good to me!
I'd like to reorganise ggsql.js a little, and move it to TypeScript. I can either push a commit to make it a bit closer to what I'd like, or deal with it in a follow up PR, which would you prefer?
| //! # What the cold start actually costs | ||
| //! | ||
| //! Measured on a release build, Apple GPU, vello-hybrid: | ||
| //! | ||
| //! | | warm | cold (first process after a build) | | ||
| //! | --- | --- | --- | | ||
| //! | `RasterRenderer::new()` | 14 ms | ~185 ms | | ||
| //! | **first render** | **~85 ms** | **~1.35 s** | | ||
| //! | later renders, 3-point plot at 1200×800 | 5 ms | 5 ms | | ||
| //! | later renders, 50k points | ~200 ms | ~200 ms | | ||
| //! | ||
| //! The first render, not the renderer's construction, is the expensive part, | ||
| //! and most of it is parley/fontique loading system faces — a per-process cost. | ||
| //! Rendering an SVG first does the same text work with no GPU and drops the | ||
| //! first raster render from ~85 ms to ~20 ms. | ||
| //! | ||
| //! So the thread builds the renderer and renders a throwaway frame at startup, | ||
| //! before anyone is waiting. The renderer is `Send` but not `Sync`, which suits | ||
| //! a thread that owns it and never shares it. |
There was a problem hiding this comment.
Similar to other comments, I don't think we need this much detail with measured numbers, just a general explanation of why we render a throwaway frame at startup for warm up.
| # The raster plot formats, which need the wgpu stack at build time and a GPU | ||
| # adapter at render time. **Default**, because Positron's Plots pane asks for | ||
| # `png` and every shipped build enables it: a plain `cargo build` that quietly | ||
| # produced SVG-only plots differed from the released kernel in exactly the way | ||
| # that is hardest to notice from the outside. For a build without wgpu, use | ||
| # `--no-default-features --features all-readers`; plots then render as SVG, | ||
| # which needs no adapter. |
There was a problem hiding this comment.
| # The raster plot formats, which need the wgpu stack at build time and a GPU | |
| # adapter at render time. **Default**, because Positron's Plots pane asks for | |
| # `png` and every shipped build enables it: a plain `cargo build` that quietly | |
| # produced SVG-only plots differed from the released kernel in exactly the way | |
| # that is hardest to notice from the outside. For a build without wgpu, use | |
| # `--no-default-features --features all-readers`; plots then render as SVG, | |
| # which needs no adapter. | |
| # The raster plot formats, which need the wgpu stack at build time and a GPU | |
| # adapter at render time. **Default**, because Positron's Plots pane asks for | |
| # `png`. |
| if (sessionMode) { | ||
| // The enum's values are already the kernel's spelling (`console`, | ||
| // `notebook`, `background`), so there is nothing to translate. | ||
| argv.push('--session-mode', sessionMode); | ||
| } |
There was a problem hiding this comment.
This flag does not exist for versions of ggsql released before the beta release. Users with existing older ggsql installations won't be able to use them in Positron once the extension upgrades, until they upgrade their system installation of ggsql too.
Bundled ggsql should be fine, but for system installations is it possible to only pass this argument for ggsql kernel versions that support it?
|
I'm fine with you committing to this branch @georgestagg |
There was a problem hiding this comment.
Okay, I have pushed my changes and I am happy with the JS/Wasm side of things now (assuming CI passes).
I have reorganised how the ggsql-wasm package is built, and moved ggsql.js into the package source as ggsql.ts. With the changes you may want to try everything on your end again, rebuilding the Wasm bundles and re-running any local tests you have been doing.
3 participants
This PR updates the hephaestus dependency to 0.4.1 and provides all of the happiness that it brings. All the file types are supported out of the box, and we can now provide a proper kernel solution to rendering for both the plot pane and for notebooks