Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update fstproject flag syntax #4947

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrikaro
Copy link

@andrikaro andrikaro commented Sep 27, 2024

root@2bd4628ac8ba:/opt/kaldi/egs/wsj/s5/utils/lang# /opt/kaldi/tools/openfst-1.8.3/bin/fstproject --help
Projects a transduction onto its input or output language.

  Usage: /opt/kaldi/tools/openfst-1.8.3/bin/fstproject [in.fst [out.fst]]

PROGRAM FLAGS:

  --project_type: type = std::string, default = "input"
  Side to project from, one of: "input", "output"

https://www.openfst.org/doxygen/fst/html/fstproject_8cc.html

I think there is a deprecated flag being used.

It seems that in some places it is up to date(on master it should be 139):

image

But there are 2300 with old syntax:

image

@jtrmal
Copy link
Contributor

jtrmal commented Oct 4, 2024

Thanks -- the only issue I currently don't know how to solve is that we aim to support sub 1.8 openfst too and this is breaking that. I understand now it's breaking the 1.8.x compatibility but...

@danpovey
Copy link
Contributor

danpovey commented Oct 4, 2024

Yeah the new maintainers of openfst don't have a lot of respect for back compatibility it seems.
I think the right fix would be something where beforehand we figure out the correct option by giving fstproject one option and seeing how it behaves. Might be necessary to use fstcompile to give it an empty fst, and check the return status. And of course check the codebase to see where we use fstproject and do the same everywhere.

@danpovey
Copy link
Contributor

danpovey commented Oct 5, 2024

Looks like you need to add back the "remove python2 installation" thing.. i thought I saw that in there beore.

@jtrmal
Copy link
Contributor

jtrmal commented Oct 6, 2024

yeah, I just removed it in the last commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants