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

Rustへの移行 #126

Merged
merged 21 commits into from
May 15, 2022
Merged

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented May 8, 2022

内容

voicevox_coreをRustに移植する話が配信でされていたのでおためしでrustへの移行をしてみるPR

その他

どうせ大きく変えるのであれば、非同期APIとかもやりたいのでdllのインターフェースも変えようかと考えてます

@qwerty2501 qwerty2501 force-pushed the feature/migrate_to_rust branch from 95623da to 5746d6a Compare May 8, 2022 17:04
@Hiroshiba
Copy link
Member

Hiroshiba commented May 9, 2022

機能を追加していくのはOSS的に見ると、あとから見返しがしやすかったり議論しやすかったりなので、別PRだと嬉しそうです!

あといきなりお1人で全部ガラッと変えるのは難しいかもなので、またVOICEVOX側にrust用のbranchを作って、そちらにPRしていく形も良いかもと思いました。
もしその方式で問題なければ、今のmainからrustブランチを作ろうと思います。

@qwerty2501
Copy link
Contributor Author

そうですね。branchがあると良さそうです。
とりあえずAPIについては #124 がmergeされる前提で、そこに合わせる感じにしようかと

@Hiroshiba
Copy link
Member

branch作りました! https://github.com/VOICEVOX/voicevox_core/tree/rust

APIに関して、良さそうに感じました。
機能に関しては、onnxruntimeを使ったり、DLLにバイナリを埋め込んだり、openjtalk使ったりとかなりいろいろあるので、一旦また簡単な実装だけでもPRいただけると進めやすいのかもと少し思いました。
やりやすいように進めて頂くのが一番良いのかなと思っています・・・!

@qwerty2501 qwerty2501 changed the base branch from main to rust May 9, 2022 01:43
@qwerty2501
Copy link
Contributor Author

@Hiroshiba target branch変えました。専用ブランチできたのであれば、CI通ってない状態でもmergeして大丈夫かなと思うのでとりあえずこのままmergeしてしまってよいかなと思います。CI修正するのはかなりかかるので
APIについて既存のものに寄せるようにしましたが、initializeのデフォルト引数についてRustにはデフォルト引数がないのでデフォルト引数なしの状態になってます

@qwerty2501
Copy link
Contributor Author

qwerty2501 commented May 9, 2022

デフォルト引数ないことについてですが、今後の修正でこれと似たような対応が必要になりそうです
#125 (comment)

@PickledChair
Copy link
Member

Rust 移行、C++ の辛い点をいくつか回避できそうで良さそうですね! Rust を勉強中なので、自分も今後できる範囲で議論や PR 等で貢献したいと思いました。

少し前に version 0.11 のコアを使う想定の Rust TTS クレートを作ってみていました https://github.com/PickledChair/voicevox-tts-rs
内容としては version 0.12 で追加される予定の TTS 機能部分(ただし現在は kana_parser を除く)の Rust による再実装と、cmake-rs を使った OpenJTalk の静的リンクを試したものです。まだ手を入れたい部分もあるのですが、もし参考になれば幸いです(あるいは今後いくつかの段階に分けて実装を進める感じであれば、このコードを踏まえて自分も PR を出そうかなと思っています)。

@qwerty2501
Copy link
Contributor Author

そちらのコードをベースにすればsynthesize部分を実装して割と早くmainにmergeできそうですね

@PickledChair
Copy link
Member

そちらのコードをベースにすればsynthesize部分を実装して割と早くmainにmergeできそうですね

そうですね、そのつもりでもうちょっと自分のコードを見直しておこうと思います……!

@qwerty2501 qwerty2501 marked this pull request as ready for review May 9, 2022 07:52
@qwerty2501
Copy link
Contributor Author

すみませんdraftのままでした

@qwerty2501
Copy link
Contributor Author

openjtalkを静的リンクする場合を考えるとcargo projectをworkspace化してしておいたほうがよさそうだったので変更

@qwerty2501 qwerty2501 marked this pull request as draft May 9, 2022 18:43
@qwerty2501
Copy link
Contributor Author

すみませんもう少し修正したいのでいちどdraftに戻します。

@qwerty2501 qwerty2501 marked this pull request as ready for review May 10, 2022 01:00
@qwerty2501
Copy link
Contributor Author

軽い実装をするつもりでしたがどの機能も何かしらのライブラリに依存しているため、やはり一度mergeしてから順次実装を追加していく方向にしたいと思います。
draftはずします

@qwerty2501 qwerty2501 force-pushed the feature/migrate_to_rust branch from 165a2b4 to 3c677b6 Compare May 10, 2022 08:33
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.

気になったところに対していくつか質問させていただきました。コード自体は全体的に問題なさそうでした!

voicevox_core/Cargo.toml Outdated Show resolved Hide resolved
voicevox_core/build.rs Outdated Show resolved Hide resolved
voicevox_core/build.rs Outdated Show resolved Hide resolved
voicevox_core/src/c_export.rs Outdated Show resolved Hide resolved
voicevox_core/src/internal.rs Outdated Show resolved Hide resolved
voicevox_core/src/internal.rs Outdated Show resolved Hide resolved
- バイナリ名を従来通りcoreに変更
- ヘッダを従来どおりcore.hに変更
- workaroundでworkspaceのルートディレクトリを取得しその配下のtargetにヘッダを出力するようにした
@qwerty2501 qwerty2501 force-pushed the feature/migrate_to_rust branch from b4fbf40 to adc9d65 Compare May 10, 2022 13:46
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.

追加で気になったところがあったので確認よろしくお願いします!

voicevox_core/Cargo.toml Outdated Show resolved Hide resolved
voicevox_core/src/c_export.rs Outdated Show resolved Hide resolved
voicevox_core/src/c_export.rs Outdated Show resolved Hide resolved
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.

小刻みなコメントになってしまいすみません。もう一箇所だけコメントしました。

voicevox_core/src/c_export.rs Outdated Show resolved Hide resolved
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! PR ありがとうございました……!

今後の実装作業について、方針や進捗状況などを整理するための Issue があると便利そうですね。作っておこうと思います。

@qwerty2501
Copy link
Contributor Author

qwerty2501 commented May 10, 2022

@PickledChair あ、LGTMいただいてたんですね。ちょっとソースコード配置するディレクトリをcrates配下にするようにしちゃいました。(実装は変えてないです。workspaceを採用しているcargo projectの場合cratesディレクトリ配下に各crateのディレクトリを作成するのはよく見ます)

とりあえずこれ以上の自分からの修正はこのPRでは行わないようにします

@PickledChair
Copy link
Member

@qwerty2501

ちょっとソースコード配置するディレクトリをcrates配下にするようにしちゃいました。

了解です! 問題ないです。

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.

Rustちゃんとわかってないので、メンテナンス面からコメントしてみました!

build.ymlが落ちる関係で、CIが必ず失敗するようになっていそうです。
別に良いのですが、テンションが下がってしまうのと、テストが通ってるかぱっとわかりにくいので、いったん消してしまっても良いかもと思いました。

core/__init__.py Show resolved Hide resolved
crates/voicevox_core/Cargo.toml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@qwerty2501
Copy link
Contributor Author

@Hiroshiba build.yml消してみました!

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

@Hiroshiba Hiroshiba merged commit e466c20 into VOICEVOX:rust May 15, 2022
qwerty2501 added a commit to qwerty2501/voicevox_core that referenced this pull request Jul 23, 2022
* いったんC++関係のコードを削除

* とりあえずcargo initializeと関数定義だけ

* APIを既存のものに合わせて定義しなおした

* cargo projectをworkspace化

* テストCI追加

* last_error_messageの定義が抜けていたので修正

* internal関数に定義を分けた

* anyhowやめた

* 元あったヘッダーファイルに合わせて出力されるように定義を変更した

* result 変換方法を変更

* 従来どおりの出力をするように修正

- バイナリ名を従来通りcoreに変更
- ヘッダを従来どおりcore.hに変更
- workaroundでworkspaceのルートディレクトリを取得しその配下のtargetにヘッダを出力するようにした

* ERROR_MESSAGEをonce cell化

* Succeed渡したときでもメッセージを返すように修正

VOICEVOX#126 (comment)

* VoicevoxResultCodeのエラーメッセージとErrorのベースとなる文字列を同一にするようにした

* output_binary_sizeの型をCのintに変更

* output_binary_sizeの型をc_intに変更

* 不要な文言を削除

* 今後workspace内にcrateが増えるとサブディレクトリがいっぱいできるのでrootディレクトリにディレクトリが増えすぎないようにcratesサブディレクトリ内に移動させた

* 無駄な空行を削除

* 不要な空白行を削除

Co-authored-by: Hiroshiba <[email protected]>

* 現状かならずCIテストが失敗するようになっているのでbuild.ymlを削除

Co-authored-by: Hiroshiba <[email protected]>
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