Use key attributes for segmenter #4985
Conversation
robertbastian
requested review from
sffc,
Manishearth,
aethanyc,
makotokato and
a team
as code owners
|
I think we should remap the names from the lstm_word_segmentation repo to something more concise
|
|
Also, wdyt about making the dictionaries named according to the languages contained in them. |
| ) -> Result<Option<DataPayload<M>>, DataError> { | ||
| match provider.load(DataRequest { | ||
| locale: &locale.into(), | ||
| key_attributes: &model.parse().unwrap(), |
There was a problem hiding this comment.
Nit: We shouldn't need to unwrap anything; I'm surprised Clippy isn't complaining since this is library code.
There was a problem hiding this comment.
This is an infallible FromStr, which clippy apparently realises. The whole DataKeyAttributes construction will be overhauled anyway once we are clearer about requirements. Ideally we'd construct from static str here.
| #[cfg(feature = "provider")] | ||
| fn lstm_model_name_to_data_locale(name: &str) -> Option<DataLocale> { |
There was a problem hiding this comment.
Thought: It's nice that we can get rid of these functions. I suggested remapping with new names, but maybe the new names should be applied deep inside DatagenProvider such that users of DatagenDriver also use the new names.
| let model = crate::lstm_data_locale_to_model_name(req.locale) | ||
| .ok_or(DataErrorKind::MissingLocale.with_req(LstmForWordLineAutoV1Marker::KEY, req))?; |
There was a problem hiding this comment.
This seems like a good place to perform the mapping. Maybe if a long name isn't found, we fall back to the long name? This way it could be easier for customers to add custom models.
|
I'd prefer not to maintain a mapping. Can we change the source repo to use more concise names, or declare short names for each model? |
#4511