{"action":"created","issue":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/116","labels_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/116/labels{/name}","comments_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/116/comments","events_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/116/events","html_url":"https://github.com/kaldi-asr/kaldi/pull/116","id":104546736,"number":116,"title":"Add faster-rnnlm rescoring to librispeech","user":{"login":"akhti","id":7470747,"avatar_url":"https://avatars.githubusercontent.com/u/7470747?v=3","gravatar_id":"","url":"https://api.github.com/users/akhti","html_url":"https://github.com/akhti","followers_url":"https://api.github.com/users/akhti/followers","following_url":"https://api.github.com/users/akhti/following{/other_user}","gists_url":"https://api.github.com/users/akhti/gists{/gist_id}","starred_url":"https://api.github.com/users/akhti/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/akhti/subscriptions","organizations_url":"https://api.github.com/users/akhti/orgs","repos_url":"https://api.github.com/users/akhti/repos","events_url":"https://api.github.com/users/akhti/events{/privacy}","received_events_url":"https://api.github.com/users/akhti/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2015-09-02T18:43:41Z","updated_at":"2015-09-03T22:28:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/pulls/116","html_url":"https://github.com/kaldi-asr/kaldi/pull/116","diff_url":"https://github.com/kaldi-asr/kaldi/pull/116.diff","patch_url":"https://github.com/kaldi-asr/kaldi/pull/116.patch"},"body":"Add installation script for faster-rnnlm\r\nUpdate rescoring results to compare HS and NCE\r\n\r\nThe pull request was 'anounced' in issue #90 \r\n\r\n"},"comment":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/comments/137590839","html_url":"https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137590839","issue_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/116","id":137590839,"user":{"login":"chenguoguo","id":11788179,"avatar_url":"https://avatars.githubusercontent.com/u/11788179?v=3","gravatar_id":"","url":"https://api.github.com/users/chenguoguo","html_url":"https://github.com/chenguoguo","followers_url":"https://api.github.com/users/chenguoguo/followers","following_url":"https://api.github.com/users/chenguoguo/following{/other_user}","gists_url":"https://api.github.com/users/chenguoguo/gists{/gist_id}","starred_url":"https://api.github.com/users/chenguoguo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chenguoguo/subscriptions","organizations_url":"https://api.github.com/users/chenguoguo/orgs","repos_url":"https://api.github.com/users/chenguoguo/repos","events_url":"https://api.github.com/users/chenguoguo/events{/privacy}","received_events_url":"https://api.github.com/users/chenguoguo/received_events","type":"User","site_admin":false},"created_at":"2015-09-03T22:28:30Z","updated_at":"2015-09-03T22:28:30Z","body":"I had a look at the scripts, and I think it's probably easier to have\nrnnlm_ver variable in the script (as the script does now), so that we know\nwhich tool we are using. IIRC Yandex's version and Mikolov's version have\ndifferent model format, although their command line arguments are mostly\nthe same. And Mikolov's version works better at least on the Aspire task I\ntested.\n\nGuoguo\n--\n\nOn Thu, Sep 3, 2015 at 6:20 PM, Daniel Povey <notifications@github.com>\nwrote:\n\n> I'd rather find a solution that everyone is happy with.\n> Yenda, I don't think it was absolutely clear to us what you were proposing\n> to do. Were you planning to just have the call 'rnnlm' and rely on the\n> fact that it's in the path via env.sh? Or are you now saying that you want\n> the env.sh to have some other variable (which would store what? The\n> location of the 'bin' directory?)\n>\n> In any case an issue with those approaches is that a person can\n> legitimately have different setups with the three different versions of the\n> RNNLM tools, e.g. different versions of the training script train RNNLMs\n> with different versions of the tools; I'm not sure that their training\n> command lines are even compatible. When doing RNNLM rescoring, I believe\n> you have to rescore with the correct RNNLM tool that matches the model you\n> trained. Certainly that is the case if we are comparing Mikolov's tool to\n> the Yandex ones; I don't know if the Yandex ones have the same model\n> format. So your proposal might just cause crashes.\n> I know this whole situation is far from ideal.\n>\n> If the newest RNNLM tool is always at least as good at Mikolov's one, then\n> one solution would be to change everything to just target that one tool,\n> but this would involve someone testing the old setups such as WSJ, which\n> might take some time.\n>\n>\n> Dan\n>\n>\n>\n> On Thu, Sep 3, 2015 at 6:09 PM, jtrmal <notifications@github.com> wrote:\n>\n> > Ok then about the versioning, even though if there is such a strong call\n> > for determinism, we should check out a specific version of the tool (as\n> we\n> > do for irstlm) and I believe it should be done other way.\n> > Yes, the run_* script should only check if the given binary is present\n> (and\n> > not install it -- we had a discussion a few days back, that some user are\n> > discouraged to have their on tools/ or perhaps whole kaldi directory) --\n> > and for that either the RNNLMHS variable or you can even have RNNLMHSVER\n> > set to something meaningful, as I assume there can be development in what\n> > that package does. As for deciding of the version of rnnlm I'm okay with\n> > keeping that switch there.\n> > Again, I don't see why adding the environment variables into the env.sh\n> is\n> > such a big problem. I think I've spent enough time on maintaining the\n> tools\n> > directory and fixing the issues stemming from the fact that people invent\n> > various \"smart\" things in their scripts how to detect if the given tool\n> is\n> > installed or not, to know this practice is helpful in a long term. I\n> didn't\n> > hear any arguments why the thing I'm proposing would be inadequate (or\n> just\n> > plain stupid).\n> >\n> > But as I'm getting tired of this discussion, feel free to accept the PR\n> and\n> > I'll just create my personal list of things I will refuse to provide\n> > help/support about.\n> > y\n> >\n> > On Thu, Sep 3, 2015 at 5:43 PM, Daniel Povey <notifications@github.com>\n> >\n> > wrote:\n> >\n> > > Well, I don't want to be obsessive about it but I think there is a\n> > certain\n> > > > code style for recipes (albeit not defined as precisely as for C++\n> > code)\n> > > > and we should be enforcing it, because it will be mostly us who will\n> > have\n> > > > do the regular maintenance.\n> > > > Yes, it makes sense to keep both installation scripts in there for\n> now,\n> > > but\n> > > > in the long term I think we should get rid of the old rnnlm code in\n> > favor\n> > > > if the new one (as the new one seems supported better -- or that is\n> my\n> > > > hope). And I think that \"magic\" with the selection of the version of\n> > > > rnnlm in the run_rnnlm.sh is really not nice. It only complicates\n> > things\n> > > in\n> > > > the long term.\n> > > > So I don't see the reason why is there such a strong push against\n> > adding\n> > > > RNNLMHS=$KALDI_ROOT/tools/rnnlm-hs\n> > > > PATH=$PATH:$RNNLMHS\n> > > > (or whatever) into tools/env.sh\n> > > >\n> > >\n> > > I think that the script should not be installing those things but\n> should\n> > > instead be checking that it's installed and maybe prompting you to\n> > install\n> > > it and then dying; this is the normal pattern and it seems better from\n> a\n> > > security standpoint. However, it looks like the existing RNNLM\n> rescoring\n> > > script, that's been there for a while, already takes the --rnnlm-ver\n> > > argument, so I think that should probably stay there for now unless\n> > someone\n> > > wants to redo all that stuff. Actually I prefer to have it explicit in\n> > the\n> > > script which version of those tools is being used, because otherwise we\n> > > have a situation where the same script can produce different results\n> > > depending which installation scripts have been previously run. This\n> seems\n> > > like a kind of nondeterminism. So I'm OK with the overall way the\n> script\n> > > works.\n> > >\n> > > Dan\n> > >\n> > >\n> > >\n> > >\n> > > >\n> > > > On Thu, Sep 3, 2015 at 4:42 PM, Daniel Povey <\n> notifications@github.com\n> > >\n> > > > wrote:\n> > > >\n> > > > > I think I am happy with the script as it is currently, just using a\n> > > > > different name. At least it's clear what's going on. I would be\n> happy\n> > > to\n> > > > > merge this pull request. @jtrmal <https://github.com/jtrmal> ?\n> > > > >\n> > > > > Also, @chenguoguo <https://github.com/chenguoguo> @hainan-xv\n> > > > > <https://github.com/hainan-xv>, please look into this and see if\n> you\n> > > > > think this has any implications for your RNNLM rescoring work.\n> > > > >\n> > > > > —\n> > > > > Reply to this email directly or view it on GitHub\n> > > > > <\n> https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137568828\n> > >.\n> > > >\n> > > > >\n> > > >\n> > > > —\n> > > > Reply to this email directly or view it on GitHub\n> > > > <https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137580467\n> >.\n> > >\n> > > >\n> > >\n> > > —\n> > > Reply to this email directly or view it on GitHub\n> > > <https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137582431>.\n> >\n> > >\n> >\n> > —\n> > Reply to this email directly or view it on GitHub\n> > <https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137587819>.\n>\n> >\n>\n> —\n> Reply to this email directly or view it on GitHub\n> <https://github.com/kaldi-asr/kaldi/pull/116#issuecomment-137589555>.\n>\n"}}