Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mocking Pinecone tests #2778

Merged
merged 15 commits into from
Jul 14, 2022
Merged

Mocking Pinecone tests #2778

merged 15 commits into from
Jul 14, 2022

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Jul 8, 2022

Related Issue(s): #2644

Proposed changes:

Pre-flight checklist

@ZanSara ZanSara added type:bug Something isn't working topic:tests ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:pinecone labels Jul 8, 2022
self.pinecone_indexes[index].delete(delete_all=True)
self.pinecone_indexes[index].delete()
Copy link
Contributor Author

@ZanSara ZanSara Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamescalam: the mock did not accept this parameter. As I imagine the mock represents the actual contract with Pinecone, I assumed this might be a leftover from older versions and removed it.

Is that true? If not, I will revert this change and add delete_all to the mock.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete_all should be included in the mock

@ZanSara
Copy link
Contributor Author

ZanSara commented Jul 8, 2022

@jamescalam: Your mock works really nicely! Thank you so much for it 😊

I have only one test failing that I can't understand well:

def test_update_meta(document_store: BaseDocumentStore):

test/document_stores/test_document_store.py::test_update_meta[pinecone] - ValueError: Namespace not found

I think you should be able to clone this branch and run the test locally to see the issue, otherwise you can see the logs here: https://github.com/deepset-ai/haystack/actions/runs/2634797675 How can I fix the mock to make this test pass?

Copy link
Contributor

@jamescalam jamescalam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this line with pass to fix:

raise ValueError("Namespace not found")

This aligns with Pinecone where if we query an empty/non-existent namespace in Pinecone it will just return an empty response (not raise a ValueError)

@ZanSara ZanSara marked this pull request as ready for review July 14, 2022 09:04
@jamescalam
Copy link
Contributor

jamescalam commented Jul 14, 2022

Replace this line with pass to fix:

raise ValueError("Namespace not found")

This aligns with Pinecone where if we query an empty/non-existent namespace in Pinecone it will just return an empty response (not raise a ValueError)

Sorry, it should be return response as it needs to return the empty response format we define at the start of the method (this is what Pinecone would return)

@jamescalam
Copy link
Contributor

@ZanSara the Tests / pinecone-tests-linux error seems valid, I will look at the document store code to fix

@jamescalam
Copy link
Contributor

@ZanSara this should work with the newer document_store #2749 - it will not work with the old one because the format of fetch requests have changed

@ZanSara ZanSara marked this pull request as draft July 14, 2022 13:48
@ZanSara ZanSara marked this pull request as ready for review July 14, 2022 13:48
Copy link
Contributor

@jamescalam jamescalam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for Pinecone doc store + mock 😁

@ZanSara
Copy link
Contributor Author

ZanSara commented Jul 14, 2022

Great, thanks! I will merge this as soon as that strange failure with the rest api tests is sorted (we just enabled a new branch protection rule for master and is not going as smoothly as we hoped 😅 )

pip install rest_api/
pip install ui/
pip install -U rest_api/
pip install -U ui/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bingo! :)

@ZanSara ZanSara merged commit 6b39fbd into master Jul 14, 2022
@ZanSara ZanSara deleted the mock-pinecone-tests branch July 14, 2022 18:03
Krak91 pushed a commit to Krak91/haystack that referenced this pull request Jul 26, 2022
* Integrating the mock into conftest.py

* re-enable workflow

* delete_all

* Update Documentation & Code Style

* remove ValueError

* Add empty response

* wrong condition

* return response

* revert removal of delete_all

* change mock

* Update Documentation & Code Style

* test for rest api, to revert

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:pinecone topic:tests type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants