Clapack as so #1236

joemarshall · 2021-02-11T19:52:47Z

And support for shared-library packages in general.

So it doesn't have to be included multiple times in scipy (scipy is now 21mb)

It is mostly a surprisingly minor change, except that I had to change it so that it loads wasm asynchronously in even in firefox. Otherwise scipy couldn't link to clapack in firefox. It also loads shared library packages before anything else when you import, this is important because it means that linked in (as opposed to dlopen) shared library calls work fine. I couldn't work out why they didn't, but they didn't work in firefox.

There's a limitation to this which is that shared libraries that depend on other shared library packages may or may not work, I'm not sure...

Closes #473
Closes #240

…ple times.

dalcde · 2021-02-12T09:39:26Z

The next emscripten version allows us to use the built-in preload wasm mechanism instead of writing our own. It will also make it so that we don't have to traverse the entire filesystem every time. #1223

rth · 2021-02-12T12:01:44Z

The next emscripten version allows us to use the built-in preload wasm mechanism instead of writing our own.

Should we postpone this until the next emscripten release, which I guess will happen soon?
Otherwise this looks very promising indeed thanks @joemarshall !

joemarshall · 2021-02-12T13:01:54Z

Sounds like a plan, current preloading is a bit of a bodge.

…

-------------------- Note: I am currently part time, my working days are Wednesday to Friday. Responses may be delayed outside these times. On 12 Feb 2021 12:02, Roman Yurchak <notifications@github.com> wrote: The next emscripten version allows us to use the built-in preload wasm mechanism instead of writing our own. Should we postpone this until the next emscripten release, which I guess will happen soon? Otherwise this looks very promising indeed thanks @joemarshall<https://github.com/joemarshall> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1236 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAK6Y665WWUXNVGCR6EDDOTS6UKDRANCNFSM4XPODWTA>. This message and any attachment are intended solely for the addressee and may contain confidential information. If you have received this message in error, please contact the sender and delete the email and attachment. Any views or opinions expressed by the author of this email do not necessarily reflect the views of the University of Nottingham. Email communications with the University of Nottingham may be monitored where permitted by law.

…e wasm

joemarshall · 2021-02-12T16:33:31Z

Actually, it occurs to me that the thing I just did to make shared libraries in separate packages work - just overriding FS.open for the duration of the package load, could also be pretty trivially altered to remove the need to go through the whole file tree looking for anything added by the new package. Would save having to wait on the changes to emscripten (do they make preloading of wasm work with side modules, or with LZ4 or something?), and for side modules with shared libraries in, they need loadDynamicLibrary as well as instantiating the wasm, so I think the code will need to stay in or be added as a load plugin somehow or else shared lib modules won't work.

joemarshall · 2021-02-12T16:33:52Z

So maybe it is worth putting this in. I'm easy.

rth

It's true that it would be nice to have,

-rw-r--r-- 1 root root   21M Feb 12 16:51 scipy.data
-rw-r--r-- 1 root root  379K Feb 12 16:51 scipy.js

earlier rather than later. I'd also like to look into updating scipy version next, and this would likely be useful there. The changes with patching FS.open look manageable to me.

Could you please resolve merge conflicts?

rth · 2021-02-12T19:23:56Z

pyodide_build/pywasmcross.py

-            for lib_name in link_libs:
-                arg = os.path.join(lapack_dir, f"{lib_name}")
-                new_args.append(arg)
-


You can also remove the INLINING_LIMIT=5 and the -Os for CLAPACK below, that was an attempt to reduce its size when statically linking, that's no longer relevant.

packages/CLAPACK/meta.yaml

rth · 2021-02-12T19:58:20Z

src/pyodide.js

+    loadPackageChain = loadPackageChain.then(() => promise.catch(() => {}));
+    await promise;
+    Module.FS.open = oldOpen;
+    console.log("LIBS:", allLibs);


We should probably remove this debugging line, or at least use messageCallback if defined.

rth · 2021-02-23T14:44:01Z

We now use emscripten 2.0.14. @joemarshall Would you be able to merge master in and address the above comments? I can help if necessary.

joemarshall · 2021-02-24T07:22:02Z

Yeah can do

…

-------------------- Note: I am currently part time, my working days are Wednesday to Friday. Responses may be delayed outside these times. On 23 Feb 2021 14:44, Roman Yurchak <notifications@github.com> wrote: We now use emscripten 2.0.14. @joemarshall<https://github.com/joemarshall> Would you be able to merge master in and address the above comments? I can help if necessary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1236 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAK6Y65M3F6T3DDRK4ZJWADTAO5MRANCNFSM4XPODWTA>. This message and any attachment are intended solely for the addressee and may contain confidential information. If you have received this message in error, please contact the sender and delete the email and attachment. Any views or opinions expressed by the author of this email do not necessarily reflect the views of the University of Nottingham. Email communications with the University of Nottingham may be monitored where permitted by law.

…to master2

joemarshall · 2021-02-24T15:26:29Z

it had an HTTP error on build_packages, I think someone with better circleCI permissions than me needs to force rerun of the testws

rth

Two comments otherwise LGTM.

The next emscripten version allows us to use the built-in preload wasm mechanism instead of writing our own. It will also make it so that we don't have to traverse the entire filesystem every time.

and for side modules with shared libraries in, they need loadDynamicLibrary as well as instantiating the wasm, so I think the code will need to stay in or be added as a load plugin somehow or else shared lib modules won't work.

So if I understand correctly we still need the current code, and only using --use-preload-plugins (or calling emscripten_run_preload_plugins() on each file) by itself won't be enough? cc @dalcde Should we look into that in a separate PR after this one is merged?

rth · 2021-02-24T16:22:32Z

src/pyodide.js

+    // and means doing a long async operation in firefox,
+    // which can cause warning messages
+    await preloadWasm();
+    self.pyodide._module.reportUndefinedSymbols();


Suggested change

self.pyodide._module.reportUndefinedSymbols();

Module.reportUndefinedSymbols();

That was changed earlier in #1195

This still needs reverting..

src/pyodide.js

Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

joemarshall · 2021-02-24T17:19:39Z

I think preload plugins only compiles and instantiates the wasm, it doesn't do the stuff about bringing symbols from the wasm into the global namespace. It's why even if you use preload plugins you can't call into a module until you dlopen it or otherwise link it in.

(I.e. emscripten-core/emscripten#7220 )

joemarshall · 2021-02-24T17:22:52Z

Incidentally, it is pretty trivial to use the same logic I use here for detecting .so files in shared library modules and apply it to the other module loads, thus removing the need to scan the whole file system. I can put that in as a separate pr once this one is in.

dalcde · 2021-02-24T21:57:44Z

Can we write our own preload plugin to do this?

joemarshall · 2021-02-25T09:31:06Z

Now you mention it, that is obviously the right way to do this, can't believe I missed it.

…

-------------------- Note: I am currently part time, my working days are Wednesday to Friday. Responses may be delayed outside these times. On 24 Feb 2021 21:58, Dexter Chua <notifications@github.com> wrote: Can we write our own preload plugin to do this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1236 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAK6Y6ZJZPHTAIQMQP3JNILTAVY6LANCNFSM4XPODWTA>. This message and any attachment are intended solely for the addressee and may contain confidential information. If you have received this message in error, please contact the sender and delete the email and attachment. Any views or opinions expressed by the author of this email do not necessarily reflect the views of the University of Nottingham. Email communications with the University of Nottingham may be monitored where permitted by law.

…to master2

…into master2

joemarshall · 2021-02-25T16:42:50Z

hopefully the latest push works, now using preload_plugins for everything (with an override for the shared lib packages)

rth

Great! This does look much better indeed.

src/pyodide.js

rth · 2021-02-25T19:52:30Z

src/pyodide.js

+    // and means doing a long async operation in firefox,
+    // which can cause warning messages
+    await preloadWasm();
+    self.pyodide._module.reportUndefinedSymbols();


This still needs reverting..

src/pyodide.js

rth

One last thing I forgot: could you please add one (or in this case probably even two) changelog entries to docs/project/changelog.md. Thanks!

joemarshall · 2021-02-26T13:28:04Z

Changelog added

joemarshall added 4 commits February 11, 2021 13:42

Made clapack a shared library so it doesn't have to be included multi…

b38cba2

…ple times.

lint

e748160

made shared library packages work in firefox

37c8292

test fixes

d17f43a

joemarshall added 2 commits February 12, 2021 16:28

made shared library load load the library as well as instantiating th…

1283fcc

…e wasm

lint

721e578

rth reviewed Feb 12, 2021

View reviewed changes

joemarshall added 2 commits February 24, 2021 14:49

Merge branch 'master' of https://github.com/iodide-project/pyodide in…

85b5945

…to master2

review fixes

764481c

rth reviewed Feb 24, 2021

View reviewed changes

Fixed merging

9e492fd

Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

rth approved these changes Feb 24, 2021

View reviewed changes

joemarshall added 3 commits February 25, 2021 13:16

Merge branch 'master' of https://github.com/iodide-project/pyodide in…

bf46abd

…to master2

made it use the built in preload_plugins

5688808

Merge branch 'clapack_as_so' of https://github.com/joemarshall/pyodide …

1d61e15

…into master2

rth reviewed Feb 25, 2021

View reviewed changes

review fixes

3349928

rth approved these changes Feb 25, 2021

View reviewed changes

changelog

40a3d65

rth merged commit 451924b into pyodide:master Feb 26, 2021

joemarshall deleted the clapack_as_so branch February 26, 2021 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clapack as so #1236

Clapack as so #1236

joemarshall commented Feb 11, 2021 •

edited by rth

dalcde commented Feb 12, 2021 •

edited by phorward

rth commented Feb 12, 2021

joemarshall commented Feb 12, 2021 via email

joemarshall commented Feb 12, 2021

joemarshall commented Feb 12, 2021

rth left a comment •

edited

rth Feb 12, 2021

rth Feb 12, 2021

rth commented Feb 23, 2021

joemarshall commented Feb 24, 2021 via email

joemarshall commented Feb 24, 2021

rth left a comment

rth Feb 24, 2021

rth Feb 25, 2021

joemarshall commented Feb 24, 2021

joemarshall commented Feb 24, 2021

dalcde commented Feb 24, 2021 via email

joemarshall commented Feb 25, 2021 via email

joemarshall commented Feb 25, 2021

rth left a comment

rth Feb 25, 2021

rth left a comment •

edited

joemarshall commented Feb 26, 2021

	self.pyodide._module.reportUndefinedSymbols();
	Module.reportUndefinedSymbols();

Clapack as so #1236

Clapack as so #1236

Conversation

joemarshall commented Feb 11, 2021 • edited by rth

dalcde commented Feb 12, 2021 • edited by phorward

rth commented Feb 12, 2021

joemarshall commented Feb 12, 2021 via email

joemarshall commented Feb 12, 2021

joemarshall commented Feb 12, 2021

rth left a comment • edited

Choose a reason for hiding this comment

rth Feb 12, 2021

Choose a reason for hiding this comment

rth Feb 12, 2021

Choose a reason for hiding this comment

rth commented Feb 23, 2021

joemarshall commented Feb 24, 2021 via email

joemarshall commented Feb 24, 2021

rth left a comment

Choose a reason for hiding this comment

rth Feb 24, 2021

Choose a reason for hiding this comment

rth Feb 25, 2021

Choose a reason for hiding this comment

joemarshall commented Feb 24, 2021

joemarshall commented Feb 24, 2021

dalcde commented Feb 24, 2021 via email

joemarshall commented Feb 25, 2021 via email

joemarshall commented Feb 25, 2021

rth left a comment

Choose a reason for hiding this comment

rth Feb 25, 2021

Choose a reason for hiding this comment

rth left a comment • edited

Choose a reason for hiding this comment

joemarshall commented Feb 26, 2021

joemarshall commented Feb 11, 2021 •

edited by rth

dalcde commented Feb 12, 2021 •

edited by phorward

rth left a comment •

edited

rth left a comment •

edited