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

ドキュメントを刷新する #532

Merged
merged 47 commits into from
Aug 2, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jun 25, 2023

内容

Rust/C/Python APIのドキュメントを刷新します。

  • Rust/C/Python API全部にドキュメントを書く
    • 表記、ドメイン用語の統一
      • example/下のサンプルコードではまだspeaker_id (話者ID)という表現になっているため、style_id (スタイルID)に改める
    • 文末の句点などの、文章の書き方も一貫させる
  • Rust/C/Python API全部において、各APIのコード例を書く。
  • Rustdoc/Doxygen/Sphinx的に正しい記法に直す
  • C APIのドキュメントのトップに健全性(soundness)周りの説明をしっかりと書き(例えば「有効な」とか「アラインメントに沿った」とか「未定義動作」というのが何を指しているかの説明)、各APIの# Safetyもしっかり記述する
  • APIが投げうるエラーについて網羅的にまとめ、Rustdocの# Errors/# PanicsやSphinxの:raises:でしっかりと記述する

Rust APIについては、どちらかというとC APIとPython APIの表記を取り纏めるために書きました。

今回は諦めるもの

手元でのドキュメントの生成と、その生成先

  • Rust API:

    cargo doc -p voicevox_core --document-private-items

    Rust APIはAPIとして公開されることが想定されていない構造になっているため、--document-private-itemsが無いとドキュメントが書かれているべき箇所が一部隠れてしまいます。

    生成先はtarget/doc/voicevox_core/index.htmlです(生成時に--openを付けるとそこが自動で開かれます)。

  • C API:

    mkdir -p public/apis/c_api &&
    >   cp crates/voicevox_core_c_api/include/voicevox_core.h docs/apis/c_api/doxygen/ &&
    >   (cd docs/apis/c_api/doxygen && doxygen)

    生成先はpublic/apis/c_api/voicevox__core_8h.htmlです。

  • Python API

    sphinx-build docs/apis/python_api public/apis/python_api

    生成先はpublic/apis/python_api/autoapi/voicevox_core/index.htmlです。

関連 Issue

その他

@qryxip
Copy link
Member Author

qryxip commented Jun 25, 2023

コードの補足説明は完成させてからやります...

qryxip added 2 commits June 25, 2023 23:07
`$ORT_OUT_DIR`のハックが効かないため。
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 25, 2023

非常に素晴らしいと思います!!!!

あ、もう遅いかもですが、1点だけ。。

修正範囲は[project-vvm-async-api]の範囲に留まりそうでしょうか?
とどまらない場合、とりあえずコード変更(と以前のものと同じような簡単なドキュメント)だけmainにマージしてしまって、そのあとドキュメント刷新するのがよくある進め方かもです。(diffとコンフリクトを減らすため)

まあでもドキュメント変更はdiffとしてわかりやすいですし、今から引き返すのが大変でしたら、[project-vvm-async-api]にマージしてmainにマージでも良いかなと思います!

@qryxip
Copy link
Member Author

qryxip commented Jun 26, 2023

このPRをmain向けにしなかった理由としては、vvm-async-apiはモジュールの構造からAPIの形まで結構な別物になっているためproject-vvm-async-api ← mainのマージ時の手間が「書き直し」レベルになるかと思ったからです。範囲に留まるというより、0.14(main)の原型を留めているAPIがもうそんなに無いのではと思っています。

diffについては、ちょっと今となっては割と手遅れ感がでているのでもうdiffで考えない方がいいのではないかと思っていました。今からでもmainとのdiffを減らす努力をすべきでしょうか?

@qryxip qryxip marked this pull request as ready for review July 8, 2023 08:53
@qryxip
Copy link
Member Author

qryxip commented Jul 8, 2023

↑のチェックボックスの半分ができていませんが、PRとして時間をかけすぎているのでdraftを外します。残りは別PRにします。

Copy link
Member Author

@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.

補足:

crates/voicevox_core/src/devices.rs Outdated Show resolved Hide resolved
pitch: f32,
}

/// AccentPhrase (アクセント句ごとの情報)。
Copy link
Member Author

Choose a reason for hiding this comment

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

AudioQueryがドメイン用語として運用してもよかった記憶があったのでAccentPhraseもそうしたが、こっちは「アクセント句」のままでもよかったかも。

Copy link
Member

Choose a reason for hiding this comment

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

エンジンだとAudioQueryは「クエリ」、AccentPhraseは「アクセント句」にしてました。
ちょっとわかりづらそうだなと思ったらこっち側で改修しようと思います。(たぶんそのままで行くと思います。)

crates/voicevox_core/src/engine/model.rs Outdated Show resolved Hide resolved
Comment on lines -8 to -13
/*
* 新しいエラーを定義したら、必ずresult_code.rsにあるVoicevoxResultCodeに対応するコードを定義し、
* internal.rsにある変換関数に変換処理を加えること
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

今となっては嘘コメントとなってしまっているため、削除。

Copy link
Member Author

Choose a reason for hiding this comment

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

mainの方に「嘘コメント一掃」みたいなPRをだしてもいいのかも。

crates/voicevox_core/src/error.rs Outdated Show resolved Hide resolved
*
* このライブラリの使用にあたっては次の条件を遵守しなければならない。違反は<b>未定義動作</b>(_undefined behavior_)である。
*
* - 「読み込みについて有効」と説明されているポインタは次の条件を満たしていなければならない。
Copy link
Member Author

@qryxip qryxip Jul 8, 2023

Choose a reason for hiding this comment

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

Rustの"valid for read"と"valid for write"をどう訳したものかと思ったが、下手に他の表現にするよりはこうした上でその定義を書き下すのが最も安全かと。

@@ -21,6 +21,7 @@ pub enum OpenJtalkError {

pub type Result<T> = std::result::Result<T, OpenJtalkError>;

/// テキスト解析器としてのOpen JTalk。
Copy link
Member Author

Choose a reason for hiding this comment

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

VOICEVOXにおけるOpen JTalkの役割を明記。
(Open JTalkは"text-to-speech system"であるため)

crates/voicevox_core/src/devices.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/engine/model.rs Outdated Show resolved Hide resolved
crates/voicevox_core_c_api/src/lib.rs Show resolved Hide resolved
@Hiroshiba
Copy link
Member

ドキュメントほんとにありがとうございます!!!
とても大変だったと思います、非常に感謝しています。

↑のチェックボックスの半分ができていませんが、PRとして時間をかけすぎているのでdraftを外します。残りは別PRにします。

こちらの方針に賛成です。
なるべく時間がかからないよう、こちら側で可能そうなことは作業分担を提案していこうと思います。

#[derive(ConstDefault)]
pub struct AccentPhrasesOptions {
/// AquesTalk形式のkanaとしてテキストを解釈する。
Copy link
Member

Choose a reason for hiding this comment

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

AquesTalkライクな読み仮名AquesTalk形式のkanaが何箇所かあるのですが、AquesTalkライクな記法で統一しようかな〜とちょっと思いました!
良さそうだったらこちらでやろうと思います。

ちなみにエンジンはAquesTalkライクな読み仮名AquesTalkライクな記法が混ざってて、まあ読み仮名というよりは記法だよな〜と。

Copy link
Member Author

Choose a reason for hiding this comment

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

「AquesTalk形式のkana」よりは十分に良い...と思うのですが、ユーザーにとっては今度は「"ライク"って何やねん」という疑問が生じそうな気もします。いっそのことドメイン用語"kana"として確立してしまうというのはどうでしょうか?

Copy link
Member

@Hiroshiba Hiroshiba Jul 15, 2023

Choose a reason for hiding this comment

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

AquesTalkライクな記法がそこそこわかりやすい+誤解ない感じで良いかなと思いました!

kanaも読み仮名と同様、ルビとかカタカナあたりと勘違いされる気がしてます。
ライクってなんやねんとは思いつつ、まあいま出てる候補の中ではAquesTalkライクな記法が一番良いかなぁと。

イマイチだとは思っているけど、エンジン側と歩調合わせないとだし、一旦後回しにしたい気持ちです。とりあえず↓のタスクに追加しておきました。

Copy link
Member

@Hiroshiba Hiroshiba Jul 23, 2023

Choose a reason for hiding this comment

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

ここもどうしましょう? あまり賛成できない感じであれば一旦マージした後にこちらで勝手に統一しようかなと考えています。

以下整理です。

  • 統一はmustだと思う
  • 「読み仮名」は定義を間違えている(アクセントは仮名ではない)のでmustで避けたい
  • 「kana」はドメイン用語を別定義しないといけない+読み仮名と勘違いされうるのでshoudとwantの間くらいで避けたい
  • 「AquesTalk形式な記法」は、厳密に言うとAquesTalkと異なるのでwantくらいで避けたい
  • 残るのが「AquesTalkライクな記法」くらいしかなく、エンジンと揃える意味でもこれにしておきたい
    • けど、ある程度の間違いは許容して伝わりやすさを優先して「AquesTalk形式な記法」はアリかも

(ChatGPTに聞いたら「AquesTalk風記法」を提案されました。少なくともVOICEVOXがバージョン1になる時はおそらくこの名称になると思います。。)

Copy link
Member

Choose a reason for hiding this comment

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

@qryxip こちらどうしましょう? 👀
任せますと言っていただければとりあえずこちらで統一しておきます 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

一つ残らず「AquesTalk風記法」で統一しました。

570d782 (#532)

image

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です!!
本当にお疲れ様でした!!!!

用語をエンジン側に合わせてくださりつつ、明瞭性が足りない部分は最小限の追加で補ってくださっているのを感じました。
とても助かります、ありがとうございます。

このあたり結構たいへんだったとかあれば知見のためにお聞きしたいです。

なんとなくですが、たぶんSafety項目のメンテナンスは正直難しいだろうなと感じました。
だいぶ深い知識が必要なことと、正確であることを担保し続ける労力がとんでもないので・・・。
ほぼ定型文なので、ドキュメントを作る関数にできたりしたらメンテナンスだいぶ楽になるかもとかは感じました。

vvmファイルへのパス
VVMファイルから ``VoiceModel`` をコンストラクトする。

:param path: VVMファイルへのパス。
Copy link
Member

@sevenc-nanashi sevenc-nanashi Jul 8, 2023

Choose a reason for hiding this comment

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

Sphinxは、
napoleonという拡張でNumPyスタイルのドキュメントでもいい感じにパースしてくれて:https://www.sphinx-doc.org/ja/master/usage/extensions/napoleon.html
MyST-ParserでMarkdownをパースしてくれるようになります:https://www.sphinx-doc.org/ja/master/usage/markdown.html

個人的には:reSTよりMarkdown+NumPyスタイルのdocstringの方が書ける人が多くて楽かなと思います。(その分依存が増えますが。ここら辺の意見を聞きたいです:@Hiroshiba

Copy link
Member

Choose a reason for hiding this comment

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

あまりこだわりなかったのですが、まあ特にこだわり無いならエンジン側に合わせてNumPyスタイルが良いのかなぁという気はします。

rst形式に特に問題を感じなかったのですが、napoleonのドキュメント読んだ感じ、型情報が書きづらい感じなんですかね。

  • どちらかというとNumPy形式のが良いけど別にrst形式でもOK
  • 改修に抵抗なければ改修が良いけど面倒なら一旦そのままでOK

って感じですかね〜

Copy link
Member Author

Choose a reason for hiding this comment

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

napoleonを使うとして、このPRでやった方がいいですかね?

Copy link
Member

@Hiroshiba Hiroshiba Jul 15, 2023

Choose a reason for hiding this comment

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

5段階中2くらいのやったほうが良さかなと思いました!
マージ優先かな~と。やるやらはともかく、タスクリストにぽんと書いといてresolveにしちゃうのどうでしょう。

Copy link
Member

Choose a reason for hiding this comment

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

こちらどうしましょう?
とりあえずissueのタスクリストに追記してしまって、一旦ここはresolveにするのが早いかなと思いました!

@qryxip
Copy link
Member Author

qryxip commented Jul 26, 2023

  • 「生存期間」についての記述を更新しました (2509a6a (#532))

@qryxip
Copy link
Member Author

qryxip commented Jul 30, 2023

@Hiroshiba
Copy link
Member

あと1点AquesTalkライクな記法のとこだけかなと!!

@qryxip
Copy link
Member Author

qryxip commented Aug 1, 2023

@Hiroshiba
Copy link
Member

AquesTalk風記法で統一されたんですね!
よい機会なのでエンジン側も統一しようかなと思いました。 VOICEVOX/voicevox_engine#720

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

読み応えのあるしっかりしたドキュメントになったと思います!ほんとにありがとうございました!!!!!
そういえばREADMEからPython APIのリンクが無いですね!思い出したときにでも足しちゃおうと思います!

あとはどれだけ使いやすくしていけるかなのかなと思いました!!
あとデータ構造の整理と。。

  • example/下のサンプルコードではまだspeaker_id (話者ID)という表現になっているため、style_id (スタイルID)に改める

PRのdescriptionに書かれてるこちらの内容、忘れそうだなとちょっと思いました!

@Hiroshiba
Copy link
Member

マージします!!!

@qryxip
Copy link
Member Author

qryxip commented Aug 2, 2023

PRのdescriptionに書かれてるこちらの内容、忘れそうだなとちょっと思いました!

他のものも含めissue化し、 #545 のタスクリストにも追記しました。

@qryxip
Copy link
Member Author

qryxip commented Aug 2, 2023

このあたり結構たいへんだったとかあれば知見のためにお聞きしたいです。
#532 (review)

こちらに答えると、一月以上要してしまったため最初の記憶が曖昧なのですが、大変だった順だと次のようになるかと思います。

  1. safety documentationの記述
  2. 表現を所々修正し、Rust/C/Pythonで統一するように
  3. Doxygen/Sphinxに合わせた書式に

safety documentationがやっぱりハードでした。そもそもC ABIを通じたユーザーに何を要求すればよいのかを考えるところから始まりましたし、いざ書こうとしても用語の日本語訳が探しても無いという事態に遭遇しました。"valid for reads" → "読み込みについて有効"とか未だにこれでよいのかと思ったりします。
まあ結果的に書く量はそんなに多くありませんでしたが...自分の中では。

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 3, 2023

ありがとうございます、非常に参考になります!!

safety documentationの記述

日本語化が難しいというのは盲点でした。お聞きして良かったです!
ちょっとまあ全然知らないのに偉そうなことを言っちゃうかもしれないのですが、Rust日本語化界隈的に実はそれなりの人数の方が持ってる課題なのかなとちょっと思いました。
この知見を生かして何かどこかでちょっとしたブログでもあげると楽しそうなのかなと思いました!

そういえばRustコミュニティの発表会みたいなのってありませんでしたっけ。Scala祭りにみたいな。
題材としてこのVOICEVOX COREは結構面白い・・・かも・・・?

表現を所々修正し、Rust/C/Pythonで統一するように

これはなるほどです。
どんどん言語を増やしていくとどんどん難しくなっていくんだろうなと感じました。

やるかやらないかさておいて、ドキュメントの修正を気軽に受けられるような仕組みを作っておくと、間違っていたとしても修正提案を受けやすいのかなと思いました!
なんかドキュメントページでたまに見かける「edit on github」みたいな。

@qryxip
Copy link
Member Author

qryxip commented Aug 4, 2023

この知見を生かして何かどこかでちょっとしたブログでもあげると楽しそうなのかなと思いました!

rust-jpのZulipで話題に出すのでもよさそうな気がしました。まあ考えておきます。

そういえばRustコミュニティの発表会みたいなのってありませんでしたっけ。Scala祭りにみたいな。
題材としてこのVOICEVOX COREは結構面白い・・・かも・・・?

Rust.Tokyoですね。10月開催で、登壇は7月〆のようです。来年mobileと、 #550 と、Rust API提供と、 然るべきコントリビューションガイドの作成と、その他面白そうなことが為せていれば考えてもいいかもしれません。
(Rust APIについては、ここでは割愛しますが考えていることがあります)

まあ今回は初回(2019年)ぶりの会場開催で、行くつもりなので、話の流れで出せそうなら出そうかなと思っています。あとは5分10分のLT会は定期的にちょくちょくあった気がします。

rust-jpの#events

なんかドキュメントページでたまに見かける「edit on github」みたいな。

確かに考えてもいいかもしれません。

@Hiroshiba
Copy link
Member

イベント参加いいですね!!!
何かお手伝いできることがあれば是非おっしゃっていただければ・・・!!

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.

4 participants