kaldi-asr / kaldi

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

mmseg not support for python3 under hkust #3238

Closed zh794390558 closed 5 years ago

danpovey commented 5 years ago

@naxingyu do you have any suggestion for what to do?

naxingyu commented 5 years ago

I'll check.

On Thu, Apr 18, 2019 at 3:12 AM Daniel Povey notifications@github.com wrote:

@naxingyu https://github.com/naxingyu do you have any suggestion for what to do?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484223338, or mute the thread https://github.com/notifications/unsubscribe-auth/ADKpxHuQxMdW8w9seVcc21nmw8_8VQr8ks5vh3IXgaJpZM4c0hkz .

naxingyu commented 5 years ago

@zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

danpovey commented 5 years ago

Obviously you would have to invoke python as python3 if you were relying on it being python 3.

On Wed, Apr 17, 2019 at 7:33 PM Xingyu Na notifications@github.com wrote:

@zh794390558 https://github.com/zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484332820, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFLOZJU63WI32LKZ47HT3PQ7MXTANCNFSM4HGSDEZQ .

naxingyu commented 5 years ago

Yes, the repo is just for test. If it works, I'll change it to accept both py2&3.

On Thu, Apr 18, 2019 at 10:38 AM Daniel Povey notifications@github.com wrote:

Obviously you would have to invoke python as python3 if you were relying on it being python 3.

On Wed, Apr 17, 2019 at 7:33 PM Xingyu Na notifications@github.com wrote:

@zh794390558 https://github.com/zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484332820, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAZFLOZJU63WI32LKZ47HT3PQ7MXTANCNFSM4HGSDEZQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484333819, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZKTRBEGRHBKY3NNIIEJNLPQ7NKZANCNFSM4HGSDEZQ .

naxingyu commented 5 years ago

When I have time, I'll fork the original mmseg module and make it work for both py2&3.

On Thu, Apr 18, 2019 at 10:51 AM Xingyu Na asr.naxingyu@gmail.com wrote:

Yes, the repo is just for test. If it works, I'll change it to accept both py2&3.

On Thu, Apr 18, 2019 at 10:38 AM Daniel Povey notifications@github.com wrote:

Obviously you would have to invoke python as python3 if you were relying on it being python 3.

On Wed, Apr 17, 2019 at 7:33 PM Xingyu Na notifications@github.com wrote:

@zh794390558 https://github.com/zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484332820 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAZFLOZJU63WI32LKZ47HT3PQ7MXTANCNFSM4HGSDEZQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484333819, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZKTRBEGRHBKY3NNIIEJNLPQ7NKZANCNFSM4HGSDEZQ .

danpovey commented 5 years ago

Actually I think Kaldi currently requires both python3 and python2.7, so a quicker fix might simply be to invoke python as python2.7. However in the longer term we do want to only depend on python3; python2.7 is required only for sequitur g2p right now, IIRC.

On Wed, Apr 17, 2019 at 7:53 PM Xingyu Na notifications@github.com wrote:

When I have time, I'll fork the original mmseg module and make it work for both py2&3.

On Thu, Apr 18, 2019 at 10:51 AM Xingyu Na asr.naxingyu@gmail.com wrote:

Yes, the repo is just for test. If it works, I'll change it to accept both py2&3.

On Thu, Apr 18, 2019 at 10:38 AM Daniel Povey notifications@github.com wrote:

Obviously you would have to invoke python as python3 if you were relying on it being python 3.

On Wed, Apr 17, 2019 at 7:33 PM Xingyu Na notifications@github.com wrote:

@zh794390558 https://github.com/zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484332820 , or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAZFLOZJU63WI32LKZ47HT3PQ7MXTANCNFSM4HGSDEZQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484333819 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAZKTRBEGRHBKY3NNIIEJNLPQ7NKZANCNFSM4HGSDEZQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484336578, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFLO7HCRW6XTADDJUBFVLPQ7PCLANCNFSM4HGSDEZQ .

naxingyu commented 5 years ago

You are right. I'll look into it.

On Thu, Apr 18, 2019 at 10:57 AM Daniel Povey notifications@github.com wrote:

Actually I think Kaldi currently requires both python3 and python2.7, so a quicker fix might simply be to invoke python as python2.7. However in the longer term we do want to only depend on python3; python2.7 is required only for sequitur g2p right now, IIRC.

On Wed, Apr 17, 2019 at 7:53 PM Xingyu Na notifications@github.com wrote:

When I have time, I'll fork the original mmseg module and make it work for both py2&3.

On Thu, Apr 18, 2019 at 10:51 AM Xingyu Na asr.naxingyu@gmail.com wrote:

Yes, the repo is just for test. If it works, I'll change it to accept both py2&3.

On Thu, Apr 18, 2019 at 10:38 AM Daniel Povey < notifications@github.com> wrote:

Obviously you would have to invoke python as python3 if you were relying on it being python 3.

On Wed, Apr 17, 2019 at 7:33 PM Xingyu Na notifications@github.com wrote:

@zh794390558 https://github.com/zh794390558 for quick fix, you could try replace the module with this: https://github.com/bxshi/py3mmseg The readme says it only works for python3.* and it only tested in python 3.3, and it hasn't been updated since 4 years ago. I'm really sorry that I don't have time to test this. If it works, it would be great if you could create a PR. Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484332820 , or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAZFLOZJU63WI32LKZ47HT3PQ7MXTANCNFSM4HGSDEZQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484333819 , or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAZKTRBEGRHBKY3NNIIEJNLPQ7NKZANCNFSM4HGSDEZQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484336578, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAZFLO7HCRW6XTADDJUBFVLPQ7PCLANCNFSM4HGSDEZQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484337205, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZKTRAMNBFTZFZ4PCCZUQ3PQ7PQTANCNFSM4HGSDEZQ .

kkm000 commented 5 years ago

FWIW, the configure still sets python 2.7 as the default. https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/tools/extras/check_dependencies.sh#L117-L128

kkm000 commented 5 years ago

On the same point, when I hacked the train.py scripts, I cried for the python3 f'...' interpolated strings. The code using .format() is supremely unreadable there.

danpovey commented 5 years ago

Most of our recent scripts that use python explicitly invoke python3. The train.py scripts should be among these. Making python the default is mostly to increase the changes of misc things in egs directories working.

On Thu, Apr 18, 2019 at 11:11 AM kkm (aka Kirill Katsnelson) < notifications@github.com> wrote:

On the same point, when I hacked the train.py scripts, I cried for the python3 f'...' interpolated strings. The code using .format() is supremely unreadable there.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484622223, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFLOYJLS7ILW5OLB3ULQDPRC2VJANCNFSM4HGSDEZQ .

naxingyu commented 5 years ago

There are cases where people installed kaldi before installing python3, and check_dependency is never touched since it's not required for compile src.

On Thu, Apr 18, 2019 at 6:04 PM kkm (aka Kirill Katsnelson) < notifications@github.com> wrote:

FWIW, the configure still sets python 2.7 as the default.

https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/tools/extras/check_dependencies.sh#L117-L128

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484436245, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZKTRBMGOMIIQTGDJT3TNDPRBBT7ANCNFSM4HGSDEZQ .

kkm000 commented 5 years ago

@danpovey

our recent scripts that use python explicitly invoke python3. The train.py scripts should be among these.

Trains scripts use py2 (via its being the implicit default). https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/egs/wsj/s5/steps/nnet3/chain/train.py#L1-L2

My hands were itchy to switch them to py3, but I have been getting 5 hours of sleep a day for the last month, and there is already stuff not fitting on my plate with Kaldi, too--I'm assigning myself more often than closing issues. And it won't be just flip-the-switch thing; I converted very simple self-test to unit tests for just two existing functions on one of the training libs (and unit tests I can run with either python); one of these had runtime errors under py3. It will take quite a few methodically spent hours to convert these scripts (and get rid of .format()s. I wanted to, but I am just very busy with other work, guys.

I like this type of refactoring, like replacing the formats, it leaves my mind mostly idle, and by the end of a day or two spent refactoring code I often suddenly "just know" a solution to stuff I put away "to sleep over it". It's a dreamy work. But I am really strained on my time currently. If this waits 3-4 weeks, I am a taker.

@naxingyu:

There are cases where people installed kaldi before installing python3

Not any more since an explicit dependency was added to check_dependencies.sh. We require both 2.7 and 3 installed, and 2.7 being the default. And the script is run by make, so there is not an upgrade path around the requirement. https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/tools/extras/check_dependencies.sh#L89-L100

danpovey commented 5 years ago

OK thanks. No hurry.

On Thu, Apr 18, 2019 at 7:53 PM kkm (aka Kirill Katsnelson) < notifications@github.com> wrote:

@danpovey https://github.com/danpovey

our recent scripts that use python explicitly invoke python3. The train.py scripts should be among these.

Trains scripts use py2 (via its being the implicit default).

https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/egs/wsj/s5/steps/nnet3/chain/train.py#L1-L2

My hands were itchy to switch them to py3, but I have been getting 5 hours of sleep a day for the last month, and there is already stuff not fitting on my plate with Kaldi, too--I'm assigning myself more often than closing issues. And it won't be just flip-the-switch thing; I converted very simple self-test to unit tests for just two existing functions on one of the training libs (and unit tests I can run with either python); one of these had runtime errors under py3. It will take quite a few methodically spent hours to convert these scripts (and get rid of .format()s. I wanted to, but I am just very busy with other work, guys.

I like this type of refactoring, like replacing the formats, it leaves my mind mostly idle, and by the end of a day or two spent refactoring code I often suddenly "just know" a solution to stuff I put away "to sleep over it". It's a dreamy work. But I am really strained on my time currently. If this waits 3-4 weeks, I am a taker.

@naxingyu https://github.com/naxingyu:

There are cases where people installed kaldi before installing python3

Not any more since an explicit dependency was added to check_dependencies.sh. We require both 2.7 and 3 installed, and 2.7 being the default. And the script is run by make, so there is not an upgrade path around the requirement.

https://github.com/kaldi-asr/kaldi/blob/299b111d05e9f628767773ca924c7a9c48f29603/tools/extras/check_dependencies.sh#L89-L100

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484751592, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZFLO44WHA3YIPAGSOBFCTPREX3NANCNFSM4HGSDEZQ .

kkm000 commented 5 years ago

Fixed in #3244, 299b111.

jtrmal commented 5 years ago

Just a comment - we shouldn't take mmseg as a dogma. There might be another self-contained segmenter )maybe even a better one). It would be more work to get it running with something else than just call the correct python 🙂 I don't argue about that .

On Fri, Apr 19, 2019 at 01:10 kkm (aka Kirill Katsnelson) < notifications@github.com> wrote:

Fixed in #3244 https://github.com/kaldi-asr/kaldi/pull/3244, 299b111 https://github.com/kaldi-asr/kaldi/commit/299b111d05e9f628767773ca924c7a9c48f29603 .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kaldi-asr/kaldi/issues/3238#issuecomment-484769444, or mute the thread https://github.com/notifications/unsubscribe-auth/ACUKYX6K66DDBWSUPZ4IGNTPRFH3VANCNFSM4HGSDEZQ .