Repository: deepset-ai/haystack
Pull request: Fix loading a saved `FAISSDocumentStore`
Loading a saved `FAISSDocumentStore` resulted in the following error: 
`TypeError: Object of type IndexFlat is not JSON serializable`

To fix this, I removed the `faiss_index` parameter from the config, as this cannot be serialized. 

Closes #1932

Problem statement:
TypeError: Object of type IndexFlat is not JSON serializable
**Describe the bug**
Index fail to be saved after retrieval training and embedding update, getting `TypeError: Object of type IndexFlat is not JSON serializable`. This also corrupt the existing index json configuration file which is more severe in case not backed-up.
Behaviour is same also if loading the trained model and updating the index and saving.

This issue seems to be related to whether FAISSDocumentStore is loaded with `faiss_index: Optional["faiss.swigfaiss.Index"]`, which then added to the `pipeline_config["params"]` and throws the error while this dictionary is attempted to be dumped to the index json configuration. 

**Error message**
```
Traceback (most recent call last):
  File "/data/home/alon_nlp/event-search/src/train/train_dpr_retriever.py", line 82, in <module>
    train()
  File "/data/home/alon_nlp/event-search/src/train/train_dpr_retriever.py", line 34, in train
    run(query_model, passage_model, doc_dir, train_filename, dev_filename,
  File "/data/home/alon_nlp/event-search/src/train/train_dpr_retriever.py", line 67, in run
    document_store.save(faiss_index_path)
  File "/data/home/alon_nlp/haystack/haystack/document_stores/faiss.py", line 533, in save
    json.dump(self.pipeline_config["params"], ipp)
  File "/home/alon_nlp/miniconda3/envs/wec-es/lib/python3.9/json/__init__.py", line 179, in dump
    for chunk in iterable:
  File "/home/alon_nlp/miniconda3/envs/wec-es/lib/python3.9/json/encoder.py", line 431, in _iterencode
    yield from _iterencode_dict(o, _current_indent_level)
  File "/home/alon_nlp/miniconda3/envs/wec-es/lib/python3.9/json/encoder.py", line 405, in _iterencode_dict
    yield from chunks
  File "/home/alon_nlp/miniconda3/envs/wec-es/lib/python3.9/json/encoder.py", line 438, in _iterencode
    o = _default(o)
  File "/home/alon_nlp/miniconda3/envs/wec-es/lib/python3.9/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type IndexFlat is not JSON serializable
```

**Expected behavior**
No error, json is not corrupted 

**Additional context**
Index json before save attempt:
```
{
  "sql_url": "sqlite:///weces_index_for_bert_dpr/weces_dev.db",
  "vector_dim": 768,
  "faiss_index_factory_str": "Flat",
  "return_embedding": false,
  "duplicate_documents": "overwrite",
  "index": "document",
  "similarity": "dot_product",
  "embedding_field": "embedding",
  "progress_bar": true
}
```
Index json after failed save attempt:
```
{"sql_url": "sqlite:///weces_index_for_bert_dpr/weces_dev.db", "vector_dim": 768, "faiss_index_factory_str": "Flat", "faiss_index": 
```

**To Reproduce**
```
document_store = FAISSDocumentStore.load(index_path=faiss_index_path,
                                             config_path=faiss_config_path)

retriever = DensePassageRetriever(document_store=document_store,
                                      query_embedding_model=query_model,
                                      passage_embedding_model=passage_model,
                                      infer_tokenizer_classes=True,
                                      max_seq_len_query=64,
                                      max_seq_len_passage=180
                                      )

retriever.train(data_dir=doc_dir,
                    train_filename=train_filename,
                    dev_filename=dev_filename,
                    test_filename=dev_filename,
                    n_epochs=n_epochs,
                    batch_size=16,
                    grad_acc_steps=8,
                    save_dir=save_dir,
                    evaluate_every=20,
                    embed_title=False,
                    num_positives=1,
                    num_hard_negatives=1
                    )

document_store.update_embeddings(retriever=retriever)
document_store.save(faiss_index_path)
```

**System:**
 - OS: 16.04.1-Ubuntu
 - GPU/CPU: TITAN Xp / Intel(R) Xeon(R) CPU E5-2699A v4 @ 2.40GHz
 - Haystack version (commit or version number): v1.0.0
 - DocumentStore: FAISSDocumentStore
 - Reader: NA
 - Retriever: DensePassageRetriever

Hints from issue discussion:
Hi @AlonEirew! Thanks for raising this issue. The problem was not about training of the retriever, but rather saving a loaded `FAISSDocumentStore`. This should be fixed once #1937 is merged.
Hi @bogdankostic did you have a look at https://github.com/deepset-ai/haystack/issues/1842 while working on this?

Commit message: Add Tests

Patch to review:
diff --git a/haystack/document_stores/faiss.py b/haystack/document_stores/faiss.py
index 7d77f7614b..c375f0aeb2 100644
--- a/haystack/document_stores/faiss.py
+++ b/haystack/document_stores/faiss.py
@@ -106,7 +106,6 @@ def __init__(
             sql_url=sql_url, 
             vector_dim=vector_dim, 
             faiss_index_factory_str=faiss_index_factory_str,
-            faiss_index=faiss_index, 
             return_embedding=return_embedding,
             duplicate_documents=duplicate_documents, 
             index=index, 
diff --git a/test/test_faiss_and_milvus.py b/test/test_faiss_and_milvus.py
index 81bcca8093..af1a3f6e9d 100644
--- a/test/test_faiss_and_milvus.py
+++ b/test/test_faiss_and_milvus.py
@@ -51,6 +51,10 @@ def test_faiss_index_save_and_load(tmp_path):
     # Check if the init parameters are kept
     assert not new_document_store.progress_bar
 
+    # test saving and loading the loaded faiss index
+    new_document_store.save(tmp_path / "haystack_test_faiss")
+    new_document_store.load(tmp_path / "haystack_test_faiss")
+
     # test loading the index via init
     new_document_store = FAISSDocumentStore(faiss_index_path=tmp_path / "haystack_test_faiss")
 
@@ -90,6 +94,10 @@ def test_faiss_index_save_and_load_custom_path(tmp_path):
     # Check if the init parameters are kept
     assert not new_document_store.progress_bar
 
+    # test saving and loading the loaded faiss index
+    new_document_store.save(tmp_path / "haystack_test_faiss", config_path=tmp_path / "custom_path.json")
+    new_document_store.load(tmp_path / "haystack_test_faiss", config_path=tmp_path / "custom_path.json")
+
     # test loading the index via init
     new_document_store = FAISSDocumentStore(faiss_index_path=tmp_path / "haystack_test_faiss", faiss_config_path=tmp_path / "custom_path.json")
 


Code review comments:
diff_hunk: @@ -90,6 +94,10 @@ def test_faiss_index_save_and_load_custom_path(tmp_path):
     # Check if the init parameters are kept
     assert not new_document_store.progress_bar
 
+    # test saving and loading the loaded faiss index
+    new_document_store.save(tmp_path / "haystack_test_faiss", config_path=tmp_path / "custom_path.json")
+    new_document_store.load(tmp_path / "haystack_test_faiss", config_path=tmp_path / "custom_path.json")
path: test/test_faiss_and_milvus.py
text: @user1:
That should be `FAISSDocumentStore.load(...)` instead and we should store the return value in a variable with a different name, e.g., `reloaded_document_store`.

Let's add the assertions afterwards:
```
# check faiss index is restored
assert reloaded_document_store.faiss_indexes[document_store.index].ntotal == len(DOCUMENTS)
# check if documents are restored
assert len(reloaded_document_store.get_all_documents()) == len(DOCUMENTS)
# Check if the init parameters are kept
assert not reloaded_document_store.progress_bar
```

@author:
Done! :)

diff_hunk: @@ -51,6 +51,10 @@ def test_faiss_index_save_and_load(tmp_path):
     # Check if the init parameters are kept
     assert not new_document_store.progress_bar
 
+    # test saving and loading the loaded faiss index
+    new_document_store.save(tmp_path / "haystack_test_faiss")
+    new_document_store.load(tmp_path / "haystack_test_faiss")
path: test/test_faiss_and_milvus.py
text: @user1:
That should be `FAISSDocumentStore.load(...)` instead and we should store the return value in a variable with a different name, e.g., `reloaded_document_store`.

Let's add the assertions afterwards:
```
# check faiss index is restored
assert reloaded_document_store.faiss_indexes[document_store.index].ntotal == len(DOCUMENTS)
# check if documents are restored
assert len(reloaded_document_store.get_all_documents()) == len(DOCUMENTS)
# Check if the init parameters are kept
assert not reloaded_document_store.progress_bar
```

@author:
Done! :)