search

mattermost / mattermost-plugin-calls

https://www.mattermost.com
Other
96 stars 55 forks source link

[MM-53354] Review websocket events #821

Closed streamer45 closed 2 months ago

streamer45 commented 2 months ago

Summary

PR reviews all of our websocket events to make some small optimizations on what we send. For example, we can now send signaling data only to the dedicated ws connection. This should work fine as handling code has been on our clients (web/mobile) for a long time. I did a quick smoke test, including reconnections, and everything was working fine. It may benefit some soaking, but overall, it's not a big change.

Ticket Link

https://mattermost.atlassian.net/browse/MM-53354

streamer45 commented 2 months ago

Transcriptions e2e tests are expected to fail until we release https://github.com/mattermost/calls-transcriber/pull/30.

streamer45 commented 2 months ago

Somehow whisper.cpp is crashing on the CI with a SIGILL (illegal instruction). Something to do with the new v1.6 version most likely. Will need more time to debug.

streamer45 commented 2 months ago

Looks like new Whisper versions are compiled against a wider CPU instruction set (e.g. AVX512) which is missing in the stock Github Action runners.

streamer45 commented 2 months ago

Building the transcriber image directly on the pipeline confirms this. E2E tests are now passing.

streamer45 commented 2 months ago

@cpoile This turned out to be a long and sweaty one :sweat_smile:

The major change is that we are now building the calls-transcriber image from the repository before running e2e tests instead of pulling the release from the public registry. This has the added benefit of e2e testing transcriber code before we release it. And of course this also means we won't have to worry about CPU instruction sets mismatches since we compile it at time of need.

streamer45 commented 2 months ago

image

:clap:

cpoile commented 2 months ago

Looks like new Whisper versions are compiled against a wider CPU instruction set (e.g. AVX512) which is missing in the stock Github Action runners.

That's a weird choice, considering how rare AVX512 support is (it was removed from Intel's consumer chips a couple generations ago).

streamer45 commented 2 months ago

That's a weird choice, considering how rare AVX512 support is (it was removed from Intel's consumer chips a couple generations ago).

Mmm, that's a good point. I suppose the reason this is added is because it's found on the systems where we build the image (our delivery self-host runners, likely EC2). But now I am getting worried about compatibility on the customer's side. It may be safer to release something with wider support then, I'll look into it.