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.

Sign up for GitHub

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

Jump to bottom

Refactor TF and TFLite implementations into their own classes/files #2146

Merged
merged 6 commits into from
Jun 20, 2019
Merged

Refactor TF and TFLite implementations into their own classes/files #2146

merged 6 commits into from
Jun 20, 2019

Conversation

reuben
Copy link
Contributor

@reuben reuben commented Jun 2, 2019

This refactors the ModelState code which currently into a base ModelState class and two implementations, TFModelState, TFLiteModelState.

This greatly improves the readability of deepspeech.cc. I've also taken the opportunity to try and make the code style a bit more consistent. I've made all member variable names end with _ and fixed inconsistent indentation in a few places.

This also paves the way for fixing the multi-stream bugs we currently have in the client. Not sure if we should take this for 0.5.0 though, as it's a big change.

@reuben
Copy link
Contributor Author

reuben commented Jun 2, 2019

Hm, looks like something is wrong with Taskcluster, it didn't pick up the PR.

@lissyx lissyx closed this Jun 3, 2019
@lissyx lissyx reopened this Jun 3, 2019
@lissyx
Copy link
Collaborator

lissyx commented Jun 3, 2019

Hm, looks like something is wrong with Taskcluster, it didn't pick up the PR.

Retriggered them, it looks okay

@reuben reuben requested a review from lissyx June 3, 2019 22:42
lissyx
lissyx reviewed Jun 4, 2019
View reviewed changes
native_client/deepspeech.cc Outdated Show resolved Hide resolved
lissyx
lissyx reviewed Jun 4, 2019
View reviewed changes
native_client/tflitemodelstate.cc
<< "file with the same size as the one used for training."
<< std::endl;
return DS_ERR_INVALID_ALPHABET;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe this kind of check should be factorized on ModelState, like with a ensure_alphabet(size_t final_dim_size) that would check against GetSize() and return the same error, since the logic is the same for TensorFlow and TFLite runtime, only where we read the value differs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I want to do that, ideally we'd uplift all the metadata/model geometry checks to ModelState, but that requires finding a good solution for embedding metadata in the TFLite model first.

lissyx
lissyx approved these changes Jun 4, 2019
View reviewed changes
Copy link
Collaborator

@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.

LGTM, a few nits / questions, but really nothing blocking. I agree we should be careful and not land this before 0.5.0, though

@reuben
Copy link
Contributor Author

reuben commented Jun 6, 2019

Pushed a commit fixing multiple concurrent streams per model instance.

@reuben reuben force-pushed the refactor-model-impls branch 4 times, most recently from ae68380 to 8abb8ce Compare June 7, 2019 19:33
@lissyx
Copy link
Collaborator

lissyx commented Jun 13, 2019

@reuben I guess we can merge that after you rebase, now that 0.5.0 is shipped.

@reuben reuben requested a review from lissyx June 13, 2019 15:31
@reuben
Copy link
Contributor Author

reuben commented Jun 13, 2019

Re-requesting review for the last commit fixing the concurrent streams bug. I'm also going to add a test that exercises this.

lissyx
lissyx reviewed Jun 13, 2019
View reviewed changes
DeepSpeech.py Outdated Show resolved Hide resolved
lissyx
lissyx approved these changes Jun 13, 2019
View reviewed changes
Copy link
Collaborator

@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.

LGTM, just make sure we do not break on CUDA :)

@lissyx
Copy link
Collaborator

lissyx commented Jun 14, 2019

@reuben I landed a fix for the ElectronJS build issues, you can rebase on top of it.

@reuben reuben force-pushed the refactor-model-impls branch 2 times, most recently from c79c253 to 48bb5e5 Compare June 17, 2019 11:45
@reuben
Copy link
Contributor Author

reuben commented Jun 19, 2019

@lissyx I've addressed your comments and also added a test in f12ea5e. The test uses the prod model so it's not actually running, since the graph version was bumped here, but I verified manually. I'll also re-export the 0.5.0 checkpoint and update our prod-model in a separate PR.

@reuben
Copy link
Contributor Author

reuben commented Jun 19, 2019

I just saw your messages about 0.5.1 with the DS_IntermediateDecode fix. I'll hold on merging this PR until we do that.

@reuben reuben merged commit a2306cf into master Jun 20, 2019
@reuben reuben deleted the refactor-model-impls branch June 20, 2019 20:03
@lock
Copy link

lock bot commented Jul 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Jul 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lissyx lissyx lissyx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@reuben @lissyx