Andrew Brown (Nov 02 2020 at 16:27): Andrew Brown (Nov 02 2020 at 16:27):@Alex Crichton and I were just discussing where to land the implementation of wasi-nn: in the Wasmtime repo temporarily or in its own separate repository? My impression from our last Wasmtime meeting was that, even though it wasn't optimal, @Dan Gohman agreed with me that placing it in the Wasmtime repository temporarily was ok.
Andrew Brown (Nov 02 2020 at 16:30):My reasons went something like: having multiple Wasmtime binaries floating around, one for each combination of WASI modules a user may want, is confusing for users and a pain for those of us who would have to maintain it. Until we have a plugin system, stuffing wasi-nn into Wasmtime as an optional feature and adding CI testing to keep it working would prevent it from getting out of date as Wasmtime changes (e.g. it depends on wiggle, @Pat Hickey, and having it in-tree would mean that we could immediately see if changes in wiggle break wasi-nn)
Dan Gohman (Nov 02 2020 at 16:31):Yeah, I think my earlier instinct was to look for ways to have it in a separate repo, but I can see the logic in having it in the main repo too
Alex Crichton (Nov 02 2020 at 16:39):I personally feel that this is best done by moving it into a separate repository until the wasi-nn proposal has progressed further. My thinking is that if we try to prototype all wasi proposals in-tree it could make the tree pretty unwieldy (things like slow CI time, maybe large clones, lots of flags/configuration to wade through, etc)
Alex Crichton (Nov 02 2020 at 16:39):I would prefer to land in-progress proposals out-of-tree and tackle the problems that brings up (like @Andrew Brown is mentioning)
Alex Crichton (Nov 02 2020 at 16:40):e.g. we should library-ify the CLI interface, make it super easy to plug in your own wasi proposal into the CLI, focus on stabilizing interfaces like witx, etc.
Till Schneidereit (Nov 02 2020 at 16:42):I have a different intuition on this, fwiw: I think there'd be a lot of value in making even experimental features available for easy testing in Wasmtime, meaning we build them by default, so they can be used with runtime flags. They wouldn't be runtime-enabled by default, of course, but there's a huge accessibility cliff between features that're included in the default build, and those that aren't
Till Schneidereit (Nov 02 2020 at 16:43):however, I very much appreciate that there are meaningful tradeoffs to this, so I wonder if we should have a more in-depth conversation on the policies around this more generally, not just for wasi-nn
Alex Crichton (Nov 02 2020 at 16:48):We could go both ways on minimizing costs really, so perhaps we could game out both scenarios for a bit and evaluate based on the end state?
Alex Crichton (Nov 02 2020 at 16:49):e.g. if adding it to the repo is incredibly cheap then it's fine, but if the cheapest we can get it is still pretty pricy that may swing the other way
Till Schneidereit (Nov 02 2020 at 16:52):yeah, that's a good point. Though I'd be surprised if find that it'd be very pricey to add something to the repo, but at the same time it's somehow very easy to use the feature from outside the repo. And no matter what, there's just a huge difference between "you can just use the feature in the default build" and "you have to build things yourself
Alex Crichton (Nov 02 2020 at 16:58):one of my main worries about adding it in-repo is the CI cost
Alex Crichton (Nov 02 2020 at 16:58):each proposal would be a new builder (presumably) with presumably only one maintainer to ping when things go wrong
Alex Crichton (Nov 02 2020 at 16:59):for something as valuable as CI that's a small bus factor for halting things
Alex Crichton (Nov 02 2020 at 16:59):I think we would also want it such that if you don't enable the feature the clone/build cost is as close as possible to what it is today
Till Schneidereit (Nov 02 2020 at 17:19):each proposal would be a new builder
can you say more about this part? ISTM if the feature is enabled by default, it'd not require an extra builder?
Till Schneidereit (Nov 02 2020 at 17:20):with presumably only one maintainer to ping when things go wrong
this is an excellent point, and something I very much agree we should avoid. So perhaps to be enabled by default, a proposal has to have a commitment by a stakeholder group — i.e. a team — to actively maintain it and address issues in a timely manner
Andrew Brown (Nov 02 2020 at 17:20):I think we would also want it such that if you don't enable the feature the clone/build cost is as close as possible to what it is today
I think my PR to merge wasi-nn in-tree meets that criteria: if the Cargo feature isn't enabled, the Wasmtime build time would not be impacted
Andrew Brown (Nov 02 2020 at 17:22):@Till Schneidereit, not sure if enabling wasi-nn by default is currently desirable (though I agree with making things super-easy to run): enabling it requires you to have OpenVINO installed on the system (which is... unlikely) or tries to build OpenVINO from source (which takes forever)
Andrew Brown (Nov 02 2020 at 17:26):I think eventually we need some form of "plugin" system to add capabilities to Wasmtime closer to runtime (along the lines of what you meant, @Till Schneidereit, but perhaps not all the way to "runtime flags"). Having wasi-nn in-tree would motivate the creation of such a system, if only to get wasi-nn out of tree.
Till Schneidereit (Nov 02 2020 at 17:29):@Andrew Brown ah yeah, that does sound like enabling it by default wouldn't be desirable right now. It's not possible to dynamically link to OpenVINO by any chance, thereby turning it into a runtime dependency?
Andrew Brown (Nov 02 2020 at 17:32):Well, OpenVINO does rely on shared libraries... maybe I'm being too careful, but I assumed that we wouldn't want to build the openvino crate unless the OpenVINO shared libraries are present on the system (either through binary installation or by building from source). Are you proposing we could just depend on the openvino crate and then the user will see a dynamic linking failure when they try to use it if they do not have the right libraries on their system?
Alex Crichton (Nov 02 2020 at 17:35):@Till Schneidereit oh I'm assuming wasi-nn is off-by-default but we still want to test wherever it lives
Alex Crichton (Nov 02 2020 at 17:35):so we'll need a CI builder running all the wasi-nn tests, and if that breaks it blocks everyone else from landing code until it's fixed
Alex Crichton (Nov 02 2020 at 17:36):My general thinking is that experimental wasi proposals shouldn't cause a burden on other developers, and the ways I can envision a burden are: CI time and flakiness, build time, and clone size
Alex Crichton (Nov 02 2020 at 17:36):Not to say the current PR doesn't handle build time and/or clone size, I think it does, just want to list things out!
Andrew Brown (Nov 02 2020 at 17:38):My general thinking is that experimental wasi proposals shouldn't cause a burden on other developers, and the ways I can envision a burden are: CI time and flakiness, build time, and clone size
I agree... and we should list the inverse side as well: experimental wasi proposals shouldn't be a burden for the developers who write them :laughing:
Andrew Brown (Nov 02 2020 at 17:39):so we'll need a CI builder running all the wasi-nn tests, and if that breaks it blocks everyone else from landing code until it's fixed
That is partly good, though? If wiggle changes enough to break wasi-nn in CI, at least we can see that and fix it, right?
Alex Crichton (Nov 02 2020 at 17:42):I'm less worried about things like wiggle refactorings causing breakage, where the wiggle refactor would ideally take into account the impact on consumers where if they're in-repo they can be evaluated
Alex Crichton (Nov 02 2020 at 17:42):(although that can also be problematic because it may be hard to build an extension like wasi-nn);
Alex Crichton (Nov 02 2020 at 17:42):I'm more worried about wasi-nn CI breaking
Alex Crichton (Nov 02 2020 at 17:42):like it's just flaky for some wasi-nn-specific reason, like OpenVINO downloads go offline or something like that
Alex Crichton (Nov 02 2020 at 17:43):or OpenVINO takes forever to build on CI (things like that, these are just examples)
Till Schneidereit (Nov 02 2020 at 17:43):so part of my thinking here is informed by how JS runtimes do make experimental features available with very low overhead, and how that's really useful for the ecosystem. I.e., you can enable most experimental JS features and tons of in-progress bundled modules and APIs in Node.js by just using runtime flags, instead of having to do custom builds. That means it becomes really easy to play around with them and validate their usefulness to content authors.
We might well simply not be there in terms of maturity of WASI, the ecosystem, or Wasmtime, but I do think it's something worth striving for
Andrew Brown (Nov 02 2020 at 17:49):I mean, I had not considered the runtime flag idea much before, but I now think it would be possible if we are ok exposing the user to "library not found" errors (though we could provide some helpful error messages there); that approach would then imply a CI step like the one Alex doesn't want to depend on, though. I do agree that the wasi-nn CI step I added is just one more thing that can go wrong
Andrew Brown (Nov 02 2020 at 17:49):(even though I would commit to fixing it if it breaks)
Alex Crichton (Nov 02 2020 at 17:52):sorry to be clear I'm just trying to be clear about the costs of putting it in-repo
Alex Crichton (Nov 02 2020 at 17:52):I don't mean to pass judgement on whether it's worth it or not
Alex Crichton (Nov 02 2020 at 17:53):I agree with you and Till that in-repo is the best for development of the proposal itself
Till Schneidereit (Nov 02 2020 at 17:53):entirely understood, and I'm glad you're raising these concerns. We should absolutely not enable experimental features lightly and without feeling confident that the results won't be painful
Andrew Brown (Nov 02 2020 at 22:54):I agree with you and Till that in-repo is the best for development of the proposal itself
@Alex Crichton, so how should we conclude this conversation? Do you want to discuss more?
Alex Crichton (Nov 02 2020 at 23:00):I think we need more input, my point about CI is something I don't think we can avoid and I don't want to speak for everyone else
Alex Crichton (Nov 02 2020 at 23:00):if we want to move forward with this I think there needs to be more buy-in and consensus about what to do about CI
Chris Fallin (Nov 02 2020 at 23:07):To give some input / my own perspective on the CI issue -- I do find that it is somewhat productivity-draining to wait for the long tail of CI builds, and I'd be worried about anything involving a large build of a dependency. Is there a way we could cache it, or download a prebuilt image/release of OpenVINO, or...?
Chris Fallin (Nov 02 2020 at 23:10):As a general point: would it make sense to think about "tiers" in the CI at some point? E.g. in the SpiderMonkey/mozilla world, we had tests that would be run nightly but wouldn't block a checkin. (IIRC that Rust does something similar?)
Chris Fallin (Nov 02 2020 at 23:11):(Perhaps that's a separate/tangential conversation, but seems to overlap at least here w.r.t. the notion of add-on / experimental pieces)
Andrew Brown (Nov 02 2020 at 23:36):The wasi-nn CI step I added in the PR takes ~5 minutes--this is significantly shorter than other steps, like the x64 backend (~11 minutes), building the API documentation (~18 minutes), test (~17 minutes), or build (~30 minutes). That is mainly because it uses a prebuilt OpenVINO binary that it installs as a GitHub action.
Andrew Brown (Nov 02 2020 at 23:40):@Chris Fallin, I like the tiered idea: it would be nice to see that certain things fail (e.g. peephole optimizer stuff, @fitzgen (he/him)) but they shouldn't absolutely block merging.
Last updated: Nov 22 2024 at 16:03 UTC