Add the ImageClassificationPipeline#11598
Conversation
| ``./my_model_directory/``. | ||
| - a path or url to a saved feature extractor JSON `file`, e.g., | ||
| ``./my_model_directory/feature_extraction_config.json``. | ||
| ``./my_model_directory/preprocessor_config.json``. |
There was a problem hiding this comment.
Changed this to the actual name of the configuration file
| ("deit", DeiTFeatureExtractor), | ||
| ("s2t", Speech2TextFeatureExtractor), | ||
| ("vit", ViTFeatureExtractor), | ||
| ("wav2vec2", Wav2Vec2FeatureExtractor), | ||
| (DeiTConfig, DeiTFeatureExtractor), | ||
| (Speech2TextConfig, Speech2TextFeatureExtractor), | ||
| (ViTConfig, ViTFeatureExtractor), | ||
| (Wav2Vec2Config, Wav2Vec2FeatureExtractor), |
| # At that point framework might still be undetermined | ||
| model = get_default_model(targeted_task, framework, task_options) | ||
|
|
||
| # Try to infer tokenizer from model or config name (if provided as str) |
There was a problem hiding this comment.
Moved the tokenizer initialization logic beneath the model initialization, with the feature processor. This way it benefits from having access to the model configuration.
| self, | ||
| model: Union["PreTrainedModel", "TFPreTrainedModel"], | ||
| tokenizer: PreTrainedTokenizer, | ||
| tokenizer: Optional[PreTrainedTokenizer] = None, |
There was a problem hiding this comment.
The tokenizer becomes an optional value, alongside the feature_extractor.
| supported_models = [item[1].__name__ for item in supported_models.items()] | ||
| supported_models_names = [] | ||
| for config, model in supported_models.items(): | ||
| # Mapping can now contain tuples of models for the same configuration. | ||
| if isinstance(model, tuple): | ||
| supported_models_names.extend([_model.__name__ for _model in model]) | ||
| else: | ||
| supported_models_names.append(model.__name__) | ||
| supported_models = supported_models_names |
There was a problem hiding this comment.
This was forgotten with the addition of the possibility to have two different models for a single configuration in #11150.
| import os | ||
| from collections import OrderedDict | ||
|
|
||
| from transformers import DeiTFeatureExtractor, Speech2TextFeatureExtractor, ViTFeatureExtractor |
There was a problem hiding this comment.
This enables the use of dummy objects here rather than having None objects
sgugger
left a comment
sgugger
left a comment
There was a problem hiding this comment.
Nice addition! Thanks for working on this!
| supported_models = [item[1].__name__ for item in supported_models.items()] | ||
| supported_models_names = [] | ||
| for config, model in supported_models.items(): | ||
| # Mapping can now contain tuples of models for the same configuration. | ||
| if isinstance(model, tuple): | ||
| supported_models_names.extend([_model.__name__ for _model in model]) | ||
| else: | ||
| supported_models_names.append(model.__name__) | ||
| supported_models = supported_models_names |
patil-suraj
left a comment
patil-suraj
left a comment
There was a problem hiding this comment.
Nice! Thank you for adding this, LGTM.
patrickvonplaten
left a comment
patrickvonplaten
left a comment
There was a problem hiding this comment.
Nice!
The only thing, I feel quite strongly about is to wrap the forward pass into a with torch.no_grad(): to avoid computing costly activations.
The rest is all nits
|
LGTM, thank you for adding this. |
Narsil
left a comment
Narsil
left a comment
There was a problem hiding this comment.
Biggest change is the tokenizer logic modification, we should really respect that if a user sends us something that is not a str(or tuple...) then we need to assume it's a valid tokenizer and pass it as is.
Also the tests are a bit bloated IMO compared to what they could be. In that instance, because we can't test parity with TF, we should just make the tests much simpler.
Also we don't test pipeline outputs which is a big issue for maintaining BC.
The image loading logic should be fixed too.
The rest are NITs.
Co-authored-by: patrickvonplaten <[email protected]>
60b0263 to
25594b5
Compare
|
I should have addressed all comments. @Narsil, if you could review once again when you have time, would love your feedback on the updated tests and on the updated |
Narsil
left a comment
Narsil
left a comment
There was a problem hiding this comment.
LGTM.
Last nit, maybe the load_image function could be put not inlined (like at the module level).
It should make it easier to test and overload.
* Add the ImageClassificationPipeline * Code review Co-authored-by: patrickvonplaten <[email protected]> * Have `load_image` at the module level Co-authored-by: patrickvonplaten <[email protected]>
6 participants
This PR adds the
ImageClassificationPipeline. It is tested on DeiT and ViT and should enable the inference API for these models.Since I encountered an issue with the
AutoExtractor, I fixed it as seen with @sgugger (namely switched it from using names such as"deit"to using the configuration class as a key, similarly to what we do in tokenizers and the other auto classes.Please let me know if you would like me to split this PR into multiple PRs for simpler reviews, happy to do so.
@NielsRogge, happy to have your review.