Conversation
…r on wasm (FIXME)
|
@robertu94 I'd like to push forward with these changes now that they've been cleaned up |
robertu94
left a comment
robertu94
left a comment
There was a problem hiding this comment.
Most of these comments are small.
CMakeLists.txt
Outdated
| set_property(CACHE LIBPRESSIO_BUILD_MODE PROPERTY STRINGS "CORE" "FULL") | ||
| endif() | ||
|
|
||
| option(LIBPRESSIO_WITH_EXTERNAL "Support external plugins" ON) |
There was a problem hiding this comment.
The external plugins cannot be supported on WASM. The external metrics could be supported in theory, but in practice, the wasi-libc doesn't support mkstemps yet (WebAssembly/wasi-libc#229). The forkexec is definitely not supported in WASM (maybe one day WASI will support command spawning, there are forks that do this, but no standard support yet).
There was a problem hiding this comment.
External is not intended to be tried explicitly to fork+exec. There are currently 4 implementations python, mpi, forkexec, and http. Maybe there are functions/refactoring we need to do to support this, and which case we can split this into a seperate issue.
There was a problem hiding this comment.
Currently, none of them can be implemented in WASM, but one may be able to come up with one that could (an in-process external ...) in the future.
How should we resolve this for now? Should forkexec get its own feature switch, like the others, but on-by-default? What should we do about the external metric? We could add a WASM-specific polyfill for mkstemps, but having an external metric without any external launch wouldn't work anyways ...
There was a problem hiding this comment.
I thought there was: https://docs.rs/wasmtime-wasi-http/latest/wasmtime_wasi_http/ which would allow the HTTP option. If you don't want to support this for now I understand, and we can add a switch.
There was a problem hiding this comment.
wasi does offer HTTP, yes, but I haven't yet added support for it in the numcodecs-wasm engine. This is mainly because the engine is set up to create a fully sandboxed environment - there is no input beyond the data bytes (the environment is controlled, randomness is seeded, stdin is empty) and the only outputs are stdout/stderr, which also provides no feedback. This is all done to ensure that execution is fully reproducible. Exposing external metrics, compressors, etc., would change the guarantee to "only deterministic if the external one is deterministic", which is something I would likely switch to at some point. But at least for now, supporting external is out of scope
There was a problem hiding this comment.
I've at least added support for wasi:http in my sandbox in juntyr/wasi-sandboxed-component#3 - this would allow code depending on wasi:http to compile, but every outgoing request is refused (yay sandbox), and there are no incoming requests (yay sandbox)
|
@robertu94 Thank you for the review! I've tried to address all of your comments, but left two unresolved since I need more input from you. |
|
@robertu94 Does this PR need anything else, or can it be merged? Ideally, I'd like to publish the libpressio-rs and numcodecs-pressio crates once this PR is upstream (and ideally ideally there has been a release with these changes so we can point at a release version) |
|
I think it looks good. I want to pull in these changes locally and do some testing to double-check that nothing broke, and I hope to do that soon. It's on my todo list I promise. |
2 participants
This PR allows libpressio to be compiled for wasm32, which is used with the libpressio-rs bindings to compile numcodecs-pressio to numcodecs-wasm-pressio