Emscripten wheel fixes and test coverage #2739

tomjakubowski · 2024-09-06T03:18:34Z

Fix Emscripten wheel build to better support use of Emscripten -fexceptions
Add pyodide virtual package, selectable from pnpm run setup, which builds perspective-python with Pyodide environment flags set
Add pyodide-pytest smoke tests for the Emscripten wheel, which run pytests in pyodide in Chrome using playwright, locally and in CI
Add convenient vscode snippets for copyright notice in # and // flavors
Add build helper functions to scripts for various important build paths, directories, and configs
Replace old-style import asserts for reading package.json with new helpers, which improves forwards Node compatibility

tomjakubowski · 2024-09-10T00:37:05Z

tools/perspective-scripts/workspace.mjs

+    return rustWheelsDir;
+}
+export function getEmscriptenWheelPath() {
+    const pspVersion = getWorkspacePackageJson().version;


I'm not sure, is the workspace package.json an OK place to get the version of the emscripten wheel? Or should this read from rust/perspective-python/Cargo.toml? If they're meant to be kept in sync, it shouldn't matter, right?

repack_wheel uses the version from pacakge.json to construct the version number for the .data directory, so seems reasonable to use it here too

huh, interesting. maybe should do like perspective-python's JS build script, and read it from perspective-python's package.json

https://github.com/finos/perspective/blob/master/rust/perspective-python/build.mjs#L28-L30

edit: fixed link

We update all package.json files at once, so the "local" one is the right place to fethc it generally, except the scripts in perspective-scripts don't live in a module, so the workspace package.json is the only option.

tomjakubowski · 2024-09-10T00:37:56Z

tools/perspective-scripts/workspace.mjs

+}
+export function getEmscriptenWheelPath() {
+    const pspVersion = getWorkspacePackageJson().version;
+    const wheeljunk = "cp39-abi3-emscripten_3_1_58_wasm32";


not sure if there's a good way to compute this (from the pyproject.toml? or Cargo.toml?)

it's probably OK to leave it like this, and just adjust it whenever it breaks; when we change ABIs or emscripten versions or whatever

Actually, on second thought, we could compute the emscripten version at least, since that's in pacakge.json. Although that may be short lived; newer pyodide-build wheels don't have the emscripten version in them, instead it's like pyodide_2024_0_wasm32; it's unclear to me if they'll be dropping support for wheels named with the emscripten version in them, but they do work currently w/ 0.26.2

IMO we get this by listing any .whl target with "emscripten" in the triple name - and if there are two (or more), error with a message instructing the dev to run pnpm run clean.

tomjakubowski · 2024-09-10T00:42:21Z

tools/perspective-scripts/install_pyodide.mjs

+    }
+}
+
+downloadPyodide();


was looking for an equivalent to if __name__ == "__main__" but the closest I could find appears to be undergoing active bikeshedding nodejs/node#49440

require.main doesn't work in Node esm

There's no easy way to do this, but every script in this folder should be considered a "main" script, that's what the folder is for. There are a handful of exceptions already e.g. sh.mjs.

tools/perspective-scripts/repack_wheel.mjs

Switch from using CMAKE_CXX_FLAGS, which has confusing and global semantics, to calling target_compile_options on all the targets This confines all the pyodide exception build code to a single branch of the cmake file Remove some commented-out code and dead branches in the build. I think this helps with reading and scanning the Cmake files Signed-off-by: Tom Jakubowski <tom@prospective.dev>

This fixes a nasty warning on installing the wheel. It could likely be fixed another way, maybe with linker flags, but we decided to cut the feature since it's not of main importance Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

This makes our wheel compatible with widgetsnbextension 4.0.11, which is the version provided by jupyterlite-pyodide-kernel 0.4.1 Signed-off-by: Tom Jakubowski <tom@prospective.dev>

In setup, this package selects `perspective-python` and sets some environment variables to make the build run for Pyodide. Co-authored-by: Andrew Stein <steinlink@gmail.com> Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Integration/smoke tests for the Pyodide wheel, which run for the perspective-pyodide virtual package and in the Pyodide CI actions. Some changes which support installing the wheels in a pytest-pyodide environment: - Defer some third-party imports to "run time" by moving them deeper than module scope - Add try/except guards to widget re-exports so that wheel can be used without Jupyter being installed - Move Jupyter deps in pyproject.toml to an optional-dependencies group Signed-off-by: Tom Jakubowski <tom@prospective.dev>

This makes the emscripten wheel build on normal PRs Signed-off-by: Tom Jakubowski <tom@prospective.dev>

This `import assert` syntax is experimental, and was removed in newer Node versions in favor of with `import ... with`. some node versions (including Node 18) support both syntaxes Unsure what minimum node version we need to support - if we can restrict to Node 18+, then we could instead replace this with `import ... with` Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

(Remove accidentally included debug log) Signed-off-by: Tom Jakubowski <tom@prospective.dev>

texodus

Thanks for the PR! Looks good!

This is a lot of commits for a cleanup PR but I will let it slide this time because the commits are descriptive and additive! (e.g. no "WIP reverting previous commit" or the like).

texodus · 2024-09-15T02:48:52Z

rust/perspective-python/package.json

    },
    "devDependencies": {
+        "@finos/perspective-scripts": "workspace:*",


Posthumous review update -

Packages in the tools/ directory are private to the source repository and they can't be included as dependencies in the published packages, even if they are just dev dependencies, because these tools ultimate are intended to depend on Perspective also and you end up with circular imports.

Makes sense. I didn't know that we don't publish @finos/perspective-scripts to NPM

tomjakubowski force-pushed the tom-pyodide-exceptions branch 5 times, most recently from 7c68aa5 to a7d22de Compare September 9, 2024 23:11

tomjakubowski commented Sep 10, 2024

View reviewed changes

tools/perspective-scripts/repack_wheel.mjs Outdated Show resolved Hide resolved

tomjakubowski changed the title ~~Draft: Emscripten wheel fixes~~ Emscripten wheel fixes Sep 10, 2024

tomjakubowski marked this pull request as ready for review September 10, 2024 02:12

tomjakubowski force-pushed the tom-pyodide-exceptions branch from e28b2c9 to 1497a50 Compare September 10, 2024 18:11

tomjakubowski and others added 10 commits September 10, 2024 11:17

Hide psp_stack_trace, psp_heap_size from pyodide

09d7221

This fixes a nasty warning on installing the wheel. It could likely be fixed another way, maybe with linker flags, but we decided to cut the feature since it's not of main importance Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Add option to make builds much more verbose

97eb93f

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Delete more dead build code

1e4bd90

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Constrain ipywidgets dep

6078daf

This makes our wheel compatible with widgetsnbextension 4.0.11, which is the version provided by jupyterlite-pyodide-kernel 0.4.1 Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Add pyodide virtual package

72212ce

In setup, this package selects `perspective-python` and sets some environment variables to make the build run for Pyodide. Co-authored-by: Andrew Stein <steinlink@gmail.com> Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Re-enable building emscripten wheel off-tag

13faa99

This makes the emscripten wheel build on normal PRs Signed-off-by: Tom Jakubowski <tom@prospective.dev>

Add vscode snippets for copyright notices

25158fd

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

tomjakubowski force-pushed the tom-pyodide-exceptions branch from 1497a50 to 25158fd Compare September 10, 2024 18:19

tomjakubowski mentioned this pull request Sep 10, 2024

Remove required dependency on ipywidgets, defer import of ipywidgets/PerspectiveWidget #2728

Closed

OOOOOOHOOW DDD

45c8af0

(Remove accidentally included debug log) Signed-off-by: Tom Jakubowski <tom@prospective.dev>

tomjakubowski mentioned this pull request Sep 11, 2024

OOOOOOHOOW DDD #2742

Closed

texodus added the internal Internal refactoring and code quality improvement label Sep 14, 2024

texodus reviewed Sep 14, 2024

View reviewed changes

texodus approved these changes Sep 14, 2024

View reviewed changes

texodus merged commit fc554a9 into finos:master Sep 14, 2024
10 checks passed

texodus changed the title ~~Emscripten wheel fixes~~ Emscripten wheel fixes and test coverage Sep 14, 2024

texodus reviewed Sep 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emscripten wheel fixes and test coverage #2739

Emscripten wheel fixes and test coverage #2739

tomjakubowski commented Sep 6, 2024 •

edited

Loading

tomjakubowski Sep 10, 2024 •

edited

Loading

tomjakubowski Sep 10, 2024

tomjakubowski Sep 10, 2024 •

edited

Loading

texodus Sep 14, 2024

tomjakubowski Sep 10, 2024

tomjakubowski Sep 10, 2024

texodus Sep 14, 2024

tomjakubowski Sep 10, 2024

texodus Sep 14, 2024

texodus left a comment

texodus Sep 15, 2024

tomjakubowski Sep 25, 2024

Emscripten wheel fixes and test coverage #2739

Emscripten wheel fixes and test coverage #2739

Conversation

tomjakubowski commented Sep 6, 2024 • edited Loading

tomjakubowski Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjakubowski Sep 10, 2024 • edited Loading

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

texodus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjakubowski commented Sep 6, 2024 •

edited

Loading

tomjakubowski Sep 10, 2024 •

edited

Loading

tomjakubowski Sep 10, 2024 •

edited

Loading