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.

Sign up for GitHub

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

docs: "APIデザイン ガイドライン"を追加 #870

Merged
merged 2 commits into from
Nov 11, 2024
Merged

docs: "APIデザイン ガイドライン"を追加 #870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
899ba0a
docs: "APIデザイン ガイドライン"を追加
qryxip Nov 9, 2024
0832bdd
`Range`についてはやめ、todoにする
qryxip Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,7 @@ Issue 側で取り組み始めたことを伝えるか、最初に Draft プル

### Rust 以外の言語の API に関する方針

VOICEVOX CORE の主要機能は Rust で実装されることを前提としており、他の言語のラッパーでのみの機能追加はしない方針としています。これは機能の一貫性を保つための方針です。
各言語の特性に応じた追加実装（例えば、Python での `style_id` の [`NewType`](https://docs.python.org/ja/3/library/typing.html#newtype) 化など）は許容されます。
[APIデザイン ガイドライン](./docs/guide/dev/api-design.md)をご覧ください。

## コアライブラリのビルド

Expand Down
15 changes: 15 additions & 0 deletions docs/guide/dev/api-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# APIデザイン ガイドライン

## Rust 以外の言語の API

VOICEVOX CORE の主要機能は Rust で実装されることを前提としており、他の言語のラッパーでのみの機能追加はしない方針としています。これは機能の一貫性を保つための方針です。

ただし機能追加ではない範囲で、各言語の習慣に適合するような変更は積極的に行っていきます。例えば:

* [`AudioQuery`](https://voicevox.github.io/voicevox_core/apis/rust_api/voicevox_core/struct.AudioQuery.html)といったJSONで表現可能なデータ型は、Pythonなら[Pydantic](https://docs.pydantic.dev)、JavaScriptなら[Zod](https://zod.dev/)といったライブラリを使って表現すべきです。
* Rust APIとやりとりする際はJSONを介して変換します。
* [`StyleId`](https://voicevox.github.io/voicevox_core/apis/rust_api/voicevox_core/struct.StyleId.html)といった[newtype](https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html)は、そのままnewtypeとして表現するべきです。
* 例えばPythonなら[`typing.NewType`](https://docs.python.org/ja/3/library/typing.html#newtype)で表現します。
* オプショナルな引数は、キーワード引数がある言語であればキーワード引数で、ビルダースタイルが一般的な言語であればビルダースタイルで表現すべきです。
* 「範囲」を表すデータ型が言語レベルである場合は、可能な限りそのデータ型を用いてAPIを構成するべきです。
* 例えばSwiftやRubyでは`Range`という名のデータ型を使って表現します。
Copy link
Member

@Hiroshiba Hiroshiba Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

これ、その言語のRangeがサポートしているものと、APIがサポートするrangeが異なる場合があってややこしいかもです！

例えばたしかPythonはrange(0, 3, -1)とかにすると、逆向きの範囲を指定できます。たぶん[2, 1, 0]。
あと飛び飛びの範囲も指定できます。range(0, 4, 2)だとたぶん[0, 2]。
このとき、例えばrender()で期待されるレンダリング結果は何になるべきなのか考えれなそうです。

一言でいうと、rangeは範囲だけを表してるものではないので、範囲だけを表したい場合にrangeを使うのは良くないのかもと･･･（今気づきました）

Copy link
Member Author

@qryxip qryxip Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

これの対象はSynthesizer::renderの引数ですね。なのでPythonだとおそらく、仮にstepに対応したとしてもrange ― というより多分sliceオブジェクトですが ― を引数に取るのはやめた方がよいという感じになると思います。

# こっちより多分、
synthesizer.render(audio_feature, slice(0, length, 10))
# こう
synthesizer.render(audio_feature, 0, length, 10)

多分正しいのはRust APIのrenderの引数をRangeにした上で、そのrenderを名指しして他のAPIもそうしろという感じになると思うのですが、今はまだ #867 でもそうなっていないわけでどうしたものか…という感じでいます。

Copy link
Member

@Hiroshiba Hiroshiba Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

あ〜〜〜なるほどです、RustのRangeは本当に範囲で、かつend >= startなんですね！
たしかにすくなくともRustは使ってもよいのかも。

いやでもrender()はend=startのときにエラーを返す設計になるかもな気がします。
個人的には一旦何も書かず、render側でRangeを使おうってなったら書き足すのが良い気がしてます。

（Range使いたい寄りだと把握しました！）

Copy link
Member Author

@qryxip qryxip Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

なるほど。そうしましょう！

0832bdd (#870)
(消したと同時にtodoを残しました)

render()はend=startのときにエラーを返す設計になるかもな気がします。

start > endはともかくstart == endだと空と解釈されると思います。他言語的にもそれが自然かと。
（実装レベルだと空テンソルをONNX Runtimeに渡したらどうなるのか私は知りませんが、別途チェックすればよさそう）

start > endだと、この(↓)表とかを考えると慣習に反しうりそうなので補足は要りますね。
#867 (comment)

ちなみにRustでリテラルを使って20..10とか書くと、denyレベルで怒られます。
image
[追記] エラーのスクショがちょっと適切じゃなかったので差し替え

Copy link
Member

@Hiroshiba Hiroshiba Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

startは含むけどendは含まないの、普通の感覚で行くとものすごい分かりにくいということに気づきました 😇
Rangeだとかがそうなってるのは、「そう設計しておくと色々便利だから」以外に説明が思いつかず、APIで同じことをすると不慣れな人にとってはなぜそうなっているかわからない気がしました。。。

（例えばstart: 10・end: 10だったとき、10を含まないのが直感的じゃない）

開区間、あるいは閉区間になってることが分かりやすい用語を探しました。
採用するかは置いといて、minとmaxが一番わかりやすそうではありました。（けど予約語なことも多そうなので迷いどころ）
fromとtoも良さそうだけど、小さい値がどちらかは分かりづらそう。
offsetとcount（length）も悪くなさそう。
https://chatgpt.com/share/673102c7-9858-8008-a5d1-fb36a5ac0382

これめちゃくちゃ厄介ですね。。。
RustはRangeで良いかもだけど、例えばC APIでstart・endにしてendは開区間にすると、まあ間違いなく閉区間だと思ってバグって使う人が出てくる気がする。。
まあrender()したものを聞いたときに、音声が不連続になるので違和感には気づけそうですが･･･････。

追記：おそらく(offset, length)が使いやすい＆わかりやすい気がする。
RustはRangeでも良いかも。このときRange(offset, offset+length)と書けるので相性も良い。
start=endのときの挙動は、サポートしてもしなくてもどちらでも良さそう。どちらにせよドキュメントに明記したい。

Copy link
Member Author

@qryxip qryxip Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

これ(↓)は以前私がした提案ですが、C APIとJava APIだけKotlinにならってbegin/start, end_exclusiveとし、他の言語では言語に合った名称 ― 例えばPythonではstart, stop ― にしておけば十分なのではと思っています。
#854 (comment)

というのも

  1. プログラミングの経験がある人で $[i, j)$ か $[i, j]$ かを気にせずにコードを書く人は多分ほぼいない
  2. "start"や"end"といった語彙が言語の標準にある場合、その語彙はその言語において一意だとしてよいはず
    • 例えばPythonで"start"と"stop"という言葉が出たら、それは $[\texttt{start}, \texttt{stop})$ という区間を意味する。NumPyもそう
    • [追記] Rustでは"end"といってもinclusiveかexclusiveかは一意ではない（後述するが実は"start"も一意ではない）けど、Range<_>とRangeInclusive<_>といったくくりのオブジェクトで取り回す文化であるため、問題ではない
  3. [追記] もし言語レベルでの明確な単語が無く、また範囲データ型のようなものが無い場合でも、Kotlinにならってend_exclusiveとでもすればよい
  4. 1., 2., 3.を踏まえると、プログラミング初学者に対しての役割はドキュメントが持つべき

だと思った次第です。

参考:

VOICEVOX/voicevox_core#854 (comment)
Ruby, C++「begin...end」
Python 「start:stop」
Rust, Scala, C# 「start..end」
Kotlin 「start..<endExclusive」
Swift 「first..<last」
Erlang 「lists::seq(From, To)」

https://discord.com/channels/879570910208733277/893889888208977960/1296122640406417468

(offset, length)

ユーザーのニーズには多分合っていると思うのですが、今現在はみ出すような範囲指定を不正入力扱いすると決めているため、"end"側の処理については絶対位置を意識せざるを得なくなり不便なのでは、という気もします。

ちなみに豆知識その2

Rustでは $(i, j]$ や $(i, j)$ も一応は表現できます。他の言語にはあまり無さそうですが…

use std::ops::Bound;

let a = [1, 2, 3, 4, 5];

// {`Included(i)`, `Excluded(i)`, `Unbounded`}²で、3²=9通り

// [i, j)
assert_eq!([2, 3], a[(Bound::Included(1), Bound::Excluded(3))]);
assert_eq!([2, 3], a[1..3]); // ショートハンド

// [i, j]
assert_eq!([2, 3, 4], a[(Bound::Included(1), Bound::Included(3))]);
assert_eq!([2, 3, 4], a[1..=3]); // ショートハンド

// [i, 右端]
assert_eq!([2, 3, 4, 5], a[(Bound::Included(1), Bound::Unbounded)]);
assert_eq!([2, 3, 4, 5], a[1..]); // ショートハンド

// (i, j)
assert_eq!([3], a[(Bound::Excluded(1), Bound::Excluded(3))]);
// ショートハンドは無い

// (i, j]
assert_eq!([3, 4], a[(Bound::Excluded(1), Bound::Included(3))]);
// ショートハンドは無い

// (i, 右端]
assert_eq!([3, 4, 5], a[(Bound::Excluded(1), Bound::Unbounded)]);
// ショートハンドは無い

// [左端, j)
assert_eq!([1, 2, 3], a[(Bound::Unbounded, Bound::Excluded(3))]);
assert_eq!([1, 2, 3], a[..3]); // ショートハンド

// [左端, j]
assert_eq!([1, 2, 3, 4], a[(Bound::Unbounded, Bound::Included(3))]);
assert_eq!([1, 2, 3, 4], a[..=3]); // ショートハンド

// [左端, 右端]
assert_eq!([1, 2, 3, 4, 5], a[(Bound::Unbounded, Bound::Unbounded)]);
assert_eq!([1, 2, 3, 4, 5], a[..]); // ショートハンド

renderの話になってきたので。
CC: @Yosshi999

Copy link
Member

Choose a reason for hiding this comment

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

たぁしかにlengthにすると最後部分の計算が面倒ですね…。
（chagpt君も全く同じ意見だった https://chatgpt.com/share/67319e1d-53b0-8008-8a27-1ebec0b8cef3 ）

少なくともmin,max指定はダメそうですね！！
例えば将来秒指定するAPIができた時に詰みそう。

lengthで範囲を指定し間違えるとエラーになってくれますが、endの解釈を間違えると変な音が出るだけなので、そういうものかと品質の誤解になりえるかも。

まーーーでもこれだけ考えて決定打が出ないということは、わりとどちらも一長一短で決まりな気がしますね！！
とりあえず今のところ、endにしてドキュメント書くでも、lengthにするでもどちらでも良さそう！！
また時間置きますかぁ。

Loading