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

ANSI escape sequenceの出力に制限をかける #386

Merged
merged 24 commits into from
Jan 29, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 19, 2023

内容

一般的なアプリケーションのように、出力がファイルにリダイレクトされていたりTERM=dumbとなっているときにはANSI escape sequenceを出さないようにします。

関連 Issue

Fixes #385.

その他

image

image

@qryxip qryxip changed the title ASCI escape sequenceの出力に制限をかける ANSI escape sequenceの出力に制限をかける Jan 19, 2023
@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2023

こっちの問題ですが、もしかしてCP932のターミナルだったら同様に色を止めた方がよい?
PowerShellだったら大丈夫だった記憶があるのですが...

https://twitter.com/chagara_r/status/1616110499808313346

@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2023

VV_OUTPUT_LOG_UTF8によるTextIOWrapper.reconfigure(encoding="utf-8")を入れたらどうなるかがツイートからはわかりませんが、多分駄目な気はします。ctypesには影響しなさそう。

@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2023

あとVOICEVOX ENGINEのことはよくわかっていないのですが、ENGINEが悪いわけではないと思います。コマンドプロンプトのことを考えたらWindowsではそもそもANSIなんか出さずにWin32で色を出すべきで、coloramaもそうしているので。

@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2023

そう考えるとやるべきなのはfwdansiを使い、tracing-subscriberからのANSI escape sequenceをWin32の色に変換することなのですが、そこまでやるべきかどうか...

(追記) 10行未満くらいの追加で収まりそうではあるんですが

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 19, 2023

まあこのリポジトリが取り組むべき本質ではないと思うので、手っ取り早くとにかく色を付けるのをなくすのが良いのかなと思いました!
メンテナンスも大変そうなので・・・。

@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2023

ENABLE_VIRTUAL_TERMINAL_PROCESSINGをWin32から問い合わせるようにしました。

@qryxip
Copy link
Member Author

qryxip commented Jan 22, 2023

これ、よく考えたら

手っ取り早くとにかく色を付けるのをなくす

と+1-0行の変更でいいんですよね。今のPRの状態でちゃんと動作するとは思いますが、単に色を無効化する方がいいかもしれません。

@Hiroshiba
Copy link
Member

なるほどです!!ちなみにどこかに設定がある感じでしょうか 👀

@qryxip
Copy link
Member Author

qryxip commented Jan 23, 2023

このPRの.with_ansi(の部分ですね。ここをfalseにすると色が止まります。

@Hiroshiba
Copy link
Member

なるほどです!!手っ取り早くそれもありなのかなと思いました!!
デバッグ時は色があったほうが見やすそう感はあるので、簡単にできるならビルド時だけオフとかもありかなと思いました!

@qryxip
Copy link
Member Author

qryxip commented Jan 26, 2023

リリースビルドの判定ではないですが、コマンドプロンプトだけ弾くようにしました。

この変更量とコメントならどうでしょうか?

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

うーん、正直全くよくわかってないのですが、LGTMで!!

@qryxip qryxip requested a review from qwerty2501 January 28, 2023 14:15
@PickledChair
Copy link
Member

ぱっと見では、現時点のコードで適切そうな印象です。また私の環境(macOS)で試したところでは、意図通りに動いていそうでした。以前のコアと今回の PR のコア双方について、エンジン経由で使ってみて得られた出力を比較してみました(python run.py &> log.txt で得られた log.txt の内容の比較)。

以前のコア:

...
�[2m2023-01-29T10:50:42.074395Z�[0m �[32m INFO�[0m �[2monnxruntime::onnxruntime�[0m�[2m:�[0m "Removing initializer \'636\'. It is not used by any node and should be removed from the model."
�[2m2023-01-29T10:50:42.074491Z�[0m �[32m INFO�[0m �[2monnxruntime::onnxruntime�[0m�[2m:�[0m "Removing initializer \'640\'. It is not used by any node and should be removed from the model."
�[2m2023-01-29T10:50:42.074513Z�[0m �[32m INFO�[0m �[2monnxruntime::onnxruntime�[0m�[2m:�[0m "Removing initializer \'644\'. It is not used by any node and should be removed from the model."
...

この PR のコア:

...
2023-01-29T10:53:06.014865Z  INFO onnxruntime::onnxruntime: "Removing initializer \'636\'. It is not used by any node and should be removed from the model."
2023-01-29T10:53:06.014937Z  INFO onnxruntime::onnxruntime: "Removing initializer \'640\'. It is not used by any node and should be removed from the model."
2023-01-29T10:53:06.014954Z  INFO onnxruntime::onnxruntime: "Removing initializer \'644\'. It is not used by any node and should be removed from the model."
...

Windows については未検証です……。どなたか確認していただければ幸いです。

@Hiroshiba
Copy link
Member

@PickledChair
確認ありがとうございます!!
まあもしwindowsで意図通りに動かなかったとしても致命的な問題にはならないかなと思うのでマージでも大丈夫かなと・・・!

@sevenc-nanashi
Copy link
Member

Windows環境で検証してみます

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented Jan 29, 2023

image
検証できました!

検証情報
require "voicevox"

puts "Voicevox::VERSION: #{Voicevox::VERSION}"
puts "Voicevox.core_version: #{Voicevox.core_version}"

Voicevox.new("./voicevox_core/open_jtalk_dic_utf_8-1.11")
E:\voicevox-project\voicevox.rb>SET RUBY_DLL_PATH=E:/voicevox-project/voicevox_core/target/release
E:\voicevox-project\voicevox.rb>bundle exec ruby __gi_test.rb > __gi_test1.txt 2>&1
E:\voicevox-project\voicevox.rb>SET RUBY_DLL_PATH=./voicevox_core
E:\voicevox-project\voicevox.rb>bundle exec ruby __gi_test.rb > __gi_test2.txt 2>&1

E:/voicevox-project/voicevox_core/target/releaseの中にonnxruntime.dllやmodelをコピー。

@PickledChair
Copy link
Member

@sevenc-nanashi

なるほどです、検証ありがとうございます!(Ruby wrapper も検証用途に便利ですね)。ちなみになのですが、PowerShell に出力を流すときは文字色がつきそうでしたでしょうか? 👀 (今回の PR は PowerShell のことも考慮しているようなので、一応お聞きしたいと思いました)macOS では iTerm2 ではこれまで通り色がつきました。

@sevenc-nanashi
Copy link
Member

image
文字色でました。

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sevenc-nanashi Windows での検証ありがとうございました、助かりました!)

@PickledChair
Copy link
Member

これも @qwerty2501 さんにレビュー依頼が行っていますが、多分問題ないと思うのでマージします……!

@PickledChair PickledChair merged commit aa0bbf1 into VOICEVOX:main Jan 29, 2023
@Hiroshiba Hiroshiba removed the request for review from qwerty2501 January 29, 2023 15:40
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.

tracing-subscriberが問答無用でANSI escape sequenceを出す
4 participants