guillaume-be / rust-bert

Rust native ready-to-use NLP pipelines and transformer-based models (BERT, DistilBERT, GPT2,...)
https://docs.rs/crate/rust-bert
Apache License 2.0
2.67k stars 217 forks source link

Derive Serde for `pipelines` models and their respective configs (cargo feature) #197

Closed sno2 closed 3 years ago

sno2 commented 3 years ago

Hello, I have been working on getting this crate to work on Deno. However, I have had to copy and paste your struct code then derive From<YOUR_STRUCT> for MY_STRUCT then From<MY_STRUCT> for YOUR_STRUCT them to pass them through buffer from Rust to Deno through FFI. It would be great if rust-bert could provide these implementations under a feature (i.e. serde). This would also make it easier for utilizing rust-pert in other FFI implementations for other languages. I am also willing to do this. Thanks

guillaume-be commented 3 years ago

Hello @sno2 ,

In principle the implementation of Serde serialization for pipelines and configurations makes sense if it facilitates the generation of bindings to other languages.

I believe the reason you have to copy and paste code on your crate is the limitation to implement 3rd party traits for 3rd party structs? Would it be possible to create local wrappers around rust-bert structs and implement Serde for them.

I would assume the difficulty may come from the fact that the Tensor and VarStore objects in the library (part of the pipelines) may not be serializable with Serde - have you tried a working minimal solution by copying code over already?

sno2 commented 3 years ago

Hello @sno2 ,

In principle the implementation of Serde serialization for pipelines and configurations makes sense if it facilitates the generation of bindings to other languages.

I believe the reason you have to copy and paste code on your crate is the limitation to implement 3rd party traits for 3rd party structs?

Yes

Would it be possible to create local wrappers around rust-bert structs and implement Serde for them.

Not without having to manually input every field inside the custom Deserialize and Serialize implementation. If I try to use a tuple struct like the following to match it:

#[derive(Debug, Serialize, Deserialize)]
pub struct JsQaAnswer(rust_bert::pipelines::question_answering::Answer);

Then, you will get an error because Answer doesn't have Serialize / Deserialize implemented.

I would assume the difficulty may come from the fact that the Tensor and VarStore objects in the library (part of the pipelines) may not be serializable with Serde

For these structs, we could just not implement Serialize / Deserialize and let the library define it's own method for doing that or simply do serde(default) and add a note in the docs. Taking out a brunt of the custom work with the other structs will definitely help.

  • have you tried a working minimal solution by copying code over already?

Yes, that's what I've been doing currently. e.g. QAInput:

use serde::{Serialize, Deserialize};
use rust_bert::pipelines::question_answering::Answer;

#[derive(Debug, Serialize, Deserialize)]
pub struct JSQaInput {
    pub question: String,
    pub context: String,
}

impl From<QaInput> for JSQaInput {
    fn from(i: QaInput) -> Self {
        Self {
            context: i.context,
            question: i.question,
        }
    }
}

impl From<JSQaInput> for QaInput {
    fn from(i: JSQaInput) -> Self {
        Self {
            context: i.context,
            question: i.question,
        }
    }
}

However, that solution will get extremely dry when we get into the configs.

sno2 commented 3 years ago

You can see my WIP Deno bindings in my bertml repo for a full reproduction of what I'm doing currently.

guillaume-be commented 3 years ago

Hello @sno2 ,

Thank you for sharing the example repository. From what I see the Serde derivation would be required for the input and output types for the pipelines (e.g. QaInput or Sentiment). It would be fine to extend these types so that they #[derive(Serialize, Deserialize] and allow bindings to other framework, a PR would be welcome!

sno2 commented 3 years ago

From what I see the Serde derivation would be required for the input and output types for the pipelines (e.g. QaInput or Sentiment). It would be fine to extend these types so that they #[derive(Serialize, Deserialize] and allow bindings to other framework, a PR would be welcome!

Nice, and just to clarify, do you want me to include the Serialize / Deserialize derivations behind a feature named serde or just have them always be included? I hadn't realized that you actually used serde's derive features for a few structs so the dependency would get downloaded anyways.