fix: prefer serde structs over custom functions #2127
Conversation
drbh
force-pushed
the
prefer-chat-object-enum
branch
from
518847b to
bf24b76
Compare
drbh
changed the title
drbh
force-pushed
the
prefer-chat-object-enum
branch
from
d43ef3d to
c326ffd
Compare
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)] | ||
| #[serde(untagged)] |
There was a problem hiding this comment.
#[serde(untagged)] is unfortunately quite dangerous.
It will stop on first match, which is in some cases not what you expect (String matches a lot of thigns for instance).
This makes the entire thing order dependent in the struct and creates hard to reason about bugs.
I'll continue reading the PR, but since tokenizers did it and we're having many issues with it, my current stance would be to stay away from it.
| pub fn from_file<P: AsRef<Path>>(filename: P) -> Option<Self> { | ||
| std::fs::read_to_string(filename) | ||
| .ok() | ||
| .and_then(|content| serde_json::from_str(&content).ok()) |
There was a problem hiding this comment.
I kind of prefer the earlier version, I find it easier to reason about.
Changing from Option to result would make it even more ergonomic (not sure we care though)
| @@ -121,35 +139,6 @@ pub(crate) enum GrammarType { | |||
| Regex(String), | |||
| } | |||
|
|
|||
| mod token_serde { | |||
There was a problem hiding this comment.
It used to be exactly like #[serde(untagged)] therefore no change in actual logic.
| .into_iter() | ||
| .map(|chunk| match chunk { | ||
| MessageChunk::Text { text } => text, | ||
| MessageChunk::ImageUrl { image_url } => format!("", image_url.url), |
There was a problem hiding this comment.
Didn't we remove that ? I see it's equivalent with what was before so it's ok, but I though we removed those.
| @@ -635,7 +636,7 @@ async fn completions( | |||
| )); | |||
| } | |||
|
|
|||
| if req.prompt.len() > info.max_client_batch_size { | |||
| if req.prompt.0.len() > info.max_client_batch_size { | |||
There was a problem hiding this comment.
That works.
Other options
let Prompt { text} = req.prompt
Or implement Deref (if that .0 becomes ubiquitous and annoying; https://doc.rust-lang.org/std/ops/trait.Deref.html#when-to-implement-deref-or-derefmut
* fix: prefer enum for chat object * fix: adjust typo * fix: enum CompletionType not ObjectType * fix: adjust typo * feat: leverage serde for conditional deser * fix: adjust HubTokenizerConfig after rebase * fix: update create_post_processor logic for token type * fix: adjust unwrap syntax in template * Fixing the post processor. --------- Co-authored-by: Nicolas Patry <[email protected]>
* fix: prefer enum for chat object * fix: adjust typo * fix: enum CompletionType not ObjectType * fix: adjust typo * feat: leverage serde for conditional deser * fix: adjust HubTokenizerConfig after rebase * fix: update create_post_processor logic for token type * fix: adjust unwrap syntax in template * Fixing the post processor. --------- Co-authored-by: Nicolas Patry <[email protected]>
* fix: prefer enum for chat object * fix: adjust typo * fix: enum CompletionType not ObjectType * fix: adjust typo * feat: leverage serde for conditional deser * fix: adjust HubTokenizerConfig after rebase * fix: update create_post_processor logic for token type * fix: adjust unwrap syntax in template * Fixing the post processor. --------- Co-authored-by: Nicolas Patry <[email protected]>
**updated
This PR prefers an enum that wraps
ChatCompletionChunkandChatCompletionand uses the serdetagmacro to set the correct object value.unfortunately serde does not provide a
rename_all = "dot.case"serde-rs/serde#2269 so each value has an explicit renamethe following enum is used
**update again
This PR has moved to leveraging serde where possible to remove custom deserialization functions and overall make the types more readable and idiomatic