pyodide / matplotlib-pyodide

HTML5 backends for Matplotlib compatible with Pyodide
Mozilla Public License 2.0
30 stars 9 forks source link

matplotlib: savefig() produces TypeError: Invalid argument type in ToBigInt operation #8

Open ashley-hh opened 2 years ago

ashley-hh commented 2 years ago

Hello fellow pyodide users and mods!! Thank you for your hard work, this is an awesome project and I've been using it heavily!! Most recently, I've been trying to plot clustermaps from seaborn. This works amazingly on Chrome and somewhat well on Firefox, but not at all on Safari. This is the error that I've been getting when I call savefig() from the matplotlib library:

Screen Shot 2021-11-02 at 2 22 07 AM

Chasing down the most recent stack call, it's this function:

Screen Shot 2021-11-02 at 2 23 26 AM

Before I go even further down the rabbit hole to bisect this error, has anyone already encountered this and know how to get around it? Is this even the right place to ask this question? This is my first experience with cross-platform software, so any advice would be greatly appreciated. Thanks in advance, and hope y'all have a lovely day!!

ryanking13 commented 2 years ago

Thanks for the report, could you provide a minimal code example to reproduce this error?

hoodmane commented 2 years ago

We still don't fully support Safari: we don't run Safari tests in the continuous integration and I don't think any of the core devs use macs so we don't test things locally in Safari. I frequently do manual experiments in both Firefox and Chrome but not in Safari. So it may be a long time before issues like this are addressed.

rth commented 2 years ago

use macs so we don't test things locally in Safari

Actually once https://github.com/pyodide/pyodide/pull/1912 is resolved, we could potentially run Safari in CI. It adds more maintenance work, but then we do have recurrent requests to for better Safari support as well...

ashley-hh commented 2 years ago

Thanks for the report, could you provide a minimal code example to reproduce this error?

Thank you for your prompt response!! I believe this should do it:

Screen Shot 2021-11-02 at 3 01 06 PM

I'm using Version 14.1.2 of Safari.

rth commented 2 years ago

Thanks @ashley-hh ! Could you please copy paste it as a code snippet so that we wouldn't have to re-type it manually?

ashley-hh commented 2 years ago

Oh, of course! My bad -- here you go!

const script = `
import micropip
import numpy as np
import os
os.environ['MPLBACKEND'] = 'AGG'
import io, base64
import sys
sys.setrecursionlimit(1000)
await micropip.install('seaborn==0.9.0')
import seaborn as sns

data = np.asarray([[1, 0, .5], [1, 0, .5], [1, 0, .5]])
fig = sns.clustermap(data)

buf = io.BytesIO()
fig.savefig(buf, format='png')`

await self.pyodide.loadPackagesFromImports(script);
const res = await self.pyodide.runPythonAsync(script);
rth commented 2 years ago

Thanks! I can reproduce on Safari 14.1. As far as I can tell the error happens when calling sns.clustermap, even without the savefig part.

BTW, in Safari 13.1 I get,

Error: WebAssembly function with an i64 argument can't be called from JavaScript (
      evaluating `dynCall("viiii", index, [a1, a2, a3, a4])`

The error is likely different because 14.1 got BigInt support. So far not much ideas of how to fix it, but clearly some argument is not being converted while it should be, since we are building without WASM_BIGINT as far as I know.

hoodmane commented 2 years ago

The flag WASM_BIGINT says that BigUint64Array and BigInt64Array exist. Emscripten always assumes that BigInt exists.

rth commented 2 years ago

@hoodmane Are you sure? https://emscripten.org/docs/getting_started/FAQ.html#how-do-i-pass-int64-t-and-uint64-t-values-from-js-into-wasm-functions doesn't sound like it.

hoodmane commented 2 years ago

I think so. For a function with an i64 argument, dynCall always gets a BigInt. But to load or store BigInt into the wasm memory without BigUint64Array requires some bitwise operations. See here in my libffi port:

https://github.com/hoodmane/libffi-emscripten/blob/1fd79801595a43e26b65b59030a9040ba124ee24/src/wasm32/ffi.c#L51]

Both with and without WASM_BIGINT, we are expected to handle BigInt but without it HEAPU64 doesn't exist. Without BigInt support at all, it's just impossible to call functions with an i64 argument from Javascript as the error message says.

hoodmane commented 2 years ago

It's funny, grepping around the emscripten source, I don't find much evidence for my claim that WASM_BIGINT works this way, but the libffi tests pass when built without WASM_BIGINT so that makes me think it's probably true anyways.