Skip to content
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

Add server code #3391

Closed
wants to merge 2 commits into from
Closed

Add server code #3391

wants to merge 2 commits into from

Conversation

opensorceror
Copy link

First of all, I want to thank you for building such a wonderful product, and open sourcing it. We are currently using this at my workplace, and so far it has served our needs really well.

I would like to contribute a simple server that I've built, that serves the inference results over a websocket. This server code is currently being used at my workplace, and is fairly battle tested.

@community-tc-integration
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@olafthiele
Copy link
Contributor

Great idea, the wish for a server is something that was mentioned a couple of times. I am not sure whether @lissyx and @reuben think this might be better added to the examples over here:

https://github.com/mozilla/DeepSpeech-examples

And maybe leave out the gitignore files?

@lissyx
Copy link
Collaborator

lissyx commented Oct 29, 2020

@opensorceror Thanks for that PR, I had a look at it, and it is well done. However, it does diverge a bit from the purpose of that repository which is to provide basic building blocks for the project. As @olafthiele suggested, that level of integration fits much better in the examples repository, and so your PR is welcome there on both r0.8 as well as master branch !

@opensorceror
Copy link
Author

@lissyx @olafthiele It makes sense to me that this should be in the examples repo, and I'm fine doing that. Before I do that though, let me make a case for including it in this (core) repo.

Currently, besides the core DeepSpeech code itself, the code for native clients is also included in this repo. One could argue that the clients should also be in a separate repo (or several), but that is not my viewpoint - I think it makes sense to include them in the core repo. Following similar logic though, I think we should provide an official server API as part of the core project, that will be maintained going forward (I'm happy to become a maintainer for this piece). I believe that it will result in increased adoption of this project, because many teams who use DeepSpeech may end up creating a server anyway. By providing an official server as part of the core project, we also encourage community contribution to it.
Besides maintaining the server piece, I'm also happy to create and maintain official Docker images and Helm charts for new releases.

As a future plan: Similar to how we have clients in multiple languages as part of the core repo, we could also provide servers in multiple languages. I have a plan to contribute a Scalatra-based server, for users who prefer the JVM ecosystem.

Just my thoughts. If you still think the server should be part of the examples repo, I am happy to move it there.

@lissyx
Copy link
Collaborator

lissyx commented Oct 29, 2020

One could argue that the clients should also be in a separate repo (or several), but that is not my viewpoint - I think it makes sense to include them in the core repo.

It makes sense, and we discussed about that back then, and decided that given the current (at the time) status of the project, our bandwidth, it was simpler and more efficient. This is coming with costs in term of complexity, because then people can (and are right) to expect consistency accross everything, even feature wise.

Some bindings are happily living outside: Rust, Go.

I think we should provide an official server API as part of the core project,

Maybe, but here we are moving from basic building blocks (bindings are required to access the library without a lot of pain), while a server is a higher-level piece. Also, it comes opiniated: people will want other archs, other languages as you said. It risk becoming a lot of not-core-related codebase, adding noise and complexity.

By providing an official server as part of the core project, we also encourage community contribution to it.

By providing an official server, people might also expect server-grade performances. Having worked a few weeks on that, there were some non trivial bottlenecks we had not enough bandwidth to address.

I'm also happy to create and maintain official Docker images

We already have some, so if you have PRs on that it's more than welcome.

Just my thoughts. If you still think the server should be part of the examples repo, I am happy to move it there.

Yeah, sorry, I hate to bring messages like that on PRs properly baked, but for the focus, and the future of the project, it's better.

FTR, we have Examples integrated in our CI, so you could add your server as an example and maintain it there, and add CI here to ensure it does not break. We'd be happy to guide you on that.

@opensorceror
Copy link
Author

Alright, you made some good points and convinced me 🙂 I'll close this PR and create one in the examples repo.

FTR, we have Examples integrated in our CI, so you could add your server as an example and maintain it there, and add CI here to ensure it does not break. We'd be happy to guide you on that.

About this, let me take some time to review the CI. For now, once I create the PR in the examples repo, I'll create an issue in that repo to track this.

@lissyx
Copy link
Collaborator

lissyx commented Oct 29, 2020

time to review the CI

It might look complicated, so don't hesitate to read #3317 and ask for help here, on Discourse, or better on Matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants