Add support for netstandard and net core in dotnet client #3373
Conversation
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
stepkillah
changed the title
native_client/dotnet/DeepSpeechConsoleNetCore/DeepSpeechConsoleNetCore.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@stepkillah Thanks, but we will also need CI support to handle building and testing that.
Could you have a look around
DeepSpeech/taskcluster/tc-build-utils.sh
Lines 240 to 338 in 51e351e
And the final part that will trigger builds
DeepSpeech/taskcluster/win-build.sh
Lines 50 to 54 in 51e351e
|
@stepkillah It's mostly about bash glue to perform the builds, so you should be able to add your own dotnetcore alternatives and @carlfm01 and myself we can help you working on that. Given I know nothing about .Net, I can't really judge your current PR and I'm unable to check whether it works or not (no Windows either), so having CI is kind of critical here. Don't hesitate to reach for help on those matters on Matrix: https://chat.mozilla.org/#/room/#machinelearning:mozilla.org |
|
@lissyx thanks a lot for your feedback, I will review/fix PR and will try to set up build and tests for that |
Thanks @stepkillah ! One thing I thought about after is that you don't provide doc update / dependencies infos, I'm sure we need some ; just to even be able to add missing bits on our CI: https://github.com/mozilla/DeepSpeech/tree/51e351e895b845e48f64f2ccb84d3bbb2b0d3379/native_client/dotnet#building-deepspeech-native-client-for-windows |
There was a problem hiding this comment.
Thanks for the PR, few questions
¿Have you tested what happens on Linux?
Since we only pack the .so for Windows we need it for non Windows as well, we need the NuGet to be able to unpack the so with the same name as Windows, if the Nuget is not able to unpack with the same name wee need to add conditional compilation or patch NativeImp.cs to select the so to select the name based on the platform.
¿Naudio keeps the same API surface for .NetCore?
moved test sound file to shared folder, using link now
native_client/dotnet/DeepSpeechConsole/DeepSpeechConsole.csproj
Outdated
Show resolved
Hide resolved
Was not able to test it on Linux yet (will try to test it today) but after a closer look - yeah, need to think about how to do it correctly, since net core is cross-platform we need to include Seems like I don't have "full" linux machine, so will use raspberry with raspbian os for test, will it be sufficient? |
|
@carlfm01 after playing around with libraries I was able to build nuget manually. It unpacking native lib based on runtime, which allows putting Still was not able to test it on Linux because noticed that my raspberry running on So wondering if I can access all needed |
Maybe do like we d o with NodeJS: build platform nuget everywhere, and merge all platforms after? |
I'm not sure we should focus all our energy on linux right now for .Net, and please keep in mind that Can't you also rely on cross-compilation ? |
Not sure what is the best way to accomplish this, but we need to do it before packing nuget here. That's seems to be a good place to insert additional dll's and so we basically need to adjust build steps to be able to create a similar folder structure Also question - do we need to separate net core from net framework builds? I don't see too much difference, but if it will be the same build - probably need to update its name |
If you want to be able to handle building on linux, you will need to. This way you can add calls on linux build scripts.
Just consider we can add a new taskcluster task that just does the nuget pack step, pulling |
|
But again I'd really prefer if we could avoid over complexify the problem here. Maybe you can just add the .Net Core support to Windows and from there we can figure out how to support all Linux flavors. |
Actually Linux it's the main reason for doing it :) hm, not completely sure how cross-compiling works, but if |
I'm also interested to avoid complexity, so trying to find the easiest and effortless way to do that, and for now, it seems like the easiest way is to update the current steps by: |
You should have mentionned that from the start, because supporting that usecase is a whole lot more efforts CI wise.
Please read the docs, the I have no idea how .Net Core works on linux, and not even how/if it can handle cross compilation. But the thing tht we cannot do is native build on ARM hardware. |
It was my understanding that Net Framework was Windows only ? But i advise strongly against updating and i really prefer we have a different codepath for Net Core. |
|
Remember also that each flavor you add will require testing on the CI, and this can be a lot of efforts. |
you're right, but the build process is very similar, for this case, it seems just a matter of replacing |
Please send a separate PR to fix that, against both branch |
|
@stepkillah FTR I have cherry-picked your commit for |
|
merge again, my IDE did this automatically this time, sorry for not rebase, but seems to be good |
Can you rebase on current master, fix the conflicts, and ensure you have some clear history? |
Clear history is crucial, otherwise your PR does not really trigger correctly because of your merge commit. |
|
BTW @stepkillah I'm sorry but I don't see any of the last review comments I left being addressed. I can understand you're busy, can you just confirm me whether this is expected or not ? Do you need more help to complete the PR ? |
|
@stepkillah Sorry, we are now going to move out of TaskCluster as highlighted on #3317. it might not be such a big deal in your case, but I don't know yet the extends of the change to GitHub Actions for Windows side. Sorry about this extra work, I understand you already spent a lot of time on that. I will have a new pass of review with this in mind. You might also want to have a look at their guides: https://docs.github.com/en/actions/guides We hope it will make it easier for people like you to contribute ! :) Happy to review changes :) |
| @@ -0,0 +1,14 @@ | |||
| build: | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
| @@ -0,0 +1,20 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This script might still be useful for GitHub Actions
| @@ -0,0 +1,11 @@ | |||
| build: | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
| @@ -0,0 +1,79 @@ | |||
| $if: 'event.event in build.allowed' | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
| @@ -0,0 +1,12 @@ | |||
| build: | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
| @@ -0,0 +1,10 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This might still have value for GitHub Actions
| @@ -342,6 +342,29 @@ run_netframework_inference_tests() | |||
| assert_working_ldc93s1_lm "${phrase_pbmodel_withlm}" "$?" | |||
| } | |||
|
|
|||
| run_netcore_inference_tests() | |||
There was a problem hiding this comment.
For now, we are still using that as much as I can state
| @@ -302,6 +305,61 @@ do_deepspeech_netframework_wpf_build() | |||
|
|
|||
| } | |||
|
|
|||
|
|
|||
| do_deepspeech_netstandard_build() | |||
There was a problem hiding this comment.
This might still be usefil for GitHub Actions
| @@ -337,6 +410,77 @@ do_nuget_build() | |||
| nuget pack nupkg/deepspeech.nuspec | |||
| } | |||
|
|
|||
| do_nuget_repackage() | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
| @@ -62,3 +77,10 @@ get_dep_nuget_pkg_url() | |||
| # This should not be reached, otherwise it means we could not find a matching nodejs package | |||
| exit 1 | |||
| } | |||
|
|
|||
|
|
|||
| get_all_deps_from_task() | |||
There was a problem hiding this comment.
Sorry, this will not be useful anymore with GitHub Actions.
|
@stepkillah If you are okay, we are going to try with @reuben to start laying the basis for GitHub Actions very soon (I'm working on my fork for now), and maybe you might want to stick to only one platform to land this for now, but leverage the easier hackability of GitHub Actions to cover more software and hardware later. Your feedback on how such a mess our current CI is could be very valuable into simplifying as part of GitHub Actions :) |
|
@lissyx yeah, there is no point to target one platform with netcore or netstandard, so I can create separate PR using github actions when you finish the initial fork. Code itself it's not a problem at all, or net core/netstandard support, the main problem here (for me at least 😄 ) is to build it using taskcluster. I think/hope it will be much easier with github actions, so it make sense to wait until initial migration to new CI is completed |
If you agree, then you can fix your PR by removing the TaskCluster bits for now, and we merge that (with green TC of course for now) and within a few days when GitHub Actions is live you can pick up :) |
|
@stepkillah If you want to have a look, #3563 :) |
|
@stepkillah Gentle ping, I've merged the first GitHub Actions bit :) |
|
I would love to see this PR finished off so I can use DeepSpeech in my applications. I'd be happy to help out with getting GitHub Actions CI finished. Is that the only thing left? If so, I could create a new PR based on the latest |
|
@lissyx hello, a long time has passed since this pr was created :) is there a chance we can finish PR? I see it uses GitHub Actions - I can try to adjust the pipeline, but I didn't find where particularly dotnet builds |
|
Maybe we should consider moving the effort to coqui instead of deepspeech ? |
|
DeepSpeech is unmaintained, see #3693. |
Added additional targets for dotnet client to support
netstandard2.0,netstandard2.1,netcoreapp3.1.Not sure if something else required, probably need to update nuget, let me know if something missing or I need to add something more.