-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove old CTC decoder (Fixes #1675) #1696
Conversation
reuben
commented
Nov 3, 2018
•
edited
Loading
edited
- Remove old decoder code and build system integration
- Remove references in code and tests
- Update documentation
8d022cc
to
da29a5c
Compare
@lissyx I'm having trouble getting the ctcdecode package to build on ARM64, getting relocation errors in libm.a when doing the final linking of the Python extension. I see that in native_client/BUILD we link against libstdc++.a, is that a requirement on our ARM platforms? The strange thing is, it's building fine on ARMv7... |
da29a5c
to
c4cbc37
Compare
They should share the same linking strategy, but maybe you lack some |
@@ -1,7 +1,7 @@ | |||
build: | |||
template_file: test-linux-opt-base.tyml | |||
dependencies: | |||
- "linux-amd64-ctc-opt" | |||
- "linux-amd64-cpu-opt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea, completion time of linux-amd64-cpu-opt
is much higher, it's going to delay everything behind, we should re-use linux-amd64-ctc-opt
to build the new python wheel instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that it's not a new Python wheel, it's several Python wheels, one for each architecture/Python version, so I tried to leverage the existing infrastructure for CPU builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it's still better IMHO to do that outside, we can easily hack around do_deepspeech_python_build
Initially the compiler error mentioned -fPIC, but compiling with that flag made no difference, it still fails with relocation issues in libm.a. |
c4cbc37
to
aa3503d
Compare
I think the problem is that definitions.mk doesn't cover all that's needed to get a build working on ARM, as it's missing some of the stuff that was added in the tensorflow CROSSTOOL file like pointing to a different linker. |
It's more simply just unable to find the proper
|
aa3503d
to
bee4d5b
Compare
We have liftoff! Thanks @lissyx! |
53ed610
to
c5d6c6d
Compare
In 38b5447 I've updated the docs and added some convenience code in |
@tilmankamp I ended up removing the COORD global entirely as it's only used in the |
4f8266b
to
c65c22f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of the PR is fine.
However there are a few places that some substitutions, e.g. cluster
to C.cluster
, seem to have been missed and a few other small nits that should be changed, e.g. from X import *
to from X import a, b, c
.
Everything should be addressed now. |
LGTM |
0a38df2
to
bd71f83
Compare
bd71f83
to
cfed8cc
Compare
@reuben, does this PR mean we don't have to generate the old trie model (i.e. from |
Yes.
…-- reuben
On 15 Nov 2018, at 03:17, Josh Meyer ***@***.***> wrote:
@reuben, does this PR mean we don't have to generate the old trie model (i.e. from v0.3.0) for training and new trie model for testing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@reuben I'm loading in an older checkpoint (v0.3.0), and I get the following error:
After some searching, I think this is related to a missing The following is the script I'm running to load the checkpoint:
Is this error a by-product of this PR? Are the older checkpoints on the release page not forwards compatible after this PR? -josh |
This works fine for me:
|
(On latest master) |
Strange, but good news. |
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. |