Permits TokenClassificationOption / DistilBertForTokenClassification to fail gracefully for an invalid configuration. #320
Conversation
There was a problem hiding this comment.
Thank you for the suggestion @andyHa ,
I have a few minor comments on the DistilBert implementation. This applies to other models, could you please update all instances of unwrapping/expecting in these model instantiations for consistency ? If you do not have the capacity please let me know I'd be happy to support.
Instead of panic, we now return a proper RustBertError so that an invalid model or config thereof wouldn't crash the whole system.
@guillaume-be thanks for the feedback - I updated all methods to consistently return a Result and changed all .expect to .ok_or_else(..)? |
tests/fnet.rs
Outdated
| @@ -227,7 +228,7 @@ fn fnet_for_token_classification() -> anyhow::Result<()> { | |||
|
|
|||
| // Forward pass | |||
| let model_output = no_grad(|| { | |||
| fnet_model | |||
| fnet_model.unwrap() | |||
There was a problem hiding this comment.
can you please be consistent for all tests with the model error handling (see the changes for albert for example):
let albert_model = AlbertForTokenClassification::new(vs.root(), &config)?;
…to fail gracefully for an invalid configuration. (guillaume-be#320) * Properly handle config errors when creating classification models Instead of panic, we now return a proper RustBertError so that an invalid model or config thereof wouldn't crash the whole system. * Properly formats newly added code. * Fixes an example within the documentation * Properly unwraps the newly created results in unit tests. * Fixes some code formatting issues. * Uses proper/idiomatic error handling in unit tests. * Moves the "?" to the correct position.
A production system shouldn't panic, if an invalid config is present. The implementation
is placed in a new method, so that the change is non-breaking.