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 electron example #65

Closed
wants to merge 3 commits into from
Closed

Conversation

dsteinman
Copy link
Contributor

This adds an ElectronJS example. The example loads up the 3 example .WAV files and processes each through DeepSpeech and displays the transcription results in the window.

The readme has complete instructions, everything works on Mac and Linux. The dev mode works on Windows npm run dev-win, but the packaged windows app using npm run dist-win does not work. See 3127

There are some complexities involved in where the pretrained English models are located. I've done my best to simplify this. It is easiest to first run in dev mode npm run dev and let it download the models, and then try the packaged version with npm run dist. The reason for the download is because in the prod package, i can't figure out how to package the models from the /public directory, it doesn't work, but it does work from the electron "appData" directory. So the downloader saves the files there. This is probably the correct way to go anyways because the .exe/.dmg package doesn't need to include the model files.

If you really don't want download the models again, you can softlink the files to their destination location (see the Uninstall section of the Readme).

Copy link
Contributor

@lissyx lissyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsteinman That looks nice, thanks, but before merging can we please add a test.sh file like e.g., https://github.com/mozilla/DeepSpeech-examples/blob/r0.7/nodejs_wav/test.sh so we could have CI support ?

Since it's broken currently on Windows we could only enable that on linux and mac for the moment, but having coverage for the future would really be cool.

From the readme, it looks like the steps should already be simple enough, especially since CI setup already downloads model / scorer.

If you feel it's too complicated, I can take care of it later.

@dsteinman
Copy link
Contributor Author

I can add a unit test -- but what should test.sh do?

I guess could test that the electron app opens in dev mode (npm run dev) and successfully transcribes all 3 wav files and closes it back down. But I'd need more things than just a bash file to do this, like a mocha/chai type test engine.

And can the CI build system run desktop apps?

@lissyx
Copy link
Contributor

lissyx commented Jul 7, 2020

I guess could test that the electron app opens in dev mode (npm run dev) and successfully transcribes all 3 wav files and closes it back down. But I'd need more things than just a bash file to do this, like a mocha/chai type test engine.

Why? We could have a CLI flag that asks for std output and then you grab it and you compare.

Also, maybe just checking return / exit code is fine, and you set correct value of it when inference runs, that should catch the kind of regression we had in your electron-builder issue.

And can the CI build system run desktop apps?

It should since I think we have some coverage of some stuff like that, but I'm not sure under windows

@lissyx
Copy link
Contributor

lissyx commented Jul 7, 2020

Again if you feel it's too complicated, let me know and we'll take care of that (but maybe later).

@dsteinman
Copy link
Contributor Author

dsteinman commented Jul 7, 2020

Again if you feel it's too complicated, let me know and we'll take care of that (but maybe later).

No problem, I've added a test.sh and rearranged where the DS model filess are linked from (/models instead of /), and I added a npm run dev-test script which adds an environment variable to the electron code. That adds outputs messages to the console like this and then shuts down the app:

[1] test: 1 2830-3980-0043.wav experience proves this
[1] test: 2 8455-210777-0068.wav your power is sufficient i said
[1] test: 3 4507-16021-0012.wav why should one hall on the way
[1] test done

So I think this might be what you'll need.

The npm run dev-test works, but I'm not sure if the test.sh file is going to work, you'll need to take a look and try it on the CI system (it doesn't work locally on its own). It might need to be tweaked for Windows too.

dsteinman@80131da

@lissyx
Copy link
Contributor

lissyx commented Jul 7, 2020

Again if you feel it's too complicated, let me know and we'll take care of that (but maybe later).

No problem, I've added a test.sh and rearranged where the DS model filess are linked from (/models instead of /), and I added a npm run dev-test script which adds an environment variable to the electron code. That adds outputs messages to the console like this and then shuts down the app:

[1] test: 1 2830-3980-0043.wav experience proves this
[1] test: 2 8455-210777-0068.wav your power is sufficient i said
[1] test: 3 4507-16021-0012.wav why should one hall on the way
[1] test done

So I think this might be what you'll need.

The npm run dev-test works, but I'm not sure if the test.sh file is going to work, you'll need to take a look and try it on the CI system (it doesn't work locally on its own). It might need to be tweaked for Windows too.

dsteinman@80131da

Thanks, I'll run that tomorrow and will fix remainders issues. To avoid breaking CI I'll hold the merge until :)

@reuben
Copy link
Contributor

reuben commented Aug 3, 2020

@lissyx ping, did this slip through the cracks?

@lissyx
Copy link
Contributor

lissyx commented Aug 3, 2020

I fear it did, ill try to take care later

@lissyx
Copy link
Contributor

lissyx commented Aug 3, 2020

I'm fixing the current r0.8 status between deepspeech and examples repos and then I'l run this one, sorry @dsteinman for letting it slip through :/

@lissyx lissyx self-assigned this Aug 3, 2020
@dsteinman
Copy link
Contributor Author

I'm fixing the current r0.8 status between deepspeech and examples repos and then I'l run this one, sorry @dsteinman for letting it slip through :/

No problem at all.

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

I'm fixing the current r0.8 status between deepspeech and examples repos and then I'l run this one, sorry @dsteinman for letting it slip through :/

No problem at all.

Please look at #69, it's working except it is not closing after

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

@dsteinman Can you suggest how to properly close the process after performing the inference?

@dsteinman
Copy link
Contributor Author

dsteinman commented Aug 4, 2020

@dsteinman Can you suggest how to properly close the process after performing the inference?

Unfortunately this might not be fixable without an unpleasant hack like some of the suggestions here:

electron-userland/electron-webpack#249

The problem going on is that "process.exit()" is stopping the app, but not the development server. How to unit test an Electron app in this way is not clear to me.

I see what you tried here, does it work?

lissyx@0d753f5

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

I see what you tried here, does it work?

I'm still having troubles with concurrently package:

[1] TensorFlow: v2.2.0-24-g1c1b2b9
[1] DeepSpeech: v0.8.0-alpha.6-104-g7fbbfeb
[1] 2020-08-04 16:17:48.134736: I tensorflow/core/platform/cpu_feature_guard.cc:143] Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 AVX512F FMA
[1] ATTENTION: default value of option force_s3tc_enable overridden by environment.
[1] model loaded
[1] test: 1 2830-3980-0043.wav experience proves this
[1] test: 2 4507-16021-0012.wav why should one hall on the way
[1] test: 3 8455-210777-0068.wav your power is sufficient i said
[1] test done
[1] wait-on http://localhost:3000 && ./node_modules/.bin/electron public/electron.js DEEPSPEECH_TEST exited with code 0
--> Sending SIGTERM to other processes..
[0] BROWSER=none npm start exited with code SIGTERM
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] dev-test: `concurrently --kill-others --success 'last' "BROWSER=none npm start" "wait-on http://localhost:3000 && ./node_modules/.bin/electron public/electron.js DEEPSPEECH_TEST"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] dev-test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/build-user/.npm/_logs/2020-08-04T16_18_11_209Z-debug.log
[taskcluster 2020-08-04 16:18:13.221Z] === Task Finished ===

Looks like without --kill-others, there's still a process running, likely the web server one. And passing that, we get exit code of 1 when electron app forces 0 since it's correct

@dsteinman
Copy link
Contributor Author

A possible alternative I can think of might be to create a test build which doesn't require the development server to be running. I'll see if I can figure out how to do this.

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

A possible alternative I can think of might be to create a test build which doesn't require the development server to be running. I'll see if I can figure out how to do this.

Maybe a npm start; true can do the trick ... but CI is having issues I can't test that now.

@dsteinman
Copy link
Contributor Author

It looks like Spectron is the proper way to test Electron apps, but I'm not exactly eager to set all this up to satisfy one CI test. Let me know if there's no other solutions found.

https://www.electronjs.org/spectron

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

if there's no other solutions found.

my hack does not work :/

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

./electron/node_modules/react-scripts/scripts/start.js: if (isInteractive || process.env.CI !== 'true') {

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

Tried setting up CI=true but it's not exiting the process

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

@dsteinman According to my reading of ./electron/node_modules/react-scripts/scripts/start.js it should properly exit on SIGTERM, so I'm unsure if this is the cause ...

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

It looks like Spectron is the proper way to test Electron apps, but I'm not exactly eager to set all this up to satisfy one CI test. Let me know if there's no other solutions found.

https://www.electronjs.org/spectron

Yes, please see how we could use that?

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

@dsteinman or just have a way that npm start exits properly, but I have no understanding of its interactions with everything here.

@dsteinman
Copy link
Contributor Author

See here:

https://github.com/kimmobrunfeldt/concurrently/issues/127

I tried modifying the npm dev-test script to this and seems to work:

"dev-test": "concurrently --kill-others --success first \"wait-on http://localhost:3000 && ./node_modules/.bin/electron public/electron.js DEEPSPEECH_TEST\" \"BROWSER=none npm start\""

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

See here:

kimmobrunfeldt/concurrently#127

I tried modifying the npm dev-test script to this and seems to work:

"dev-test": "concurrently --kill-others --success first \"wait-on http://localhost:3000 && ./node_modules/.bin/electron public/electron.js DEEPSPEECH_TEST\" \"BROWSER=none npm start\""

Testing ... :)

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

it worked!

@lissyx lissyx closed this Aug 4, 2020
@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

Thanks @dsteinman I'll make a lastPR for msaterbranch

@dsteinman
Copy link
Contributor Author

Okay, great! Thanks.

@lissyx
Copy link
Contributor

lissyx commented Aug 4, 2020

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.

3 participants