Handle invalid metadata for `SQLDocumentStore` #2868

anakin87 · 2022-07-21T22:09:02Z

Related Issue(s): #2792

Proposed changes:

Skip writing metadata if they are not automatically cast to string by SqlAlchemy
Valid types are str, int, float, bool, bytes, bytearray, NoneType
Added specific test

Pre-flight checklist

I have read the contributors guidelines
I have enabled actions on my fork
If this is a code change, I added tests or updated existing ones
If this is a code change, I updated the docstrings

In #2792, we seem to agree that it would be good to have a more expressive format for metadata value (not simply string).
We also understand that it would be a complex change that could break several things.

So to avoid blocking errors, for now, the best solution seems to skip writing metadata that is incorrectly managed by SqlAlchemy.
And I've started this implementation...
(Another possibility is to write a string representation in the DB even for complex metadata.)

I make the PR a draft, to run all the tests...

…ndle_nostring_metadata_sqlstore

ZanSara

Looks promising! That's a nice default behavior for the SQLDocumentStore for now. Left a few comments for improvements

ZanSara · 2022-07-22T07:51:16Z

haystack/document_stores/sql.py

+                    if value is None or isinstance(value, self.valid_metadata_types):
+                        valid_meta_orms.append(MetaDocumentORM(name=name, value=value))
+                    else:
+                        logger.warning(
+                            f"Metadata '{name}' skipped for document {doc.id}, since it has invalid type: {type(value).__name__}.\n"
+                            f"SQLDocumentStore accepts only the following types: {', '.join([el.__name__ for el in self.valid_metadata_types])}"
+                        )


For speed reason I'd use a try-catch in here. It also saves you from specifying the allowed types in line 112.

In addition, this is quite a bad error imho, so I'd raise the log to ERROR or even EXCEPTION. Personally, if this is an issue that requires action from the users in almost all cases, I'd go for EXCEPTION. If you see any situation in which the users might want to ignore that for a good reason, then ERROR is better (the only difference between the two is that exception shows the stack trace while error doesn't).

Agree on logging an ERROR. I think that there are cases when the user simply wants to ignore this error (see SQL based Datastores fail when document metadata has a list #2792 initial issue).

About try-catch: the problem is that the error only shows up, during the first writing operation of the whole record, eg here:

haystack/haystack/document_stores/sql.py

Line 401 in 8ee2b6b

self.session.query(MetaDocumentORM).filter_by(document_id=doc.id).delete()

IMHO, we could properly use try-catch, if the exception was raised during the creation of MetaDocumentORM.
What do you think?

Oh ok, I see the point. There are two ways to go:

This one (just fine), or

See if SQLAlchemy offers any hook to deal with unsupported types, like json and Pydantic do. Do you mind having a look? If the library offers no help there, we can stick with your original solution.

ZanSara · 2022-07-22T07:57:24Z

test/document_stores/test_document_store.py

@@ -453,6 +453,32 @@ def test_write_document_meta(document_store: BaseDocumentStore):
    assert document_store.get_document_by_id("4").meta["meta_field"] == "test4"


+@pytest.mark.parametrize("document_store", ["sql", "faiss", "milvus1", "milvus"], indirect=True)


I would test all docstores here, so we're sure there is a consistent behavior across all of them.

I wanted to test only docstores, whose metadata are written in SQL databases.
For other docstores, I think that we don't have this error...

Oh you're right, because the other docstores would be happy to deal with lists in the meta, so they won't show this error.

Then we might even want to test only sql. I got thinking here because, for example. pinecone uses SQLDocumentStore as well, and keeping track of all subclasses would be prone to errors. Let's just test sql, it should be enough.

ZanSara · 2022-07-22T08:31:04Z

test/document_stores/test_document_store.py

+    assert not "invalid_meta_field" in document_store.get_document_by_id("1").meta
+    assert document_store.get_document_by_id("1").meta["valid_meta_field"] == "test1"
+    assert not "invalid_meta_field" in document_store.get_document_by_id("2").meta
+    assert document_store.get_document_by_id("2").meta["valid_meta_field"] == "test2"


Just to make it slightly stricter, how about testing the whole dictionary?

Suggested change

assert not "invalid_meta_field" in document_store.get_document_by_id("1").meta

assert document_store.get_document_by_id("1").meta["valid_meta_field"] == "test1"

assert not "invalid_meta_field" in document_store.get_document_by_id("2").meta

assert document_store.get_document_by_id("2").meta["valid_meta_field"] == "test2"

assert document_store.get_document_by_id("1").meta == {"valid_meta_field": "test1"}

assert document_store.get_document_by_id("2").meta == {"valid_meta_field": "test2"}

…ndle_nostring_metadata_sqlstore

…e' into handle_nostring_metadata_sqlstore

anakin87 · 2022-07-22T15:24:59Z

See if SQLAlchemy offers any hook to deal with unsupported types, like json and Pydantic do.

@ZanSara Generally, SQLAlchemy prefers to let these checks to the DBs, but I found simple validators!
So now, the validation is defined in MetaDocumentORM. I found this solution simpler and cleaner 🙂

Maybe the error handling and logging can be improved, even if now I think it's not so bad.

ERROR - haystack.document_stores.sql - Document 4 - Discarded metadata 'strange_meta_2', since it has invalid type: tuple.
SQLDocumentStore can accept and cast to string only the following types: str, int, float, bool, bytes, bytearray, NoneType

I also improved the test...

ZanSara

Nice work, good to go! The validation hooks for SQLAlchemy are really neat. I agree the database should always have the last word, but a bit of proactive validation won't hurt and saves a scary error to the users, so I really like it.

Thank you very much @anakin87 😊

* modify notebook * skip invalid metadata * Update Documentation & Code Style * fix nonetype * fix nonetype * drop nonetype from valid types * drop nonetype from valid types * fix * Update sql.py * sqlalchemy validation * removed newlines * Update Documentation & Code Style Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

anakin87 and others added 4 commits July 20, 2022 21:54

modify notebook

559fa02

Merge branch 'master' of https://github.com/anakin87/haystack into ha…

2699acb

…ndle_nostring_metadata_sqlstore

skip invalid metadata

55db1e3

Update Documentation & Code Style

9630b83

anakin87 marked this pull request as draft July 21, 2022 22:09

anakin87 and others added 7 commits July 22, 2022 00:12

fix nonetype

5b64cce

fix nonetype

bdba198

drop nonetype from valid types

a2fc297

drop nonetype from valid types

72d8de3

fix

07e9747

fix

1f4d317

Update sql.py

fcd047d

ZanSara reviewed Jul 22, 2022

View reviewed changes

ZanSara added type:bug Something isn't working topic:sql topic:metadata topic:document_store journey:intermediate labels Jul 22, 2022

anakin87 and others added 7 commits July 22, 2022 15:23

Merge branch 'deepset-ai:master' into master

cc30e3e

Merge branch 'master' of https://github.com/anakin87/haystack into ha…

4e207aa

…ndle_nostring_metadata_sqlstore

sqlalchemy validation

d68c845

Merge branch 'deepset-ai:master' into handle_nostring_metadata_sqlstore

7f17903

Merge remote-tracking branch 'origin/handle_nostring_metadata_sqlstor…

657cf50

…e' into handle_nostring_metadata_sqlstore

removed newlines

137bac3

Update Documentation & Code Style

7a9efbd

anakin87 marked this pull request as ready for review July 22, 2022 15:25

ZanSara approved these changes Jul 25, 2022

View reviewed changes

ZanSara merged commit 7dcef68 into deepset-ai:master Jul 25, 2022

anakin87 deleted the handle_nostring_metadata_sqlstore branch July 25, 2022 13:11

anakin87 mentioned this pull request Oct 5, 2022

fix: Flatten DocumentClassifier output in SQLDocumentStore; remove _sql_session_rollback hack in tests #3273

Merged

6 tasks

anakin87 mentioned this pull request Oct 20, 2022

Metadata values are being dropped when list of values are provided #3434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid metadata for `SQLDocumentStore` #2868

Handle invalid metadata for `SQLDocumentStore` #2868

anakin87 commented Jul 21, 2022 •

edited by ZanSara

Loading

ZanSara left a comment

ZanSara Jul 22, 2022

anakin87 Jul 22, 2022

ZanSara Jul 22, 2022

ZanSara Jul 22, 2022

anakin87 Jul 22, 2022

ZanSara Jul 22, 2022

ZanSara Jul 22, 2022

anakin87 commented Jul 22, 2022

ZanSara left a comment

		@@ -453,6 +453,32 @@ def test_write_document_meta(document_store: BaseDocumentStore):
		assert document_store.get_document_by_id("4").meta["meta_field"] == "test4"


		@pytest.mark.parametrize("document_store", ["sql", "faiss", "milvus1", "milvus"], indirect=True)

Handle invalid metadata for SQLDocumentStore #2868

Handle invalid metadata for SQLDocumentStore #2868

Conversation

anakin87 commented Jul 21, 2022 • edited by ZanSara Loading

Pre-flight checklist

ZanSara left a comment

Choose a reason for hiding this comment

ZanSara Jul 22, 2022

Choose a reason for hiding this comment

anakin87 Jul 22, 2022

Choose a reason for hiding this comment

ZanSara Jul 22, 2022

Choose a reason for hiding this comment

ZanSara Jul 22, 2022

Choose a reason for hiding this comment

anakin87 Jul 22, 2022

Choose a reason for hiding this comment

ZanSara Jul 22, 2022

Choose a reason for hiding this comment

ZanSara Jul 22, 2022

Choose a reason for hiding this comment

anakin87 commented Jul 22, 2022

ZanSara left a comment

Choose a reason for hiding this comment

Handle invalid metadata for `SQLDocumentStore` #2868

Handle invalid metadata for `SQLDocumentStore` #2868

anakin87 commented Jul 21, 2022 •

edited by ZanSara

Loading