search

kaldi-asr / kaldi

kaldi-asr/kaldi is the official location of the Kaldi project.
http://kaldi-asr.org
Other
14.03k stars 5.3k forks source link

[DO NOT MERGE] Support openfst 1.8.2 #4829

Open georgthegreat opened 1 year ago

georgthegreat commented 1 year ago

This is a demonstration PR requested by @jtrmal in #4716.

There are only code changes, though we also have some CMakeLists changes needed to build kaldi via nixpkgs.

jtrmal commented 1 year ago

thanks, looks actually quite straight forward any reason why you removed the kaldi namespace scoped int definitions? y.

On Thu, Feb 23, 2023 at 1:25 PM Yuriy Chernyshov @.***> wrote:

This is a demonstration PR requested by @jtrmal https://github.com/jtrmal in #4716 https://github.com/kaldi-asr/kaldi/pull/4716

You can view, comment on, or merge this pull request online at:

https://github.com/kaldi-asr/kaldi/pull/4829 Commit Summary

File Changes

(22 files https://github.com/kaldi-asr/kaldi/pull/4829/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4829, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYXYVA454PVDH2RAWZH3WY6TRTANCNFSM6AAAAAAVF7GKME . You are receiving this because you were mentioned.Message ID: @.***>

georgthegreat commented 1 year ago

I do not think there was any particular reason fo removing (though the same was done in #4716).

kaldi uses int32 (without _t) suffix from openfst, but openfst removed these aliases in 1.8.2 (I have no idea why this was done in a version changing patch number only).

I think that the intergration will be done version-by-version and we will be able to discuss the changes in the detailed PRs.

jtrmal commented 1 year ago

thanks, makes sense. yeah, sometimes the numbering vs changes was/is pretty wild in the 1.8.x versions y.

On Thu, Feb 23, 2023 at 1:36 PM Yuriy Chernyshov @.***> wrote:

I do not think there was any particular reason fo removing (though the same was done in #4716 https://github.com/kaldi-asr/kaldi/pull/4716).

kaldi uses int32 (without _t) suffix from openfst, but openfst removed these aliases in 1.8.2 (I have no idea why this was done in a version changing patch number only).

I think that the intergration will be done version-by-version and we will be able to discuss the changes in the detailed PRs.

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4829#issuecomment-1442249834, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYX7E73T5GZNNQEHNDVDWY6U2JANCNFSM6AAAAAAVF7GKME . You are receiving this because you were mentioned.Message ID: @.***>

jtrmal commented 1 year ago

I suppose we should leave this a few days as is, just to gather inputs from other people... @danpovey any opinion from you?

galv commented 1 year ago

My only comment is that I'm not a big fan of polluting the global namespace with int32, etc. It can cause downstream pain for those who use kaldi as a library.

danpovey commented 1 year ago

Is there demand for the latest version of OpenFST, or a concrete reason to upgrade? I'm concerned that if we merge it, it will break existing builds and force people to try to update their OpenFST and recompile Kaldi. Together with the int32 change, it could break a lot of peoples' builds if they have integrated Kaldi into their projects. So there would have to be a fairly compelling reason to merge this into the master branch, IMO.

h-vetinari commented 1 year ago

Just for cross-reference purposes, @mmcauliffe has a PR against this PR that could hopefully be considered here.

mmcauliffe commented 1 year ago

So migrating to OpenFst 1.8.2 makes it a lot easier for us to get shared libraries working for Windows, and we additionally don't have to ship OpenFst 1.7.X libraries inside the kaldi package. I have everything building successfully on conda-forge here: https://github.com/conda-forge/kaldi-feedstock/pull/38, but it'd be ideal to have these changes upstream.

danpovey commented 1 year ago

ping to keep it open, although we probably won't merge any time soon

kkm000 commented 1 year ago

Superseding: #4716

kkm000 commented 1 year ago

@danpovey

ping to keep it open, although we probably won't merge any time soon

Dan, just slap the label https://github.com/kaldi-asr/kaldi/labels/stale-exclude on it, and the bot will shut up. I don't know if there's a magic trick to add a label in an e-mail reply. Close: #1234 on a line by itself closes an issue, but that's all I know. :(