Support for both OpenFST 1.7.3 and 1.8.2 by jtrmal · Pull Request #4927 · kaldi-asr/kaldi

jtrmal · 2024-07-25T09:31:54Z

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 · 2024-07-25T10:23:18Z

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

csukuangfj · 2024-07-25T10:42:53Z

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 · 2024-07-25T10:44:38Z

HINT:

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

jtrmal · 2024-07-25T13:33:53Z

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 · 2024-07-25T16:24:42Z

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

csukuangfj · 2024-07-31T02:44:22Z

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

Have you fixed it?

jtrmal · 2024-07-31T05:28:05Z

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 <#4927 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYXZ5U7W6XI4XAGESXYDZPBFRXAVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGUZTEMJTGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

kkm000 · 2024-08-28T09:06:07Z

          fst::FstPrinter<StdArc> fstprinter(phn2word, NULL, NULL, NULL, false, true, "\t");
-          fstprinter.Print(&std::cerr, "standard error");
+          printer_print(std::cerr, fstprinter, "standard error");
+          //fstprinter.Print(&std::cerr, "standard error");


Remove this?

kkm000 · 2024-08-28T09:09:03Z

+      fst::StdCompactAcceptorFst cfst(fst);
+      cfst.Write(os, write_options);


Is this pre-1.8 compatible? Should we go flat out 1.8+? We've had the version of OpenFST fixed pretty much forever.

kkm000 · 2024-08-28T09:11:27Z

+if [ $OPENFST_VER_NUM -lt 10800 ]; then
+  echo "CXXLANGVERSION = c++14"
+else
+  echo "CXXLANGVERSION = c++17"
+fi >> kaldi.mk


Can you think of a case when we would run into a pre-C++14 compiler? Maybe it's time to go C++17 unconditionally?

kkm000 · 2024-08-28T09:14:11Z

 #include "fstext/fst-test-utils.h"
 #include "base/kaldi-math.h"

+#include "fstext/openfst_compat.h"
+


Sort #includes alphabetically. Or, if openfst_compat.h requires being included the last, then add a comment to that effect.

kkm000 · 2024-08-28T09:18:03Z

+  for (SymbolTable::iterator iter = symtab.begin();
+      iter != symtab.end();
+      ++iter) {


Ah, that would have been for (SymbolTable::iterator iter : symtab) in C++17 ;-)

better to use auto with range based for here.

You can argue with @danpovey about this.🙂

In this particular case, I agree with you. My own rule of thumb is, if the real or conceptual type is obvious, use either auto; otherwise, the type must be explicit. In this case, the conceptual type, namely the iterator, is obvious. But Dan wants explicit types, with an exception for extreme ugliness of their explicit spelling, I believe.

kkm000 · 2024-08-28T11:59:46Z

+#ifndef KALDI_FSTEXT_OPENFST_COMPAT_H
+#define KALDI_FSTEXT_OPENFST_COMPAT_H
+
+


#include <fst/flags.h> is for the one below, only if you like it better.

<utility> is for std::forward. std::string is used in printer_print.

Suggested change

#include <fst/arc-map.h>

#include <fst/flags.h>

#include <string>

#include <utility>

kkm000 · 2024-08-28T12:18:04Z

+
+
+#if OPENFST_VER < 10800
+#define FST_FLAGS_fst_weight_separator FLAGS_fst_weight_separator


This may be a bit C++ish than the macros (https://godbolt.org/z/n5roPYzET):

Suggested change

#define FST_FLAGS_fst_weight_separator FLAGS_fst_weight_separator

DECLARE_string(fst_weight_separator);

inline constexpr std::string& FST_FLAGS_fst_weight_separator =

FLAGS_fst_weight_separator;

To your liking, anyway. Either is fine.

kkm000 · 2024-08-28T12:21:54Z

+  return ArcMap(std::forward<Args>(args)...);
+}
+
+using MapFstOptions=ArcMapFstOptions;


Suggested change

using MapFstOptions=ArcMapFstOptions;

using MapFstOptions = ArcMapFstOptions;

I suggest that clang-format should be used to format the file automagically.

@csukuangfj, I suggest the same… I mean, feel free… 😁

Guys, the focus here is on getting something committed that won't break things, we're in maintenance mode so it's not the time to worry too much about how it looks.

kkm000 · 2024-08-28T12:23:01Z

  const char *prefix_ptr = prefix.c_str();
  size_t prefix_len = strlen(prefix_ptr);  // allowed to be zero but not encouraged.
+#if OPENFST_VER >= 10800
+  for (SymbolTable::iterator siter = symTable->begin(); siter != symTable->end(); ++siter) {


Past 80.

Suggested change

for (SymbolTable::iterator siter = symTable->begin(); siter != symTable->end(); ++siter) {

for (SymbolTable::iterator siter = symTable->begin();

siter != symTable->end(); ++siter) {

kkm000 · 2024-08-28T12:48:27Z

 namespace kaldi {

-typedef kaldi::int32 int32;
+typedef int32 int32;


This looks weird. Is it still required? IIRC, this was a workaround for a bug in MS compiler 10 years ago. I may be totally wrong, tho.

I'd say it's likely not required, probably this is from a sed replace operation.

kkm000 · 2024-08-28T12:55:18Z

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 · 2024-09-05T11:25:15Z

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 · 2024-09-05T12:55:18Z

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 · 2024-09-05T15:21:42Z

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 · 2024-09-07T14:46:28Z

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 · 2024-09-08T07:32:16Z

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 · 2024-09-09T08:53:29Z

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 <#4927> into master. — Reply to this email directly, view it on GitHub <#4927 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYX6VSMOCVKPEZSILL63ZVP4RVAVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGE3TGNBXGI2TANI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

danpovey · 2024-09-09T11:41:23Z

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 <#4927> into master. > > — > Reply to this email directly, view it on GitHub > <#4927 (comment)>, 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 <#4927 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZFLO3DB5NRE4XUOV3NEJDZVVOZ7AVCNFSM6AAAAABLOEXNFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZXGUZDIMBSGU> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

kkm000 · 2024-09-09T11:46:36Z

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...

georgthegreat and others added 5 commits July 24, 2024 09:21

Support openfst-1.7.6

3f170d4

Support openfst-1.8.0

e460d8a

Support openfst-1.8.1

5ccce55

Support openfst-1.8.2

9a213be

add support for later openfst versions

57efb6b

fix the non-compiling tests, thx to @csukuangfj

a7fcc2d

kkm000 reviewed Aug 28, 2024

View reviewed changes

kkm000 mentioned this pull request Sep 3, 2024

Fix build with latest version of openfst #4096

Closed

danpovey merged commit 13441d0 into kaldi-asr:master Sep 8, 2024

h-vetinari mentioned this pull request Sep 16, 2024

[DO NOT MERGE] Support openfst 1.8.2 #4829

Closed

h-vetinari mentioned this pull request Oct 14, 2024

Update build/version conda-forge/kaldi-feedstock#51

Closed

csukuangfj mentioned this pull request May 28, 2025

Build Error when using GCC 14 kkm000/openfst#59

Open

		fst::StdCompactAcceptorFst cfst(fst);
		cfst.Write(os, write_options);

		#ifndef KALDI_FSTEXT_OPENFST_COMPAT_H
		#define KALDI_FSTEXT_OPENFST_COMPAT_H

+#include <fst/arc-map.h>
+#include <fst/flags.h>
+#include <string>
+#include <utility>



		#if OPENFST_VER < 10800
		#define FST_FLAGS_fst_weight_separator FLAGS_fst_weight_separator

-#define FST_FLAGS_fst_weight_separator FLAGS_fst_weight_separator
+DECLARE_string(fst_weight_separator);
+inline constexpr std::string& FST_FLAGS_fst_weight_separator =
+    FLAGS_fst_weight_separator;

	using MapFstOptions=ArcMapFstOptions;
	using MapFstOptions = ArcMapFstOptions;

	for (SymbolTable::iterator siter = symTable->begin(); siter != symTable->end(); ++siter) {
	for (SymbolTable::iterator siter = symTable->begin();
	siter != symTable->end(); ++siter) {

Conversation

jtrmal commented Jul 25, 2024

Uh oh!

jtrmal commented Jul 25, 2024

Uh oh!

csukuangfj commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csukuangfj commented Jul 25, 2024

Uh oh!

jtrmal commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtrmal commented Jul 25, 2024

Uh oh!

csukuangfj commented Jul 31, 2024

Uh oh!

jtrmal commented Jul 31, 2024 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkm000 commented Aug 28, 2024

Uh oh!

danpovey commented Sep 5, 2024

Uh oh!

danpovey commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danpovey commented Sep 5, 2024

Uh oh!

danpovey commented Sep 7, 2024

Uh oh!

danpovey commented Sep 8, 2024

Uh oh!

jtrmal commented Sep 9, 2024 via email

Uh oh!

danpovey commented Sep 9, 2024 via email

Uh oh!

kkm000 commented Sep 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

csukuangfj commented Jul 25, 2024 •

edited

Loading

jtrmal commented Jul 25, 2024 •

edited

Loading

danpovey commented Sep 5, 2024 •

edited

Loading