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

Node.js向けのFFIを実装 #758

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

takejohn
Copy link

内容

NAPI-RS を用いて Node.js 向けの FFI を実装します。

関連 Issue

ref #550

その他

#757 で Neon を使おうと思っていましたが、より開発が活発で情報が見つかる(と思われる) NAPI-RS を使うことにしました。

@takejohn takejohn marked this pull request as ready for review March 20, 2024 14:23
@takejohn
Copy link
Author

Ready for reviewとしてマークしました。
creates/voicevox_core_node_api をカレントディレクトリとして、
./README.md に書いたように yarn, yarn build, yarn docs を実行すると、./docs 以下にHTML形式のドキュメントを得られます。

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

とりあえず一通り。
Tripletごとにパッケージ分けるのって避けれたりします?絶対にしんどいと思うので

crates/voicevox_core_node_api/.yarnrc.yml Outdated Show resolved Hide resolved
crates/voicevox_core_node_api/.prettierrc Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Pythonと違ってJavaScriptは非同期がメイン(主観)なので、blocking APIは提供しなくてもいいと思います(個人の思想ですが)

Copy link
Member

Choose a reason for hiding this comment

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

NodeではないですがDenoだと例えば標準ライブラリの各機能に対して同期版があったりします (e.g. Deno.readFileに対してDeno.readFileSync)。ただNodeでもそれをやるべきかというと微妙かもしれませんが。

@Hiroshiba #662 の観点から一言お願いします。

Copy link
Member

Choose a reason for hiding this comment

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

絶対ないといけないとは思わない、とはいえ実装を拒む理由も特にない、って感じですかね!

webのjsならともかく、nodeを使う人のレベル感は割と高そうなので、非同期版のみでも大丈夫だと思います。
とはいえ別にあったらダメというわけでもない気がしました。トップレベルasyncが書けない場合があったりとか、何かと便利ではあると思うので。

Copy link
Member

Choose a reason for hiding this comment

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

-muslまでの差分は提供しなくてもいいと思います。

crates/voicevox_core_node_api/src/namespaces/blocking.rs Outdated Show resolved Hide resolved
@takejohn
Copy link
Author

Tripletごとにパッケージ分けるのって避けれたりします?絶対にしんどいと思うので

npm ディレクトリ以下は未使用だったので削除しました。

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

遅くなりましたが、概ね良いと思います!

lintに引っ掛かってる部分とコンフリクトについては、ご面倒であればこちらでやらせて頂ければと思っているのですが、どうでしょうか?

(追記) #761 対応もありました。StyleMetatypeというものが追加されています。

@takejohn
Copy link
Author

遅くなりましたが、概ね良いと思います!

lintに引っ掛かってる部分とコンフリクトについては、ご面倒であればこちらでやらせて頂ければと思っているのですが、どうでしょうか?

(追記) #761 対応もありました。StyleMetatypeというものが追加されています。

ありがとうございます。自分で対応してみます。

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

概ね良いと言った手前ですがいくつか:

Cargo.toml Outdated Show resolved Hide resolved
crates/voicevox_core_node_api/.cargo/config.toml Outdated Show resolved Hide resolved
crates/voicevox_core_node_api/Cargo.toml Outdated Show resolved Hide resolved
crates/voicevox_core_node_api/Cargo.toml Outdated Show resolved Hide resolved
crates/voicevox_core_node_api/rustfmt.toml Outdated Show resolved Hide resolved
__dirname,
"..",
"..",
"..",
Copy link
Member

Choose a reason for hiding this comment

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

#791 (comment)

Suggested change
"..",
"test_util",
"data",

Copy link
Member

Choose a reason for hiding this comment

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

.gitattributesでlinguist-vendored linguist-generatedを設定した方がよいかも。

参考:
https://github.com/VOICEVOX/voicevox_core/blob/18aec9f1bd279856eaf5007f021fe8501c82c4a6/crates/voicevox_core_java_api/.gitattributes

`npm test` でテストを行うことができます。

```console
❯ npm test
Copy link
Member

Choose a reason for hiding this comment

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

テスト前にcargo build -p test_utilが要るんじゃないかと思います。
(クリーンなリポジトリからvoicevox_core_node_apiだけビルドした場合、crates/test_util/dataは生成されない)

参考:

- run: cargo build -p test_util -vv # build scriptにより/crates/test_util/data/の生成

cargo build -p test_util -vv # build scriptにより/crates/test_util/data/の生成

npm test内に仕込むのがいいかもしれませんが、どうやるかは私にはよくわからないです。なんかnpm-run-all2っていう、かつてのnpm-run-allのフォークがあるっぽい…?

Copy link
Member

Choose a reason for hiding this comment

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

"test": "run-s test:build-util test:ava",
"test:build-util": "cargo build -p test_util",
"test:ava": "ava"

npm-run-all2使うとこんな感じだと思います。
まぁ直列ならOS差は困るほどはないので、

"test": "cargo build -p test_util && ava"

でいいかも。avaにセットアップのHookがあるならそれでビルドしてもいいかも?

Comment on lines +1 to +2
#![deny(clippy::all)]

Copy link
Member

Choose a reason for hiding this comment

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

NAPI-RSの慣習だと思いますが、このリポジトリの場合はCIでやってるので不要かと思います。

- run: cargo clippy -vv --all-features --features onnxruntime/disable-sys-build-script --tests -- -D clippy::all -D warnings --no-deps
- run: cargo clippy -vv --all-features --features onnxruntime/disable-sys-build-script -- -D clippy::all -D warnings --no-deps

ClippyというのはVSCodeとかのエディタ上で動くので(もし設定されていなかったのであれば設定することをお勧めします)、少しうっとおしくなってしまうかと思います。

Suggested change
#![deny(clippy::all)]

@qryxip
Copy link
Member

qryxip commented May 23, 2024

pub fn unload_voice_model(&self, voice_model_id: String) -> Result<()> {
convert_result(
self.synthesizer
.unload_voice_model(&VoiceModelId::new(voice_model_id)),
Copy link
Member

Choose a reason for hiding this comment

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

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.

None yet

4 participants