search

kaldi-asr / kaldi

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

Support for both OpenFST 1.7.3 and 1.8.2 #4927

Closed jtrmal closed 3 days ago

jtrmal commented 1 month ago

I added a thin compatibility layer (and a few ifdefs) By default the build infrastructure uses OpenFST-1.7.2 and there should be no change compared to current pipelines. It is however switch the openFst version while making /tools like so:

make clean && make -j 18 OPENFST_VERSION=1.8.3

and after that the version should be picked up by configure (also to be re-run) and flags will be appropriately defined. Will be grateful for reviews/testing --so far I tested only on Apple M3

For 1.8.3, not all tests will compile. There are two outstanding tests failing to compile with this error:

In file included from trivial-factor-weight-test.cc:23:
../fstext/trivial-factor-weight.h:393:14: error: no viable overloaded '='
  data->base = new StateIterator< TrivialFactorWeightFst<A, F> >(*this);
  ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
trivial-factor-weight-test.cc:139:46: note: in instantiation of member function 'fst::TrivialFactorWeightFst<fst::GallicArc<fst::ArcTpl<fst::TropicalWeightTpl<float>>>, fst::GallicFactor<int, fst::TropicalWeightTpl<float>>>::InitStateIterator' requested here
        typename Arc::Weight, GALLIC_LEFT> > fwfst(gallic_fst);
                                             ^
trivial-factor-weight-test.cc:213:10: note: in instantiation of function template specialization 'fst::TestFactor<fst::ArcTpl<fst::TropicalWeightTpl<float>>>' requested here
    fst::TestFactor<fst::StdArc>();
         ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:233:71: note: candidate function not viable: no known conversion from 'StateIterator<TrivialFactorWeightFst<GallicArc<ArcTpl<TropicalWeightTpl<float>, int, int>, fst::GALLIC_LEFT>, GallicFactor<int, TropicalWeightTpl<float>, fst::GALLIC_LEFT>>> *' to 'unique_ptr<StateIteratorBase<GallicArc<ArcTpl<TropicalWeightTpl<float>>>>>' for 1st argument
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(unique_ptr&& __u) _NOEXCEPT {
                                                                      ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:243:71: note: candidate template ignored: could not match 'unique_ptr<_Up, _Ep>' against 'StateIterator<TrivialFactorWeightFst<GallicArc<ArcTpl<TropicalWeightTpl<float>, int, int>, fst::GALLIC_LEFT>, GallicFactor<int, TropicalWeightTpl<float>, fst::GALLIC_LEFT>>> *'
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(unique_ptr<_Up, _Ep>&& __u) _NOEXCEPT {
                                                                      ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:268:71: note: candidate function not viable: no known conversion from 'StateIterator<TrivialFactorWeightFst<GallicArc<ArcTpl<TropicalWeightTpl<float>, int, int>, fst::GALLIC_LEFT>, GallicFactor<int, TropicalWeightTpl<float>, fst::GALLIC_LEFT>>> *' to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(nullptr_t) _NOEXCEPT {
                                                                      ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:127:59: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'StateIterator<TrivialFactorWeightFst<GallicArc<ArcTpl<TropicalWeightTpl<float>, int, int>, fst::GALLIC_LEFT>, GallicFactor<int, TropicalWeightTpl<float>, fst::GALLIC_LEFT>>> *' to 'const std::unique_ptr<fst::StateIteratorBase<fst::GallicArc<fst::ArcTpl<fst::TropicalWeightTpl<float>>>>>' for 1st argument
class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
                                                          ^
1 error generated.
make[1]: *** [trivial-factor-weight-test.o] Error 1

if anyone can help me figure out the issue, I will be grateful

cc: @georgthegreat @mmcauliffe

jtrmal commented 1 month ago

So, on linux the tests fail to compile with the same error. So I guess it's not just apple/clang weirdness.

csukuangfj commented 1 month ago

Let me help you.

You can change

../fstext/trivial-factor-weight.h:393:14

from

data->base = new StateIterator< TrivialFactorWeightFst<A, F> >(*this);

to

data->base.reset(new StateIterator< TrivialFactorWeightFst<A, F> >(*this));

By the way, if you need to support multiple openfst versions, then you can do

#if OPEN_FST_VERSION_XXX
data->base = new StateIterator< TrivialFactorWeightFst<A, F> >(*this);
#elif OPEN_FST_VERSION_YYY
data->base.reset(new StateIterator< TrivialFactorWeightFst<A, F> >(*this));
#endif
csukuangfj commented 1 month ago

HINT:

data->base was a raw pointer, but it is changed to a std::unique_ptr in openfst 1.8.3

jtrmal commented 1 month ago

The test is failing because of line src/fstextra/lattice-utils-test.cc:109 I modified it thusly


        // since semiring is idempotent, this should succeed too.
        std::cout << "Distance: " << ShortestDistance(cfst) << " x " << ShortestDistance(nbest_fst_1b) << std::endl;
        assert(ApproxEqual(ShortestDistance(cfst),
                           ShortestDistance(nbest_fst_1b)));

The output is:

Distance: 0,1.25,1_2 x 0,1.25,

lattice-utils-test: lattice-utils-test.cc:109: void fst::TestShortestPath() [with Weight = fst::LatticeWeightTpl<double>; Int = int]: Assertion `ApproxEqual(ShortestDistance(cfst), ShortestDistance(nbest_fst_1b))' failed.

NB: I was able to repro it on another machine/distro.

jtrmal commented 1 month ago

Full log from the test lattice-utils-test.testlog.txt

csukuangfj commented 1 month ago

Full log from the test lattice-utils-test.testlog.txt

Have you fixed it?

jtrmal commented 1 month ago

I didn't, yet, apologies. I might be able to do it next week. Or if you want to take a stab at it...? Y.

On Wed, Jul 31, 2024 at 04:44 Fangjun Kuang @.***> wrote:

Full log from the test lattice-utils-test.testlog.txt https://github.com/user-attachments/files/16380954/lattice-utils-test.testlog.txt

Have you fixed it?

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4927#issuecomment-2259532130, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYXZ5U7W6XI4XAGESXYDZPBFRXAVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGUZTEMJTGA . You are receiving this because you authored the thread.Message ID: @.***>

kkm000 commented 2 weeks ago

I can take a stab at it tomorrow… To be more precise, it's like 6 in the morning here, and it's still "today", so tomorrow can be any time within 24h from now...

danpovey commented 6 days ago

I had my wife check that it compiles for her and it was OK. I want to keep this moving or people will become dispirited from the slow progress, so I'll merge this and if there are problems we'll fix them later.

danpovey commented 6 days ago

Wait, I see there's a test failing, let me see if I can fix it. Edit: can't reproduce right now as we're having problems connecting to the outside from our servers.

danpovey commented 6 days ago

Does anyone have time to run the tests locally and see what the output of the failing test (lattice-utils-test I think) is? Our systems have some issues right now that make this difficult.

danpovey commented 4 days ago

The issue does not seem to happen with OpenFST 1.7.2. Trying with 1.8.3 which is what the failed test ran with.

danpovey commented 3 days ago

OK, I have not been able to reproduce the issue with openfst-1.8.3 because of this: /star-dan/kaldi/tools/openfst-1.8.3/missing: line 81: aclocal-1.16: command not found but let me just merge anyway because it looks like it doesn't hurt with earlier versions of OpenFST so nothing is broken that was working before. We can fix this later.

jtrmal commented 2 days ago

Weird, I cannot reproduce the issue either. I'm confused, but hey, at least it's working y.

On Sun, Sep 8, 2024 at 9:32 AM Daniel Povey @.***> wrote:

Merged #4927 https://github.com/kaldi-asr/kaldi/pull/4927 into master.

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4927#event-14173472505, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYX6VSMOCVKPEZSILL63ZVP4RVAVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGE3TGNBXGI2TANI . You are receiving this because you authored the thread.Message ID: @.***>

danpovey commented 2 days ago

we could perhaps add something to the github actions to cat the .testlog file to the screen on failing test?

On Mon, Sep 9, 2024 at 4:53 PM jtrmal @.***> wrote:

Weird, I cannot reproduce the issue either. I'm confused, but hey, at least it's working y.

On Sun, Sep 8, 2024 at 9:32 AM Daniel Povey @.***> wrote:

Merged #4927 https://github.com/kaldi-asr/kaldi/pull/4927 into master.

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4927#event-14173472505, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACUKYX6VSMOCVKPEZSILL63ZVP4RVAVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGE3TGNBXGI2TANI>

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/pull/4927#issuecomment-2337524025, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFLO3DB5NRE4XUOV3NEJDZVVOZ7AVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZXGUZDIMBSGU . You are receiving this because you modified the open/close state.Message ID: @.***>

kkm000 commented 2 days ago

I'll do my best to look at it today. As soon as I crawl out of the comms closet. Looks like I got me my Internet back...