-
-
Notifications
You must be signed in to change notification settings - Fork 182
Add support for WASM plugins (with examples) #2158
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
Conversation
|
As I commented earlier I don't think doing away with the need for restarts is worth the trouble, but instead what you have here is really really interesting and pushing us forward. Just the ability to run a wasm radar server demonstrates the possibilities. Since this is such an elephant I'll probably make several review passes, so don't expect the first one include everything. |
tkurki
left a comment
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.
Please npm run format and npm run lint and fix lint errors.
Prefer import over require (lint will tell you that). Prefer typing things over adding ignore comments for any complaints.
I don't think we need to add the special support for wasm debugging, especially the serverroute. If this is something that you think would be useful let's think about the general case of configuring debugging for troubleshooting different kinds of problems. for the end user something being WASM should not be of special concern. Like, why should they care?
|
I appreciate the comment that you'll continue the work, but no need to comment on each item until each has been addressed. |
|
Git technique: you are merging master to this branch, which causes extra commits that are just noise. Please read up on rebasing - in general rebasing on master is a much better option: no extra merge commits and if conflicts arise you face them early and the branch will be eventually ready for merging to master. |
|
Thank you very much that you take your time and push this so much. PR: WASM Plugin Support with HotplugSummaryAdds WebAssembly plugin support to Signal K Server with complete hotplug lifecycle management. WASM plugins run alongside traditional Node.js plugins with the same enable/disable, install/uninstall behavior. ChangesCore WASM Runtime
Plugin Hotplug
SDK
Documentation
Tests
Examples
Files Changed140 files changed, 25,572 insertions, 80 deletions Key New Files
Breaking ChangesNone. WASM support is enabled by default. Testingnpm test # 124 tests passing |
|
See 2eda5f0 - let’s not include the radar API in this PR. I am thrilled about the possibilities there, but want to get Kees et al involved in the discussion, that is kinda orthogonal to adding wasm support. I assume the mayara plugin will also see separate development, so splitting it to another PR with the radar API would be the best way forward imho. |
|
As I understood you don't want me to comment every point, so I just "resolved" the ones that I have adressed and fixed. And comment on the important topics. |
I also struggled with this and wondered whether I should include it or not. In principle, we were almost in agreement, but the one open issue that he didn't respond to was whether we need a control websocket because the radar models are too different. However, we won't know the answer to that for another six months or a year, once everything has been tested and we have gained more experience. In the coming days, I will primarily work on MayaRa and focus my attention on control, not image enhancement. Then we should see whether the API has matured enough by the time of the merge or whether we should remove it before the “final merge.” As I see we are merging to a 3.x alpha - so even remonving from the last beta before RC would be still ok imho. I hope you agree with this approach. |
|
I successfully tested all functions of my furuno radar with the Radar API (v5) in the last build. Would love to see it merge. Let me know if you want me to change any additional things within this PR. |
tkurki
left a comment
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.
Need to stop this round now, continue over the weekend.
Tried to make suggestions as much as I could to make it easy to incporporate the feedback.
This is looking mighty good!
tkurki
left a comment
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.
Next pass, more comments. Still ways to go...
tkurki
left a comment
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.
More review comments.
packages/server-admin-ui/src/views/Configuration/Configuration.js
Outdated
Show resolved
Hide resolved
tkurki
left a comment
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 is very easy to add AI generated content, but I fear nobody will keep it up to date nor remove stale content, so let's try to be a bit leaner.
examples/wasm-plugins/example-anchor-watch-dotnet/ISSUE_REPORT.md
Outdated
Show resolved
Hide resolved
tkurki
left a comment
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.
Next round...
| */ | ||
|
|
||
| // Export all public types and functions | ||
| export * from './plugin' |
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.
Would it make sense to make the API granular, in the direction we are moving in with Server API, or are we happy like this? (don't have a strong opinion on this, you have much better insight here)
|
There's a build failure that needs addressing: https://github.com/SignalK/signalk-server/actions/runs/20374772192 |
It was introduced by your last commit: no worries, I will look into it later, want you to finish all first. 04:27am here, so still plenty of "day" left :). |
|
I really, really wish you had not included the hotplug mechanism in this PR. It adds complexity that is not directly connected to what I think is the core, the wasm plugin mechanism. For example there's a nontrivial amount of code to disable webapps for webapp & plugin combinations. But what I find really odd is that the hotplug mechanism does not hotplug plugins and webapps that are installed from the App store. I would think that the most important hotplug feature: not needing to restart the server to get access to the newly added addins. Is loading newly installed addins without restart not supported? Only developers toggle things on and off, end users rarely do that. Would it be possible to create separate PRs for the wasm support (i'm ok with including radar api, even if i would like to have that separate also) and hotplug? I would love to move forward with wasm. Related: many parts of hotplug.md now read like a PR description, not a long term document. I am personally not very fond of repeating the application code verbatim in documents, as code may evolve and often docs are not updated. To me permanent documents should not describe in detail the steps how something was implemented, but how it works. |
Why Hotplug Exists in the WASM PRThe hotplug mechanism was originally added for WASM plugin lifecycle management:
Your Observation is Correct - But There's an OpportunityYou're right that the current hotplug doesn't support "install and use immediately" for App Store plugins. However, here's what's interesting: WASM plugins CAN support this - the infrastructure exists:
Node.js plugins cannot easily support this due to module caching and Express route registration at startup. ProposalOption A: Make "Install & Use Immediately" a WASM Advantage
Option B: Split the PR as You Requested
Which direction would you prefer? Regarding Node.js Plugin HotplugThe Node.js hotplug (enable/disable without restart) was added for uniformity - same behavior for both plugin types. We can remove it if you prefer WASM-only scope. Regarding DocumentationAgreed - |
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 now tested the assemblyscript example webapps but they failed. The faults don't seem serious. I wonder if there should be a test that compiles them (at least the easy ones with npm based build), starts the server and asserts they are present?
I've also looked a bit into the hotplug infrastructure code. To be honest I think the hotplug is not keeping the core lean. It seems like Claude implemented it piecemeal and the result is not as clear as it could be. The logic seems to be spread in many spots over the codebase. I think that needs rework: after all there are just a few operations that actually change what plugins and webapps are available:
- initial plugins and webapps registering & enablement
- plugin enable/disable
- install / uninstall of plugins and webapps
Modifying the app 'splugins and webapps properties is imho a bad idea: I think the code would be easier to understand if we either
- always filter on the fly to app properties to get enabled ones
- keep separate data structures for all and enabled ones
The preexisting code is a bit weird, because it started as webapps and plugins being totally separate, but now that line has blurred. I think a single data structure holding both would be better. Room for improvement.
I am not totally against hotplug, but imho it should start from the users' perspective, where the main painpoints afaik are requiring restart for
- new plugin/webapp installs (solvable)
- uninstalls (partly solvable)
- updates (not solvable for plugin routes, webapps ok)
(At this point I read your reply to my separate comment)
I think we should split the PR, not include any hotplug features, and then armed with the knowledge gained here reimplement the parts of hotplug that are feasible, from scratch with a bit better code organisation.
examples/wasm-plugins/example-hello-assemblyscript/package.json
Outdated
Show resolved
Hide resolved
tkurki
left a comment
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 new package assemblyscript-plugin-sdk needs to be included in container build from master and in release workflows.
In master container build packages are built and stored as build artifacts so that the container build step can use them, as they have not yet been published in npm.
Release workflow checks the packages' versions against npm and publishes new versions as needed.
https://github.com/SignalK/signalk-server/blob/master/.github/workflows/release.yml
tkurki
left a comment
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.
Making progress, I see "Hello from AssemblyScript!" in my DataBrowser!
Some more details to tweak.
- Add AssemblyScript plugin SDK for WASM plugin development - Support WASM plugins written in Rust, Go, .NET, and AssemblyScript - Implement Component Model bindings for SignalK API - Add plugin lifecycle management (load, start, stop, unload) - Support multiple WASM formats (Component Model, standard WASM) - Add TCP socket API for network-capable WASM plugins - Implement plugin configuration storage via plugin-config-data - Support WASM plugin HTTP endpoints and webapps - Add comprehensive documentation and example plugins - Fix Asyncify race conditions for network-capable WASM plugins - Make WASM runtime toggleable via server settings
|
I think we are in a good enough shape with this. The commit log is not worth keeping and I don't want 100+ tweakthisandthat commits in master, so I'd like to split this to two commits. I don't really care if the split is 100% accurate or that they are 100% independent, but would be still good practise imho. This is how Claude would split it: https://github.com/tkurki/signalk-server/pull/new/WASM_WASIX-split How would you like to proceed? This is your work, so don't want my name on it. |
No worries @tkurki, I think in reality you spent at least as much time on it as I did. |
|
Good news! C# for WASM might work soon. |
|
Docker build is failing https://github.com/SignalK/signalk-server/actions/runs/20862210828/job/59944659181. |
|
Fixed in fd61431 |
|
🚀 🚀 🚀 |

WASM Implementation
Documentation
Example WASM Plugins
JS/TS
-course-provider-plugin-wasm
RUST
GO
C# .NET