Repository: deepset-ai/haystack
Pull request: Simplify loading of `EmbeddingRetriever`
This PR adds a method to the `EmbeddingRetriever` to infer the model_format parameter automatically from the model's config files if no model_format is provided by the user.

Closes #2406


Problem statement:
Simplify loading of `EmbeddingRetriever`
Loading of `EmbeddingRetriever`s is quite complex, e.g. it is quite confusing to determine which model to use with which model format (we can use model format `"sentence_transformers"` with model that are on the HF model hub (e.g. `sentence-transformers/all-mpnet-base-v2`)). 

Also, it might make sense to deprecate FARM style loading of models eventually.

Hints from issue discussion:
I just had an in-depth look into the `EmbeddingRetriever` and came to the conclusion that it might be possible to drop the `model_format` entirely. Currently, we support the following formats: `"farm"`, `"transformers"`, `"sentence_transformers"` and `"retribert"`.

- Both `"farm"` and `"transformers"` use the `_DefaultEmbeddingEncoder` -> these two cases can be merged
- As pointed out in the [SentenceTransformers documentation](https://www.sbert.net/docs/pretrained_models.html#pretrained-models), all `"sentence_transformers"` models are hosted on the HF model hub. Therefore, these models can also be used by `_DefaultEmbeddingEncoder`. To be able to load local sentence transformers models, we would need to make sure [here](https://github.com/deepset-ai/haystack/blob/5a1e98e3ff6f30faba06c8d7dc337158ab6fc4a0/haystack/modeling/infer.py#L184) that we either use `InferenceProcessor.load_from_dir` (if the model is in farm format) or `Processor.convert_from_transformers` (if the model is in HF transformers format)
- `"retribert"` format can be inferred either from the model name or from `model_type` in `config.json`

As a result, we would just need `_DefaultEmbeddingEncoder` (for `"farm"`, `"transformers"` and `"sentence_transformers"` format) and `_RetribertEmbeddingEncoder` (for `"retribert"` format, which can be inferred from model name or `model_type` in `config.json`).

One concern I still have with this approach is that apparently, as noted in #2301, `_SentenceTransformersEmbeddingEncoder` is a lot faster than `_DefaultEmbeddingEncoder`. I think the reason for this is that the default setting for `max_seq_len` of the `EmbeddingRetriever` is 512. In `_DefaultEmbeddingEncoder`, each input is always padded to `max_seq_len` (512 for the default case), while in `_SentenceTransformersEmbeddingEncoder`, the input texts are sorted by length and each input is only padded to the length of the longest sequence in the same batch.
One concern that I would have is that there might be a future mismatch in sentence transformers and our `_DefaultEmbeddingEncoder` in terms of features. This would mean that we'd have to implement additional features if sentence transformers adds any. I also would not want to miss out on the speed advantage.
Another reason why not to remove `_SentenceTransformersEmbeddingEncoder`: In #2388, we are currently implementing GPL, but only for the `_SentenceTransformersEmbeddingEncoder`.
We decided to keep all `_DefaultEmbeddingEncoder`, `_SentenceTransformersEmbeddingEncoder` and `_RetribertEmbeddingEncoder`.

We also decided to infer the `model_format` parameter automatically like this:

1. If `config_sentence_transformers.json` in model path or in model hub repo, use `_SentenceTransformersEmbeddingEncoder`.
2. If `model_type` == `"retribert"` (in `config.json`), use  `_RetribertEmbeddingEncoder`.
3. Else, use `_DefaultEmbeddingEncoder`.

Commit message: Update Documentation & Code Style

Patch to review:
diff --git a/docs/_src/api/api/retriever.md b/docs/_src/api/api/retriever.md
index 8cfe87ab81..43cdfae618 100644
--- a/docs/_src/api/api/retriever.md
+++ b/docs/_src/api/api/retriever.md
@@ -1171,7 +1171,7 @@ class EmbeddingRetriever(BaseRetriever)
 #### EmbeddingRetriever.\_\_init\_\_
 
 ```python
-def __init__(document_store: BaseDocumentStore, embedding_model: str, model_version: Optional[str] = None, use_gpu: bool = True, batch_size: int = 32, max_seq_len: int = 512, model_format: str = "farm", pooling_strategy: str = "reduce_mean", emb_extraction_layer: int = -1, top_k: int = 10, progress_bar: bool = True, devices: Optional[List[Union[str, torch.device]]] = None, use_auth_token: Optional[Union[str, bool]] = None, scale_score: bool = True, embed_meta_fields: List[str] = [])
+def __init__(document_store: BaseDocumentStore, embedding_model: str, model_version: Optional[str] = None, use_gpu: bool = True, batch_size: int = 32, max_seq_len: int = 512, model_format: Optional[str] = None, pooling_strategy: str = "reduce_mean", emb_extraction_layer: int = -1, top_k: int = 10, progress_bar: bool = True, devices: Optional[List[Union[str, torch.device]]] = None, use_auth_token: Optional[Union[str, bool]] = None, scale_score: bool = True, embed_meta_fields: List[str] = [])
 ```
 
 **Arguments**:
@@ -1182,10 +1182,14 @@ def __init__(document_store: BaseDocumentStore, embedding_model: str, model_vers
 - `use_gpu`: Whether to use all available GPUs or the CPU. Falls back on CPU if no GPU is available.
 - `batch_size`: Number of documents to encode at once.
 - `max_seq_len`: Longest length of each document sequence. Maximum number of tokens for the document text. Longer ones will be cut down.
-- `model_format`: Name of framework that was used for saving the model. Options:
+- `model_format`: Name of framework that was used for saving the model or model type. If no model_format is
+provided, it will be inferred automatically from the model configuration files.
+Options:
+
 - ``'farm'``
 - ``'transformers'``
 - ``'sentence_transformers'``
+- ``'retribert'``
 - `pooling_strategy`: Strategy for combining the embeddings from the model (for farm / transformers models only).
 Options:
 
diff --git a/haystack/json-schemas/haystack-pipeline-master.schema.json b/haystack/json-schemas/haystack-pipeline-master.schema.json
index f61a907b56..fa117e876c 100644
--- a/haystack/json-schemas/haystack-pipeline-master.schema.json
+++ b/haystack/json-schemas/haystack-pipeline-master.schema.json
@@ -2265,7 +2265,6 @@
             },
             "model_format": {
               "title": "Model Format",
-              "default": "farm",
               "type": "string"
             },
             "pooling_strategy": {
diff --git a/haystack/nodes/retriever/dense.py b/haystack/nodes/retriever/dense.py
index a4ebf7a1a7..dfdc1f8d50 100644
--- a/haystack/nodes/retriever/dense.py
+++ b/haystack/nodes/retriever/dense.py
@@ -3,6 +3,7 @@
 import logging
 from pathlib import Path
 from copy import deepcopy
+from requests.exceptions import HTTPError
 
 import numpy as np
 from tqdm.auto import tqdm
@@ -11,6 +12,8 @@
 from torch.nn import DataParallel
 from torch.utils.data.sampler import SequentialSampler
 import pandas as pd
+from huggingface_hub import hf_hub_download
+from transformers import AutoConfig
 
 from haystack.errors import HaystackError
 from haystack.schema import Document
@@ -1452,7 +1455,7 @@ def __init__(
         use_gpu: bool = True,
         batch_size: int = 32,
         max_seq_len: int = 512,
-        model_format: str = "farm",
+        model_format: Optional[str] = None,
         pooling_strategy: str = "reduce_mean",
         emb_extraction_layer: int = -1,
         top_k: int = 10,
@@ -1469,11 +1472,14 @@ def __init__(
         :param use_gpu: Whether to use all available GPUs or the CPU. Falls back on CPU if no GPU is available.
         :param batch_size: Number of documents to encode at once.
         :param max_seq_len: Longest length of each document sequence. Maximum number of tokens for the document text. Longer ones will be cut down.
-        :param model_format: Name of framework that was used for saving the model. Options:
+        :param model_format: Name of framework that was used for saving the model or model type. If no model_format is
+                             provided, it will be inferred automatically from the model configuration files.
+                             Options:
 
                              - ``'farm'``
                              - ``'transformers'``
                              - ``'sentence_transformers'``
+                             - ``'retribert'``
         :param pooling_strategy: Strategy for combining the embeddings from the model (for farm / transformers models only).
                                  Options:
 
@@ -1514,7 +1520,6 @@ def __init__(
 
         self.document_store = document_store
         self.embedding_model = embedding_model
-        self.model_format = model_format
         self.model_version = model_version
         self.use_gpu = use_gpu
         self.batch_size = batch_size
@@ -1525,19 +1530,24 @@ def __init__(
         self.progress_bar = progress_bar
         self.use_auth_token = use_auth_token
         self.scale_score = scale_score
+        self.model_format = self._infer_model_format(embedding_model) if model_format is None else model_format
 
         logger.info(f"Init retriever using embeddings of model {embedding_model}")
 
-        if model_format not in _EMBEDDING_ENCODERS.keys():
+        if self.model_format not in _EMBEDDING_ENCODERS.keys():
             raise ValueError(f"Unknown retriever embedding model format {model_format}")
 
-        if self.embedding_model.startswith("sentence-transformers") and self.model_format != "sentence_transformers":
+        if (
+            self.embedding_model.startswith("sentence-transformers")
+            and model_format
+            and model_format != "sentence_transformers"
+        ):
             logger.warning(
                 f"You seem to be using a Sentence Transformer embedding model but 'model_format' is set to '{self.model_format}'."
-                f" You may need to set 'model_format='sentence_transformers' to ensure correct loading of model."
+                f" You may need to set model_format='sentence_transformers' to ensure correct loading of model."
             )
 
-        self.embedding_encoder = _EMBEDDING_ENCODERS[model_format](self)
+        self.embedding_encoder = _EMBEDDING_ENCODERS[self.model_format](self)
         self.embed_meta_fields = embed_meta_fields
 
     def retrieve(
@@ -1817,3 +1827,25 @@ def _preprocess_documents(self, docs: List[Document]) -> List[Document]:
             doc.content = "\n".join(meta_data_fields + [doc.content])
             linearized_docs.append(doc)
         return linearized_docs
+
+    @staticmethod
+    def _infer_model_format(model_name_or_path: str) -> str:
+        # Check if model name is a local directory with sentence transformers config file in it
+        if Path(model_name_or_path).exists():
+            if Path(f"{model_name_or_path}/config_sentence_transformers.json").exists():
+                return "sentence_transformers"
+        # Check if sentence transformers config file in model hub
+        else:
+            try:
+                hf_hub_download(repo_id=model_name_or_path, filename="config_sentence_transformers.json")
+                return "sentence_transformers"
+            except HTTPError:
+                pass
+
+        # Check if retribert model
+        config = AutoConfig.from_pretrained(model_name_or_path)
+        if config.model_type == "retribert":
+            return "retribert"
+
+        # Model is neither sentence-transformers nor retribert model -> use _DefaultEmbeddingEncoder
+        return "farm"
diff --git a/test/conftest.py b/test/conftest.py
index 2f4f8902ad..ded06bb3e1 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -674,10 +674,7 @@ def get_retriever(retriever_type, document_store):
         )
     elif retriever_type == "retribert":
         retriever = EmbeddingRetriever(
-            document_store=document_store,
-            embedding_model="yjernite/retribert-base-uncased",
-            model_format="retribert",
-            use_gpu=False,
+            document_store=document_store, embedding_model="yjernite/retribert-base-uncased", use_gpu=False
         )
     elif retriever_type == "dpr_lfqa":
         retriever = DensePassageRetriever(
diff --git a/test/document_stores/test_document_store.py b/test/document_stores/test_document_store.py
index 59b7f9e8bb..956f1b22ac 100644
--- a/test/document_stores/test_document_store.py
+++ b/test/document_stores/test_document_store.py
@@ -1391,7 +1391,9 @@ def test_elasticsearch_synonyms():
 @pytest.mark.embedding_dim(384)
 def test_similarity_score(document_store_with_docs):
     retriever = EmbeddingRetriever(
-        document_store=document_store_with_docs, embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2"
+        document_store=document_store_with_docs,
+        embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2",
+        model_format="farm",
     )
     document_store_with_docs.update_embeddings(retriever)
     pipeline = DocumentSearchPipeline(retriever)
@@ -1411,6 +1413,7 @@ def test_similarity_score_without_scaling(document_store_with_docs):
         document_store=document_store_with_docs,
         embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2",
         scale_score=False,
+        model_format="farm",
     )
     document_store_with_docs.update_embeddings(retriever)
     pipeline = DocumentSearchPipeline(retriever)
@@ -1430,6 +1433,7 @@ def test_similarity_score_dot_product(document_store_dot_product_with_docs):
     retriever = EmbeddingRetriever(
         document_store=document_store_dot_product_with_docs,
         embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2",
+        model_format="farm",
     )
     document_store_dot_product_with_docs.update_embeddings(retriever)
     pipeline = DocumentSearchPipeline(retriever)
@@ -1449,6 +1453,7 @@ def test_similarity_score_dot_product_without_scaling(document_store_dot_product
         document_store=document_store_dot_product_with_docs,
         embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2",
         scale_score=False,
+        model_format="farm",
     )
     document_store_dot_product_with_docs.update_embeddings(retriever)
     pipeline = DocumentSearchPipeline(retriever)
diff --git a/test/nodes/test_retriever.py b/test/nodes/test_retriever.py
index 5c817fb2ed..36b5c1ec56 100644
--- a/test/nodes/test_retriever.py
+++ b/test/nodes/test_retriever.py
@@ -591,7 +591,9 @@ def test_embeddings_encoder_of_embedding_retriever_should_warn_about_model_forma
 
     with caplog.at_level(logging.WARNING):
         EmbeddingRetriever(
-            document_store=document_store, embedding_model="sentence-transformers/paraphrase-multilingual-mpnet-base-v2"
+            document_store=document_store,
+            embedding_model="sentence-transformers/paraphrase-multilingual-mpnet-base-v2",
+            model_format="farm",
         )
 
         assert (


Code review comments:
diff_hunk: @@ -1391,7 +1391,9 @@ def test_elasticsearch_synonyms():
 @pytest.mark.embedding_dim(384)
 def test_similarity_score(document_store_with_docs):
     retriever = EmbeddingRetriever(
-        document_store=document_store_with_docs, embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2"
+        document_store=document_store_with_docs,
+        embedding_model="sentence-transformers/paraphrase-MiniLM-L3-v2",
+        model_format="farm",
path: test/document_stores/test_document_store.py
text: @author:
`_DefaultEmbeddingEncoder` and `_SentenceTransformersEmbeddingEncoder` produce different scores, so to fix this test, I set `model_format="farm"` here. The alternative would be to not set `model_format` (i.e. make use of `_SentenceTransformersEmbeddingEncoder` by inferencing the format from the configs) and change the expected scores.

@user1:
I think because of the speed advantage of `_SentenceTransformersEmbeddingEncoder` over `_DefaultEmbeddingEncoder` we will use `_SentenceTransformersEmbeddingEncoder` more often for sentence-transformers models in practice. Our tutorials most often also use `_SentenceTransformersEmbeddingEncoder` explicitly. Therefore, I'd say we should also focus on testing `_SentenceTransformersEmbeddingEncoder` and remove the `model_format` and change the expected scores. It would be best to test both `_DefaultEmbeddingEncoder` and `_SentenceTransformersEmbeddingEncoder`.

@user1:
I am afraid that we currently have no test coverage of `_SentenceTransformersEmbeddingEncoder`. This is something we should definitely change before merging this PR.