New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Node package improvements #2403
Comments
I think |
I think it would be good to release an alpha first e.g., #2405 would also be good to include. |
I'm +1 with it |
Seems like this release could be made now? I'm keen to try it out. |
I made an alpha release @rth @hoodmane could you publish JS package to npm registry? I don't have access to our npm registry. |
Thanks @ryanking13 ! Just curious where did push 07be1f4 for that release, as I can't see it on main. If you send me your npm username, I'll add you. |
@rth I just pushed it. But... commit hashes are different. I think I am misunderstanding something about git cherry-pick.
It's ryanking13. |
Thanks! No worries, as long as it worked. I sent you the invite. |
I find out that I published the wrong stuff (TypeScript files, not JavaScript) in npm It looks like npm does not allow removing and republishing the same version. So I deprecated Sorry about the mistake. FYI, this happened because I called |
No, worries, let's push a pyodide@0.20.1-alpha.2 and I think you should be able to yank the other one. |
Pushed to It seems like unpublishing npm package is forbidden when there is a dependent package (npm/cli#1686 (comment)). |
Being able to make mistakes is the entire point of this issue! Thanks for making a release so that it can be tested. I tried it out and here's what I found:
|
Thanks a lot for testing it @alexmojaki . Maybe @mneil would be interested in having a look. |
I think the webpack issue is tricky. Ideally webpack should not try to bundle |
I have tried this with webpack and have not got it working the way I'd like yet enough to propose both the correct webpack config or changes to the distribution itself. When loading in the browser you need pyodide.js, pyodide.asm.js, pyodide.data, and the wasm file. It makes me wonder why these files are all separate in the first place? I can see holding off on wasm loading and keeping it separate for a worker but is there a reason the other files are separated? Those files assume loading via fetch which means a couple things in the browser...
Nodejs loading is all synchronous and has access to node modules. This isn't really an issue in node. But, the package itself could distribute files that make it easier to use in browser with no impact on node. |
These are separate files because Emscripten generates them that way. But we could switch to rolling up (or hopefully esbuilding soon) We really want |
Not the best for for bundle size but the data file could be bases64 encoded and inlined into the asm.js. THe benefit of that is loading / setup on the browser becomes simpler. Downside is you get about a 30% bump in filesize on the encoded file.
Are there specific concerns about this? One other issue I have right now with the pyodide.js file is that it forces the use of some globals. It is also checking for "IS_NODE" in a way that fails in webpack since process, fs, require, can all be mocked or replaced entirely causing pyodide.js to believe it is in node when it is in the browser. Bringing pyodide.asm.js and pyodide.js into a single file (and potentially encoding the data file as well into a single file) and removing the hard requirement of globals for the file system in particular would allow users like myself to choose their own filesystem replacement, their own fetch implementation, etc... |
Also an increase in load time and ram usage. See the Emscripten docs:
I don't think we are interested in going this route.
Emscripten tries to import ~4 different node modules only in node, not in browser. So these would need to be added to webpack externals, or we would need to patch Emscripten not to do this or... something? But also, it's 2.7 Mb of Javascript which has already been bundled and treeshaken and completely prepared for use by Emscripten, having webpack try to parse and optimize that again seems like it would be unnecessarily bad for build times. And it is completely possible webpack could mess things up. Another thing to consider is that we get better load times by splitting these files the way we do, because Also, splitting the files significantly improves the speed of partial rebuilds, particularly when only working on the Python source files or the Typescript source files and not the C code. rollup is really slow, though with esbuild it would be less of a problem. You may be able convince us, I am not very knowledgeable about the Javascript ecosystem.
We are interested in allowing those features, but I'm not sure the normal import system is the best way. We could for instance add a loading setting that tells Pyodide whether it is in the browser or not and overrides the guess. |
👍 explicit is better than implicit, yadda yadda: if |
oof! no, thanks! |
@hoodmane are you saying that errors like |
Yeah more or less. The other issue is that webpack thinks that node apis should be shimmed in the browser but most modules we load are node shims for browser apis. |
I've put together a sample webpack application that loads pyodide. I avoided processing the pyodide.js file at all as well as handled copying the files out of node modules into the distribution folder. This way minimizes modifying pyodide at all. One exception is that I deleted the globalThis.loadPyodide as it's not needed to have that global. I mentioned in #2549 that to make this work pyodide needs to exports all the files that it requires. I changed the package.json exports to:
I am using Example code, copying the files manually out of the node modules folder and not using webpack.
Error:
|
Well I think the issue is that we didn't deploy all the packages to npm to reduce download size -- we didn't want to deploy 200+ mb to npm. Try using: pyodide.loadPackage({indexURL : "https://cdn.jsdelivr.net/pyodide/v0.20.1a1/full/" }); |
I think that's what we want, though? So there is an issue in v0.20.1a1 that packages.json doesn't include all packages (while it should), however aside from that last time I tried to use it in node it does download and cache the packages But then again I have little experience with the JS packaging ecosystem, so if someone is sure there is no way around it, we can certainly change it. |
I just pulled the v20 off the cdn, to see how it resolves these and I'm realizing that it's not broken. It's that the other wheels that pyodide has internally are on the cdn and not in the node module. Once I've installed it locally and run it in the browser I'm not sure how to proceed. I am not using webpack when doing any of this. Just trying to use pyodide on my own page without specifying an indexURL. The indexURL is automatically determined. It knows I'm at http://localhost:9000 for example. It uses that url to load all the other files like pyodide.asm.data, the wasm, etc.... But these extra packages like micropip, or anything on this list https://pyodide.org/en/stable/usage/packages-in-pyodide.html are not accessible at that url. If I set indexURL to be the cdn prefix then I can get it to load those packages except that now it also tries to load all my other files from the cdn (like the wasm) - which is not what I would expect or want. If this is the solution then the node module only needs to have pyodide.js in it and no other files. As an end user this is confusing / difficult to manage. However, I also understand that all those "pyodide" packages are likely large and there were already concerns about the size of the application on npm. I'll think about how what I would want to happen vs. the constraints I can think of and try to enumerate them. Please help expand on this.
Publish pyodide specific modules to npmWe could publish each of the packages that pyodide loads with Add new configuration to loadPyodideindexURL should be reserved for the pyodide index and not used for loading core files like the asm.data, wasm, etc.... Add a new option to configure where the core files come from. By default it could be indexURL for backwards compatability. Takes care of 3,4,5 but still complicates 1 and 2 Document the need to download these individuallyRequire users download individual pyodide packages like micropip when using them with npm. Similar to "Publish pyodide specific modules to npm" except that the files stay on a cdn or similar. Document how to do this and or make it easy for users to get the files. This solves 1,2,3,4,5. I think this is the most complicated solution in terms of usability but it handles the most cases. Use pyodide as it is today, have a way to get it all locally easily and document the requirement. It's also low level of effort to implement. |
Thanks for the experiments and for opening a discussion @mneil. Maybe we need to use a separate issue for this conversation.
I was thinking of providing a simple command (#2402) to install wheel files from CDN, which can be done automatically or manually. I'm not sure how these downloaded packages can be easily shipped in bundlers though. |
Two issues still with 1.20.1-a2:
However, in reality if using ESM, one should import as below:
Note that the filename should be 'pyodide.mjs' not 'pyodide.js', and there's no default export from 'pyodide.mjs'
Code used is:
|
Thanks for the info about the docs being wrong. In node v16.14.0 I don't get import { loadPyodide } from "pyodide/pyodide.mjs";
async function main(){
let pyodide = await loadPyodide({"indexURL" : "./node_modules/pyodide"});
pyodide.runPython("print('1+1 is', 1+1)");
console.log(pyodide.version);
}
main(); prints:
|
For the same code, i get
Node version:
Oops... Sorry I didn't mention that I'm using an Intel version of node on on M1 mac... maybe that's an issue |
Hard to know. It is even possible that there could be a regression from v16.14.0 to v16.14.2 though this is probably even less likely. Do you have nvm? Do other versions of node have the problem too? |
We appreciate the bug report even if it isn't obvious how to reproduce it. |
Thanks I can confirm that with node arm version, it works as expected. It might well just be the issue with mixed m1, rosetta2, and nodejs/wasm. EDIT: the issue with the RuntimeError is also resolved on Intel. It's the issue with my own setup.
|
Sorry @hoodmane for pulling this issue back up out of the grave, but your "./node_modules/pyodide" option for the indexURL is something I spent literally hours trying to find.
My only problem is that that breaks when the application is bundled for deployment (the |
Well there's the https://github.com/pyodide/pyodide-webpack-plugin which I believe can bundle Pyodide. |
It would be good to release an alpha version with #2394 to help the node people test whether #2393 and related issues are fixed. We also have problems with webpack which probably aren't fixed, maybe we should try to improve the webpack situation first too.
I'm not sure how to version this stuff. I wouldn't normally want to include #2387 and #2391 in a patch release, but we could backport everything else. Or we could throw them into
0.20.1
they aren't humongous changes. Or we could plan to do a very smallv0.21.0
release.@alexmojaki @mneil @rth @ryanking13
The text was updated successfully, but these errors were encountered: