search

jkawamoto / ctranslate2-rs

Rust bindings for OpenNMT/CTranslate2
https://docs.rs/ct2rs
MIT License
14 stars 2 forks source link

Abort callback #65

Open thewh1teagle opened 1 week ago

thewh1teagle commented 1 week ago

Provide option to pass callback function which will be called just like the stream callback to abort the translation if true returned and keep it if false returned

jkawamoto commented 1 week ago

You can pass an optional callback as an argument to Translator::batch_translate, etc. If the callback returns an error, the translation will be aborted. I think it’s better than returning a boolean value because callbacks can use ?.

thewh1teagle commented 1 week ago

You can pass an optional callback as an argument to Translator::batch_translate, etc. If the callback returns an error, the translation will be aborted. I think it’s better than returning a boolean value because callbacks can use ?.

Thanks! I didn't know it was possible to return an error; that sounds even better.

By the way, have you considered using bindgen for the bindings? I'm not an expert in that field, but using bindgen along with creating a ctranslate2-rs-sys crate seems cleaner. I was inspired by whisper-rs.

jkawamoto commented 1 week ago

I tried bindgen, but if I’m not mistaken, it aims to integrate C libraries rather than C++ libraries. It looks like whisper.cpp provides C interface, so bindgen is a good option, while CTranslate2 provides only C++ interface. In this case, I think cxx works better.

thewh1teagle commented 1 week ago

I tried bindgen, but if I’m not mistaken, it aims to integrate C libraries rather than C++ libraries. It looks like whisper.cpp provides C interface, so bindgen is a good option, while CTranslate2 provides only C++ interface. In this case, I think cxx works better.

Bindgen works with cpp as well, as the project I mentioned whisper-rs is a bindings to whisper.cpp Maybe using bindgen in long term in this project will help, I don't have much experience with it but look more promising.

jkawamoto commented 1 week ago

The problem I encountered when I tried to use bindgen was that it didn’t support template classes in C++ yet (see also https://rust-lang.github.io/rust-bindgen/cpp.html#unsupported-features). CTranslate2 uses them. For example, Translator::translate_batch (https://github.com/OpenNMT/CTranslate2/blob/master/include/ctranslate2/translator.h) takes std::vector<std::vector<std::string>>, which is incompatible with bindgen so far, so we have to write a wrapper manually.

Once bindgen adds support for template classes, it might be easier to use.