deepset-ai
fix: `SentenceWindowRetriever` convert metadata fields back to int
Proposed Changes:
Pinecone converts all meta numbers in the meta field to float (https://docs.pinecone.io/guides/data/filter-with-metadata). This causes the SentenceWindowRetriever to crash completely.
This PR checks if the metadata values are floats and converts them back to integers making PineCone supported by the SentenceWindowRetriever
How did you test it?
- unit tests,
- integration tests
- manual verification
Checklist
- I have read the contributors guidelines and the code of conduct
- I have updated the related issue with new insights and changes
- I added unit tests and updated the docstrings
- I've used one of the conventional commit types for my PR title:
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:. - I documented my code
- I ran pre-commit hooks and fixed any issue
Pull Request Test Coverage Report for Build 10594089336
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.2%) to 90.36%
| Totals | |
|---|---|
| Change from base Build 10592675152: | 0.2% |
| Covered Lines: | 7021 |
| Relevant Lines: | 7770 |
💛 - Coveralls
Since this change is related to how Pinecone handles metadata, I think it would be simpler and more appropriate to intervene on the Pinecone side.
Here, for example: https://github.com/deepset-ai/haystack-core-integrations/blob/50352b95e0caeedeb5779eeec3b6a7bd79542d80/integrations/pinecone/src/haystack_integrations/document_stores/pinecone/document_store.py#L270
WDYT?
It's a good suggestion, but we can't just blindly convert everything back to integers.
To make this generic, we need to know which type the values were originally, i.e.: before being stored in Pinecone; and I don't know where to store that information.
Yes, I understand.
I'm simply suggesting that we move the _convert_to_int method to Pinecone with the same keys used here.
Unrelated: I have the impression that the fact that our DocumentSplitter creates a page_number meta field poses risks of overriding user-provided information.
``
Yes, I understand.
I'm simply suggesting that we move the
_convert_to_intmethod to Pinecone with the same keys used here.
ah ok, now I understand what you suggested - the only thing is that for the tests we will need to import Pinecone/add them to the CI
ah ok, now I understand what you suggested - the only thing is that for the tests we will need to import Pinecone/add them to the CI
No, I suggest modifying the Pinecone Document Store in core-integrations.
Here: https://github.com/deepset-ai/haystack-core-integrations/blob/50352b95e0caeedeb5779eeec3b6a7bd79542d80/integrations/pinecone/src/haystack_integrations/document_stores/pinecone/document_store.py#L270