Add CTC decoder builds for more Python versions #3596
Comments
|
@Ayushsunny This might be easier, if you are still willing to learn GitHub Actions at the same time |
Thank you so much as you want me to learn. It's so nice of you Sir @lissyx . |
Read this code, get yourself into https://docs.github.com/en/actions/guides :) |
|
@Ayushsunny Have you been able to get a grasp of what needs to be done ? |
|
@Ayushsunny Ping? |
|
I'm really sorry Sir @lissyx |
Thanks, I don't want to pressure you, it's fine if you can't take it, but we just need to know :) |
|
So Sir, as per my understanding I think this should be the code which I have to add. @lissyx ? jobs:
build:
runs-on: macos-10.15
strategy:
matrix:
python-version: [ '2.x', '3.x', 'pypy-2.7', 'pypy-3.6', 'pypy-3.7' ]
name: Python ${{ matrix.python-version }} sample
steps:
- uses: actions/checkout@v2
- name: Setup python
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
architecture: x64
- run: python my_script.py ` |
|
@Ayushsunny Sorry looks like my reply got lost ; that's the good basis, you'll have to build on top of that. |
Which reply Sir? @lissyx |
One I thought I had sent
What you shared is not what we need, but it's on the right path. We need a job like the one I shared earlier, but on more versions of python. |
Okay @lissyx Sir I got it do we need like this right ? build-ctc-decoder:
name: "Build CTC decoder Python package for testing"
needs: [ swig_macOS ]
runs-on: macos-10.15
strategy:
matrix:
python-version: [3.6.8, 3.7.9, 3.8.8, 3.9.2]
if: ${{ github.event_name == 'pull_request' }}
steps:
- uses: actions/checkout@v2
with:
fetch-depth: 0
- uses: ./.github/actions/install-python-upstream
with:
version: ${{ matrix.python-version }}
- run: |
python --version
pip --version
- uses: actions/download-artifact@v2
with:
name: "swig_macOS"
path: ${{ github.workspace }}/native_client/ds-swig/
- run: |
ls -hal ${{ github.workspace }}/native_client/ds-swig/bin
ln -s ds-swig ${{ github.workspace }}/native_client/ds-swig/bin/swig
chmod +x ${{ github.workspace }}/native_client/ds-swig/bin/ds-swig ${{ github.workspace }}/native_client/ds-swig/bin/swig
- run: |
make -C native_client/ctcdecode/ \
NUM_PROCESSES=$(sysctl hw.ncpu |cut -d' ' -f2) \
bindings
- uses: actions/upload-artifact@v2
with:
name: "ds_ctcdecoder-test.whl"
path: ${{ github.workspace }}/native_client/ctcdecode/dist/*.whl
- run: |
make -C native_client/ctcdecode clean-keep-third-party |
|
But sir How I am supposed to fix this if I am not able to find this /macOS-amd64.yml file in master branch for more knowledge I am sticking the screenshots here : Not available in the master branch -
see this is available in your branch -
|
|
@Ayushsunny looks like the original file has been renamed to build-and-test.yml. I think this is the section you want to be looking at (it looks like what was originally posted by @lissyx after a very brief inspection). |
|
Yes @dijksterhuis, Thank you so much. |
Yes. The whole idea of our switching to Github Actions is to allow people to run CI easily on their account as well:
Tada, things runs :). Experiment, ask us help and we can move forward. |
I see you opened the PR against our repo ; if you open it against your repo, then you can run GitHub Actions without our approval and debug on your own side ;) |
|
@Ayushsunny That's a good start, you might want to add / experiment more test coverage on all those versions now |
Yes Sir, Thank you so much for your review |
|
Sir I have also made a PR against my repo and I think I have updated all what we need Ayushsunny#2 |
|
Sir @lissyx ? |
It looks good but I don't see any CI run ? |
Sir, Can you approve the PR which was made against the Original Repo. |
Sir, Everything is looking fine my side. |
No, it is not:
|
Sir, even after updating as you've said, it's again throwing error something like that |
This is specific to macOS that does not use the same action. You need three-digits versions number for it. |
Hello Sir, I'm bothering you too much today see again it throws some new error which was not added by me https://github.com/Ayushsunny/DeepSpeech/runs/2492609550?check_suite_focus=true |
it is failing because trying to rebuild Please investigate what versions of numpy provides a wheel on pypi that works for python 3.8 and then update the env variables |
Sir, I have tried to find out every solution for this but I am not able to. are we going to add this version ?? |
|
@Ayushsunny Do you understand the problems stated in https://github.com/mozilla/DeepSpeech/blob/master/.github/actions/numpy_vers/README.md and how we solve them ? |
|
@lissyx , Yes Sir that we have to avoid having to rebuild numpy so we stick to versions where there is an existing upstream wheel file. |
I have already explained to you, I can't explain more if you don't tell precisely what you don't understand. |
|
@lissyx I went through this thread, spent some time looking at github actions, came up with this pull request hmen97#2. CTC decoder has been built for
The test python bindings step for each platform is running into an error or being skipped, logs below:
Will it be solved if I add the required prebuilt numpy wheel from pypi(for each python version) in this project or does some other change have to be made while saving artifacts? |
This change does not look super good at a first glance, you bump the minimal versions on many linux versions for example, this is going to break backward compatibility
I can't give any definitive answer only from that line, I need to know how you repro that ...
Those prebuilt wheels were for Aarch64 only. Without context, I have no idea what you are doing. |
Used this repo for minimum recommended version of numpy. I can run more tests to find the minimum stable version for each platform&py-version combination.
I just changed the numpy versions in .github/actions/numpy_vers/action.yml and in github/workflows/build-and-test.yml I added python versions in the build ctc decoder matrix strategy. On creating a pull request to my forked master branch, the checks are triggered. Basically was checking how everything works, or rather where and why it breaks for different versions.
My bad the wheels are already present here, when I checked it was down yesterday probably because of the outage. But let's say we need a lower version of aarch64 numpy wheel, do we stick to what numpy has wheels for(1.19.2 onwards) or build our own like you did |
nice repo, is this official? it seems to only focus on arm/aarch64 though it still does not explain why you push everything with a bare minimum of
Please give me a link to the errors ?
|
|
So: https://github.com/hmen97/DeepSpeech/runs/2781014601?check_suite_focus=true#step:10:152 This is exactly what I was telling, you bumped linux minimal deps with no justifications, so the packages on https://github.com/lissyx/deepspeech-python-wheels built only for CI are not met. |
At some point, I think what should be done is using this to replace the hard-coded list as much as possible, instead. And have ways to reproduce what you did ; because so far, I only see versions being changed with nothing to justify, and breaking backward compat. |
Looks like now PyPi accepts
|
Oh yeah, I just bumped that up to the min numpy version from the versions in specific platform & instruction set combination. Will revert that to what it was before.
Yeah I can do that, but I just need one clarification, the host system is a |
I'm not sure I get your question. Python wheel are cross-compiled, as |
|
it is built here for linux/armv7 for example: DeepSpeech/.github/workflows/build-and-test.yml Lines 2280 to 2353 in 9e67724 |
|
And specifics are handled in DeepSpeech/.github/workflows/build-and-test.yml Lines 2343 to 2349 in 9e67724 |
|
The action is here https://github.com/mozilla/DeepSpeech/blob/master/.github/actions/python-build/action.yml and it relies on makefile level abstraction of the cross-compilation of the bindings |
|
The build is done here https://github.com/mozilla/DeepSpeech/blob/master/native_client/python/Makefile#L13 and you can see we depend on a lot of env variables they are being set in https://github.com/mozilla/DeepSpeech/blob/master/native_client/definitions.mk and for example you can see the ones for linux/armv7 in https://github.com/mozilla/DeepSpeech/blob/master/native_client/definitions.mk#L72-L92 |
And as you can see, this is where we set some
|
I followed the calls all the way to the native_client/python/Makefile , didn't notice the definitions.mk. Ok so I think I get it now, correct me if I'm wrong, you set the architecture to So based on the os and architecture, if I create another case for |
|
BTW, the original issue was about the CTC decoder python binding, not the DeepSpeech python binding |
Yeah sorry for deviating, I got through a few levels of understanding because of that. Anyway please check this pull request. The Win|Build Tensorflow (opt) test is failing in master too, here are the logs, looks like Bazel isn't able to find some Visual C++ build tools. |
I'm sorry but:
|
|
Hey @lissyx I checked the successful pull requests, looks like they were using an artifact, which is missing in my run, so this Win|Build Tensorflow (opt) is passed without actually running any of the build instructions in the test. Can you check if that artifact still exists in the CI_ARTIFACTS_DIR? or maybe there is some config I'm missing... here are some logs for ref... 2021-06-20T12:34:40.8675106Z HEAD is now at 6f9dee7 Merge 5f1a600 into 9e67724 |
The artifact is missing because previous builds expired after the 90 days, and your logs shows something is broken on Windows builds. You need to investigate this, I can't do more. |
We hard-coded the action call with one version
DeepSpeech/.github/workflows/macOS-amd64.yml
Lines 54 to 86 in 5f566f4
train-test-modeljobs using a matrix as well to ensure test coverageThe text was updated successfully, but these errors were encountered: