-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
mpy-cross: WASM target and Python wheel. #11723
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
base: master
Are you sure you want to change the base?
Conversation
Code size report:
|
We've been running mpy-cross in the browser for several years now for Pybricks. For reference, here is how we are doing it: https://github.com/pybricks/pybricks-micropython/tree/master/npm/mpy-cross
Even if it raises a |
I did wonder if maybe you had because I saw your commit 115acad but wasn't sure if that was an experiment or not. Unfortunately I don't think this approach works for WASM, only for a .js output. With a plain WASM runtime we don't have a filesystem etc. (WASI would change that potentially though).
Yep. Functional NLR is only necessary if you want to be able to handle the exception and resume somewhere in the stack. In MicroPython, any exception in the compiler is always handled at the top level (in other words, the compiler doesn't use exception handling for program flow, only error handling). So in wasm-mpy-cross, the NLR depth was always 1 anyway, and the top level handler terminates the program, which means we can get away with just intercepting the error at the time it's raised. So if SyntaxError is raised, the nlr_jump will call the mpycross_error extern (provided by Python) with the formatted error output, which then terminates the WASM environment. |
Codecov Report
@@ Coverage Diff @@
## master #11723 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 158 158
Lines 20900 20900
=======================================
Hits 20563 20563
Misses 337 337
|
Added |
py/nlr.h
Outdated
@@ -89,8 +89,12 @@ | |||
#define MICROPY_NLR_MIPS (1) | |||
#define MICROPY_NLR_NUM_REGS (MICROPY_NLR_NUM_REGS_MIPS) | |||
#else | |||
#ifndef MICROPY_NLR_SETJMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get how this works. If you don't define MICROPY_NLR_SETJMP
then the check above for #if !MICROPY_NLR_SETJMP
should evaluate to true...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is to be able to say "I don't have support for setjmp" (by setting MICROPY_NLR_SETJMP to zero), in which case, if the arch-specific configuration cannot be done (because it falls through the if/elif), then don't try and enable setjmp.
So it works because like you say, the top-level if evaluates to true, but we need the final fall through to just set MICROPY_NLR_NUM_REGS to zero to disable NLR entirely.
I guess what we really need is a way for the port to convey that NLR is best-effort (i.e. if the arch-specific implementation is unavailable, it doesn't matter, but don't use setjmp). But really the better option would be to have a way to disable NLR. So I've done that instead.
mpy-cross/mpconfigport_main.h
Outdated
|
||
// Adds mp_raw_code_save_file as a helper to write .mpy files to the | ||
// filesystem. | ||
#define MICROPY_PERSISTENT_CODE_SAVE_FILE (1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this still needs to be:
#ifndef MICROPY_PERSISTENT_CODE_SAVE_FILE
#if defined(__i386__) || defined(__x86_64__) || defined(_WIN32) || defined(__unix__) || defined(__APPLE__)
#define MICROPY_PERSISTENT_CODE_SAVE_FILE (1)
#else
#define MICROPY_PERSISTENT_CODE_SAVE_FILE (0)
#endif
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see what you mean. My thinking here was that mpy-cross/main.c needs mp_raw_code_save_file
to exist, and that's what setting MICROPY_PERSISTENT_CODE_SAVE_FILE
does -- it provides the default implementation.
But it's weird to have non-embedded OS-specific code in py/ and it doesn't really make sense for mpy-cross's mpconfigport to know about the platforms that something in py/persistentcode.c works on.
So I think it makes more sense to move mp_raw_code_save_file
out of persistentcode.c and put it in mpy-cross/main.c
, so I've done that instead.
mpy-cross/wasm.c
Outdated
MP_UNREACHABLE | ||
} | ||
|
||
mp_import_stat_t mp_import_stat(const char *path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can disable MICROPY_ENABLE_EXTERNAL_IMPORT
and then this function isn't needed (that could be done for the existing native variant as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good idea. Done.
mpy-cross/wasm.c
Outdated
NORETURN void nlr_jump(void *val) { | ||
vstr_t buf; | ||
vstr_init(&buf, 1024); | ||
mp_print_t fd_print = {&buf, fd_print_vstr}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use vstr_init_print()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Done (and below).
def _exec(self, args): | ||
try: | ||
st = os.stat(self._binary) | ||
os.chmod(self._binary, st.st_mode | stat.S_IEXEC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it try to make it executable, surely it already is executable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. This originally comes from the current PyPI package -- https://gitlab.com/alelec/mpy_cross/-/blob/master/mpy_cross/__init__.py#L16
@andrewleech do you remember the history here?
This looks really good! I wonder if we should use the term "variant" here to describe the native vs wasm version of mpy-cross. And then have a variants subdir like the unix port, that contains the various mpconfigport_xxx.h files. |
I agree it's conceptually very similar to unix variants... although the variants have a lot more in common than native-vs-wasm for mpy-cross (i.e. they're completely independent makefiles). I'm happy to change it, but I think what we have is fine. |
39d691a
to
4ff19cd
Compare
I've added Some notes on the three runtimes:
So right now I've just listed wasmtime in requirements.txt. I think with some clever tricks with https://peps.python.org/pep-0508/ we can make the requirements.txt install a suitable runtime for the current target. |
*/ | ||
|
||
mergeInto(LibraryManager.library, { | ||
mpycross_error: function(buf, len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be defined? It's inserted into the wasm namespace by the various backends, so this here never gets called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this file is an input to emcc to tell it what to expect will be provided by the runtime. At a minimum you can write:
mergeInto(LibraryManager.library, {
mpycross_error: null
});
but I think it's clearer to include the signaure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see.
Yes better to include the signature, but add a comment that it will be replaced with a real implementation when instantiating it in a WASM runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
py/mpconfig.h
Outdated
// A port may explicitly disable the NLR if it is unable to provide a suitable | ||
// implementation. If so, it must provide nlr_jump, and handle that in some | ||
// way (i.e. terminate). | ||
#ifndef MICROPY_NLR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think disabling this will be useful for more than just a mpy-cross wasm workaround? I preferred your previous solution, just a simple way to overridde NLR in "emergency" situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so (although maybe it's useful when first starting a port?).
I do like that this way is very explicit as to what it's doing -- everything is a no-op except nlr_jump.
The previous solution was a bit awkward in that it relied on the particular interpretation of disabling setjmp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've re-done this by adding a MICROPY_NLR_TERMINATE
option. This is how a port (e.g. mpy-cross.wasm) signals that it explicitly wants this disable, but it's overall a much smaller change to nlr.{ch}. The idea is that it mirrors the existing MICROPY_NLR_SETJMP
which is how a port signals that it explicitly wants the setjmp implementation.
mpy-cross/main.c
Outdated
(void)ret; | ||
} | ||
|
||
void mp_raw_code_save_file(mp_compiled_module_t *cm, const char *filename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could be useful for more than just mpy-cross. Eg it could be used by the unix port if it ever supports saving .mpy files, for example like micropython.save_mpy(...)
. So that's why I thought it was OK to keep it in py/persistentcode.c
, similar to how mp_lexer_new_from_fd()
is in py/lexer.c
and guarded by MICROPY_HELPER_LEXER_UNIX
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I put it back in persistentcode.c.
But I don't understand the logic behind the #if
guard for MICROPY_PERSISTENT_CODE_SAVE_FILE
in mpconfigport.h. mpy-cross requires this feature -- if it doesn't compile then mpy-cross isn't going to work anyway.
description = "MicroPython cross-compiler" | ||
readme = "README.md" | ||
authors = [ | ||
{name = "Damien George", email = "damien@micropython.org"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need something more generic here, like "The MicroPython authors"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should maybe do this for mpremote too. Let's do both in a separate PR. Note you can set "Maintainers" separately to "Author" too.
e.g. https://pypi.org/project/numpy/ (from https://github.com/numpy/numpy/blob/main/pyproject.toml#L26)
0308442
to
d433292
Compare
mpy-cross/mpy_cross/compiler.py
Outdated
""" | ||
raise NotImplementedError() | ||
|
||
def compile(self, src, dest=None, src_path=None, opt=0, march=None, emit=EMIT_BYTECODE): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could also take advantage of the recent addition to mpy-cross
to read from stdin and write to stdout so that we can compile bytes in to bytes out with no file I/O.
Maybe separate compile_file()
and compile_bytes()
function, or something like that?
We have a use case for this and currently have to jump through hoops to save things to temporary files and make sure we clean them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compile
method already exists (it's used in makemanifest.py and is supported in the existing mpy_cross package on PyPI).
However, what if we allowed src
and dest
to be file-like objects? (In particular, I'm thinking io.BytesIO
). Would that suit your use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. I'm assuming it could work like this...
If src
is a string, pass it as a command line arg or if it is a file-like object, pipe it to stdin of the mpy-cross
process. Likewise, if dest
is a string, pass it as the --output
argument or if it is a file-like object, pip it to stdout of the mpy-cross
process.
mpy-cross/mpy_cross/compiler.py
Outdated
- src: The path to the .py file | ||
|
||
Optional keyword arguments: | ||
- dest: The output .mpy file. Defaults to `src` (with .mpy extension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Python doc comments are usually restructured text, *src*
(or ``src``
) would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to "Google style" docstring comments, which is what we're using in the stub file work for the rest of the docs.
mpy-cross/mpy_cross/compiler.py
Outdated
|
||
Optional keyword arguments: | ||
- dest: The output .mpy file. Defaults to `src` (with .mpy extension) | ||
- src_path: The path to embed in the .mpy file (defaults to `src`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the explanation here could be made more clear. It took me a minute to remember that mpy-cross
allows overriding the name of the python file (i.e. __file__
).
Maybe something like "if provided, override the source file name embedded in the generated .mpy file, otherwise use src for the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done.
mpy-cross/mpy_cross/compiler.py
Outdated
- march: One of the `NATIVE_ARCH_*` constants (defaults to | ||
None, which is treated as NATIVE_ARCH_NONE). Architecture to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just make the default NATIVE_ARCH_NONE
and save the explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the silly tricks I was doing to declare the NATIVE_ARCH_* constants above, it was confusing the ruff linter. I've just made the constants explicitly, and updated it like you suggest.
try: | ||
import wasm3 | ||
except ImportError: | ||
raise CompilerNotFoundError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try: | |
import wasm3 | |
except ImportError: | |
raise CompilerNotFoundError | |
try: | |
import wasm3 | |
except ImportError as e: | |
raise CompilerNotFoundError from e |
Unless we need to support older Python versions where this won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this to work on Python 2.x... :'(
mpy-cross/requirements.txt
Outdated
@@ -0,0 +1,14 @@ | |||
# wasmtime has the best general compatibility. | |||
wasmtime==11.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if these were optional dependencies so that we don't always have to install all runtimes, e.g. pip install mpy-cross[pywasm3]
to get the runtime you actually want to use (or none at all if we want to use a the native backend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to come up with a requirements.txt that works for as many people as possible. I think what I can do is a combination of "Extras" and "Environment Markers" (following https://peps.python.org/pep-0508/) such that pip install mpy_cross
"just" works by default, but can be customised with e.g. pip install mpy-cross[pywasm3]
like you suggest.
At this stage, I think the best option is to come up with an environment marker that will install wasmtime by default for all the supported wasmtime targets, and then the others can be manually chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I understood PEP-508 enough, but it seems like I don't.
I've updated the branch with a requirements.txt that will correctly install wasmtime on the platforms that wasmtime supports (64 bit windows, linux, linux-arm, mac). But I don't know how to declare an optional dependency such that e.g. pip install mpy-cross[pywasm3]
will do what you'd expect (i.e. install the optional dependency with the "pywasm3" extra tag).
@dlech Do you know what I'm missing here? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, extras are only additive. So the first thing that comes to mind is that we would need to split this up into an mpy-cross-core
and an mpy-cross
package.
The mpy-cross
package could include the __main__.py
and mpy-cross
script and the "just-works" dependencies (and backwards compatible API with existing mpy-cross
package if we need that too). The dependencies could either be mpy-cross-core[all]
or some platform-specific variations.
The mpy-cross-core
could contain only the programmatic API and the mpy-cross
binaries. All dependencies in this package (wasm runtimes) would be under an extra flag, i.e. consumers must either specify an extra dependency or supply their own mpy-cross
executable.
I suppose we could take this one step further and put the binaries and backend code for each of the various backends in separate mpy-cross-backend-xyz
packages, but the binaries are probably small enough that it isn't worth the extra effort to go this far. On the other hand, as time goes on and the number of binaries grows, e.g. for multiple MPY versions compounded with multiple runtimes, this may eventually make more sense to do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it a bit more, having the suggested package split could also allow the mpy-cross
package to follow the existing versioning scheme of exactly matching the MicroPython version while the mpy-cross-core
package could actually use proper semver for the Python package itself.
#else | ||
// Enable the fall-back setjump implementation. | ||
#define MICROPY_NLR_SETJMP (1) | ||
// #warning "No native NLR support for this arch, using setjmp implementation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just delete this warning if we are not going to enable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too... I'll leave it to @dpgeorge to decide.
|
||
#if MICROPY_NLR_SETJMP | ||
#include <setjmp.h> | ||
#endif | ||
|
||
typedef struct _nlr_buf_t nlr_buf_t; | ||
struct _nlr_buf_t { | ||
typedef struct _nlr_buf_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just bikeshedding, but I thought these typedefs were more readable the way they were
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it was improving consistency with how we were doing this elsewhere, e.g. https://github.com/micropython/micropython/blob/master/py/runtime.h#L66 and https://github.com/micropython/micropython/blob/master/py/pairheap.h#L48 (amongst others). (But I also like this way better, so ... I'm biased)
As py.mk also needs to know the path to the port config file, a port should use this rather than setting CFLAGS directly. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This lets mpy-cross just set the native arch without having to know the mapping. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
The port is required to terminate on exception, and does so by enabling MICROPY_NLR_TERMINATE and provide an implementation of nlr_terminate. This would only be used in a situation where you didn't expect exceptions to be raised, or if terminating the program was a valid thing to do. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This is step 1 of adding a WASM target to mpy-cross. Splits the config between mpy-cross specific (i.e. compiler configuration) and target specific (i.e. type config). This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This compiles mpy-cross.wasm, which can be run either in the browser or via any WASM runtime. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This adds support for using either a native or WASM binary in the Python wrapper for mpy-cross. WASM support is provided by wasmtime, pywasm3 and wasmer. It will use whichever is available. At least one should be an option on every supported platform/architecture. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This will generate a wheel using the WASM binary. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
This PR adds:
A way to compile mpy-cross to a self-contained WASM (not WASI) binary. This binary exposes an entry point to take Python source as bytes (as well as the various options), and return the generated .mpy file bytes. This could be used directly from Javascript/Node (will add an example).
A way to use this binary from the mpy_cross Python wrapper (currently via the
wasmtime
,pywasm3
andwasmer
runtimes). Between these three runtimes, all of the host targets and architectures we need should be covered. The performance in both these runtimes is excellent. Initial tests show very little difference to the native binary. It also works with pywasm (which is a pure-Python WASM runtime), but too slow to be of practical use.This is an alternative to #10834. (cc @dlech @andrewleech). Instead of having to go through the whole process to generate wheels for every architectures and platforms, we generate a single .wasm binary, and rely on there being a WASM runtime available instead for the target (which there seems to be). We can also ship multiple .wasm binaries in the (single) wheel for all the different bytecode versions we want to support.
In more detail:
__main__
entry point (which can now use either backend).Future work:
This work was funded through GitHub Sponsors.