[DO NOT MERGE] Support openfst 1.8.2

[georgthegreat]

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]

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 <#4716> ------------------------------ You can view, comment on, or merge this pull request online at: #4829 Commit Summary - 444310f <444310f> Support openfst-1.7.6 - f9ed1f1 <f9ed1f1> Support openfst-1.8.0 - 97a3ed0 <97a3ed0> Support openfst-1.8.1 - 2738ab1 <2738ab1> Support openfst-1.8.2 File Changes (22 files <https://github.com/kaldi-asr/kaldi/pull/4829/files>) - *M* src/base/kaldi-types.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-338cced3fed5be3eaa511ced41fe5d8d2ddf826d0468f2d8c06edb62f3f6d042> (43) - *M* src/chain/chain-supervision.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-97b4cf1998923a8df2ee0389cb0f0b19c9748b51cdbeb918fd47af69f78f5eb4> (10) - *M* src/fstext/determinize-star-inl.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-590d49f0b93e938a995c590ad70e2a53fb1b94258d4ad97522a36cd2d6f3434d> (2) - *M* src/fstext/fstext-utils-inl.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-b81b6c4725a09fefa354c7254e507fdd7155ea50c1e748caf7d20578263d2baa> (14) - *M* src/fstext/fstext-utils.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-1a3cf91d8ea0684e7d813e5b6d9f3f840ed5dbc442a9ae0bbc84a32fb9164fd0> (2) - *M* src/fstext/kaldi-fst-io-inl.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-df6432d60ea4c258778715418d22f6b78e3e4994368110d7d672d71c8f0eb1e8> (4) - *M* src/fstext/lattice-utils-inl.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-a2be040d3e733298435b7a7282c6e9deed62dc8016dc8e352f93eaaca9be6a5e> (4) - *M* src/fstext/lattice-weight.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-0246f8b9b11a217cf632e47f1092dcfd15e00d152d604ef896a77c0d7c3d822b> (16) - *M* src/fstext/pre-determinize-inl.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-55c7c244ad61bfdfea40f20df92673668012f9c5370e8e413818d65fe8685f3a> (4) - *M* src/kws/kws-functions.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-6d7e99f5a0ca53c9aa3767c9d1daf188ff80de8a2775d3c7824b87fc3ce74188> (6) - *M* src/kws/kws-functions2.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-4522a1bca8bdeab2b9f9e369a2925b70c1564b9c3a16c0ceef0cefca29453975> (2) - *M* src/lat/arctic-weight.h <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-94d4afb50baa3cdc337971cc81ee91255466880c0d277660b0c53db75df4b782> (2) - *M* src/lat/determinize-lattice-pruned.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-955cf999ef10e53375535153a680bbaeb7ba2e915ef23519b2cdff2df3f330a8> (6) - *M* src/lat/kaldi-lattice.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-6067cd9f4b6bba704091d66af6d93835b635582d85b5c255b911b3dbe262ab0e> (6) - *M* src/lat/lattice-functions-transition-model.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-e33def0af8d6b8e2a1a89b609710e575e5e911ea01651c0fa901e4371cdcf1b2> (4) - *M* src/lat/minimize-lattice.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-3630d7c7d4773e706652a7531c30f380bf279a2115c743c514333c4d2fad3d61> (2) - *M* src/lat/push-lattice.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-d941706d70a1fc3e2361eec9a05f53c95def3f3a3265fd551f10206a8a20d85b> (4) - *M* src/lat/sausages.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-6090dc2d42cd888fe45715a3855828a40b19594469cda957daca05e253928ae9> (2) - *M* src/nnet3/nnet-batch-compute.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-671613d156c376f99de26fda93d5ca1ece2512fd8f33f5d3eb057f2ba9e6c51f> (2) - *M* src/online/online-tcp-source.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-a8fcfc9a9dd43a08562bb46fbfac70836f3b6c4a8344cd4fbe212db9571f9189> (2) - *M* src/rnnlm/rnnlm-test-utils.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-a10c74bb73b82fe84df0f0db3da1710bf9e9f3e791f775569111bf0786a0470d> (2) - *M* src/tree/tree-renderer.cc <https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-b2119935db1c8759ce969fbd5119cab86135ef190279c89fa3a3dfd297016ea0> (4) Patch Links: - https://github.com/kaldi-asr/kaldi/pull/4829.patch - https://github.com/kaldi-asr/kaldi/pull/4829.diff — Reply to this email directly, view it on GitHub <#4829>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYXYVA454PVDH2RAWZH3WY6TRTANCNFSM6AAAAAAVF7GKME> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[georgthegreat]

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]

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 <#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 <#4829 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYX7E73T5GZNNQEHNDVDWY6U2JANCNFSM6AAAAAAVF7GKME> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jtrmal]

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

[galv]

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]

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]

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

[mmcauliffe]

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: conda-forge/kaldi-feedstock#38, but it'd be ideal to have these changes upstream.

[danpovey]

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

[kkm000]

Superseding: #4716

[kkm000]

@danpovey

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

Dan, just slap the label stale-exclude Stale bot ignore this issue 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. :(

[jtrmal]

We merged the updated version #4716 but thank you so much for providing this POC

[h-vetinari]

We merged the updated version #4716 but thank you so much for providing this POC

You probably mean #4927? In any case, great to see this merged!

[jtrmal]

yeah, apologies, I need more coffee :)