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

feat!: VoiceModelFile::close後もidmetasへのアクセスを保証 #937

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 23, 2025

内容

  1. Rust APIでは、VoiceModelFile::closeの返り値を()から(VoiceModelId, Vec<SpeakerMeta>)
  2. C APIでは、voicevox_voice_model_file_closevoicevox_voice_model_file_deleteの二つを提供するように
    voicevox_voice_model_file_closevoicevox_voice_model_file_deleteに改名
  3. Python APIとJava APIでは、__(a)exit__後もidmetasにアクセス可能であることをドキュメント上で約束

関連 Issue

その他

@qryxip qryxip marked this pull request as draft January 23, 2025 18:31
@qryxip qryxip changed the title feat: VoiceModelFile::close後もidmetasへのアクセスを保証 [WIP] feat: VoiceModelFile::close後もidmetasへのアクセスを保証 Jan 23, 2025
@qryxip qryxip changed the title [WIP] feat: VoiceModelFile::close後もidmetasへのアクセスを保証 [WIP] feat!: VoiceModelFile::close後もidmetasへのアクセスを保証 Jan 24, 2025
@qryxip qryxip force-pushed the feat-ensure-voice-model-file-id-and-metas-accessible branch 2 times, most recently from 8429882 to 99530cc Compare January 24, 2025 17:08
@qryxip qryxip marked this pull request as ready for review January 24, 2025 17:08
@qryxip qryxip requested a review from Hiroshiba January 24, 2025 17:08
@qryxip qryxip force-pushed the feat-ensure-voice-model-file-id-and-metas-accessible branch from 99530cc to c8afcda Compare January 24, 2025 17:27
@qryxip qryxip changed the title [WIP] feat!: VoiceModelFile::close後もidmetasへのアクセスを保証 feat!: VoiceModelFile::close後もidmetasへのアクセスを保証 Jan 24, 2025
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_voice_model_file_delete一本に絞ってもよいかもしれません。idとか知りたきゃ先に取っておけみたいな感じで。

Copy link
Member Author

Choose a reason for hiding this comment

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

1c1cb24 (#937)

考えたのですが、voicevox_voice_model_file_delete一本にしてしまいました。これによりC APIの変更は「voicevox_voice_model_file_closedeleteに改名」ということのみになります。理由としてはC API経由でバインディングを作る人とかに混乱をもたらすだろうと思ったからです。あと実装面でも(既にご覧になったと思いますが)小さくない複雑性をもたらすので、少なくとも今は要らないかなと思った次第です。

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…?です!

そもそもopenしてないとid見れないんでしたっけ。
だったらちょっとだけ不便かもですね…! 

いつでもid取得できる形にできればcloseの返り値はいらなくなりそう。
(closeでidとかが帰るのはかなり珍しそうな直感。)

@qryxip qryxip requested a review from Hiroshiba January 25, 2025 06:18
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!

とりあえず再掲

そもそもopenしてないとid見れないんでしたっけ。
だったらちょっとだけ不便かもですね…! 

いつでもid取得できる形にできればcloseの返り値はいらなくなりそう。

@qryxip
Copy link
Member Author

qryxip commented Jan 25, 2025

ちょっと理解しきれてないかもなのですが、それをやっているのがPythonとJavaで、Rustではムーブセマンティクスの概念に沿う形がいいかなと思ってます。このPRのdocs/guide/dev/api-design.mdに書いたみたいに。
(CXXでC++ API作るときになっても、抜け殻にmetasを持たせたままにするわけにはいかないので同様の形になると思ってます。)

@qryxip qryxip merged commit 9431e00 into VOICEVOX:main Jan 25, 2025
30 checks passed
@qryxip qryxip deleted the feat-ensure-voice-model-file-id-and-metas-accessible branch January 25, 2025 14:04
@Hiroshiba
Copy link
Member

あ、ほんとですね! openするとインスタンスが帰ってくるようになってたので問題なかった!
(インスタンスを作った後にopenする形だと勝手に勘違いしました!)

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.

2 participants