feat: validate transforms on compile #151
Conversation
CodSpeed Performance ReportMerging #151 will not alter performanceComparing Summary
Footnotes
|
| pub struct ModelConfig { | ||
| pub model_type: String, | ||
| pub pad_token_id: u32, | ||
| pub num_labels: Option<usize>, |
There was a problem hiding this comment.
I suspect this is done for classification? Does it make sense to group class-related items under a classification key?
There was a problem hiding this comment.
@javiermtorres this is taken from the ModelConfig schema from huggingface. unfortunately can't change it :')
javiermtorres
left a comment
javiermtorres
left a comment
There was a problem hiding this comment.
Stronger types and dry runs are both great ideas 👍 Some minor remarks, but totally mergeable.
Did you take a look at https://luau.org/sandbox? Would it be worth it?
| .into_owned(); | ||
|
|
||
| outputs = state.transform().postprocess(outputs)?; | ||
| outputs = EmbeddingTransform::new(state.transform_str())?.postprocess(outputs)?; |
There was a problem hiding this comment.
This would allow changing the transform at runtime. Do we aim to do that at some point?
There was a problem hiding this comment.
This was basically already the case. We can improve though, I think this is a good flag
| fn postprocess(&self, (data, mask): Self::Input) -> Result<Self::Output, ApiError> { | ||
| let func = match &self.postprocessor { | ||
| Some(p) => p, | ||
| None => { |
There was a problem hiding this comment.
I'd even hope of making something like None => default_pool, but let me know if this would not be totally fitting.
| impl TransformValidatorExt for EmbeddingTransform { | ||
| fn dry_run(&self, _model_config: &ModelConfig) -> Result<()> { | ||
| // create dummy hidden states with shape [batch_size, seq_len, hidden_dim] | ||
| let dummy_hidden_states = random_tensor(&[BATCH_SIZE, SEQ_LEN, HIDDEN_DIM], (-1.0, 1.0))?; |
There was a problem hiding this comment.
Would it still be valid testing if we used zero'd vectors?
Also, what about calling these TEST_BATCH_SIZE and so on?
There was a problem hiding this comment.
@javiermtorres with all zeros? we could, but we might get some issues when unit testing things like softmax
Scope creep: