-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Broken check_characters.py #3599
Comments
@CatalinVoss Do you mind checking? |
Ah sorry about that! I think two options
Do you have a preference between these two? |
I'd prefer 1 It could be awesome if you can add GitHub Actions coverage :) |
I updated the docs. It's awesome that you moved to Github Actions -- I love them. However, I couldn't figure out at a quick glance where you'd want to add an invocation of this script in the various workflows. I'm probably not the right person to write a test for this as I've also never used this script… |
I'd suggest adding a new job for that, next to e.g. the C++ binary tests |
So after thinking again, it also means we are breaking backward compatility. |
@reuben Do you have an opinion on whether it is sound we break backward compat here? |
it's even going to break because |
If you prefer, we can also just revert the remote I/O changes for this
file, which would mean that you just can’t do this for remote files.
…On Tue, Apr 6, 2021 at 05:26 lissyx ***@***.***> wrote:
We update the import to from io import open_remote which assumes that
you're calling the script from within the util directory.
it's even going to break because io is a python provided module, so
usually it should take precedence.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#3599 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACRFKYFY647VTVZXPWLOTTTHL4YBANCNFSM42JBTP7A>
.
|
Either way, current 0.9.3 images and docs are broken, and whatever fix we pick we need to make a new dot release (which makes me unhappy), so I prefer to wwait for reuben's opinion :) |
Sorry 😞 |
It was not against you, it's just that it's not the best time to have to do one. It just proves that what is not tested is broken ;) |
@CatalinVoss Do you mind sending the same PR agaisnt |
Reopening for |
@lissyx OK. Are there any other docs to update or is this one file change the only one? |
No it's the same one |
@CatalinVoss Thanks! |
Regression from remote IO work:
DeepSpeech/training/deepspeech_training/util/check_characters.py
Line 22 in 5f566f4
This fails with
Repro'd on
mozilla/deepspeech-train:v0.9.3
Docker imageThe text was updated successfully, but these errors were encountered: