Query response without answers #2161

ZanSara · 2022-02-10T14:26:37Z

So far, pipelines returning no answers or no documents in response to a query would break Pydantic's validation of the response in the REST API and break them.

This PR makes sure both fields contain at least an empty list (not None) before returning the response.

Closes #1863

review-notebook-app · 2022-02-10T14:48:27Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

…i/haystack into query_response_without_answers

julian-risch

I have some open questions written down in the comments. Other than that, could you please remove the commits about tutorial 6 from this PR?

julian-risch · 2022-02-11T08:48:50Z

tutorials/Tutorial6_Better_Retrieval_via_DPR.ipynb

@@ -384,11 +384,11 @@
   "source": [
    "# !pip install git+https://github.com/deepset-ai/haystack.git#egg=farm-haystack[milvus]\n",
    "\n",
-    "#from haystack.utils import launch_milvus\n",
-    "#from haystack.document_stores import MilvusDocumentStore\n",
+    "# from haystack.utils import launch_milvus\n",


The tutorial 6 changes from #2148 made it in here. We shouldn't do that. Could you please remove them?

julian-risch · 2022-02-11T08:49:00Z

docs/_src/tutorials/tutorials/6.md

@@ -116,13 +116,13 @@ See [their docs](https://milvus.io/docs/v1.0.0/milvus_docker-cpu.md) for more de


 ```python
-!pip install git+https://github.com/deepset-ai/haystack.git#egg=farm-haystack[milvus]
+# !pip install git+https://github.com/deepset-ai/haystack.git#egg=farm-haystack[milvus]


The tutorial 6 changes from #2148 made it in here. We shouldn't do that. Could you please remove them?

julian-risch · 2022-02-11T08:50:28Z

docs/_src/api/openapi/openapi.json

I thought we already beautified the formatting of this file in one of the previous PRs, didn't we?

Nevermind. Just found it in https://github.com/deepset-ai/haystack/pull/2164/files Just make sure that merging this file gives the expected result in the end. I doubt that this will work automatically.

julian-risch · 2022-02-11T08:54:44Z

rest_api/test/test_rest_api.py

@@ -215,6 +215,23 @@ def test_query_with_invalid_filter(populated_client: TestClient):
    assert len(response_json["answers"]) == 0


+def test_query_with_no_documents():


I'd say let's rename to test_query_with_no_documents_and_answers() because this test checks both.

julian-risch · 2022-02-11T08:58:17Z

rest_api/controller/search.py

@@ -65,7 +67,7 @@ def query(request: QueryRequest):
        return result


-def _process_request(pipeline, request) -> QueryResponse:
+def _process_request(pipeline, request) -> Dict[str, Any]:


Why can't we keep QueryResponse here? Line 85 looks like we are making sure it's a QueryResponse.

Yes I thought the same. However, in practice we were returning a dictionary all the time and worse yet, returning a real QueryResponse causes a Pydantic validation error. So I decided to change the type to clarify this.

Let's try to improve on this. The Dict[str, Any] is also inconsistent with the response model in @router.post("/query", response_model=QueryResponse, response_model_exclude_none=True).
@tholor do you have some advice how to make use of QueryResponsehere?

I had a look at the FastAPI docs and it looks like it's designed to handle dictionaries without any issue: https://fastapi.tiangolo.com/tutorial/response-model/ However I agree it's odd that it was failing validation when returning a QueryResponse.

The most important thing is that we declare QueryResponse as the response model on the real endpoints (enables openapi/swagger documentation, validation ...).

On this "helper method" _process_request: I agree that the old version didn't make sense (returning a dict, but annotating with QueryResponse). I think it would make most sense to change the actual returned object to a QueryResponse - What pydantic error did you get there when trying it?

If this is a rabbit hole, switching the return type annotation to Dict is also not too bad here IMO as we still enforce the response model on the higher-level endpoint.

I don't see much value in adding this "conversion-and-back-to-dict step" in line 85. This will happen anyway on-the-fly in the endpoint and is just extra computation time 🤔

Pydantic errors are always hard to figure out. If I implement the method this way:

def _process_request(pipeline, request) -> QueryResponse: ... result = pipeline.run(query=request.query, params=params, debug=request.debug) response = QueryResponse(**result) ... return response

I get this error on every tests that calls /query:

I pushed a commit with the method implemented this way so you can see yourself how the error looks like. For me is really hard to parse 😅 QueryResponse should expect DocumentSerialized and AnswerSerialized objects, or equivalent dicts, not necessarily dicts only... and yet it fails validation. let me know if you've seen this before and what causes it

Here the actual errors: https://github.com/deepset-ai/haystack/runs/5156855217?check_suite_focus=true

I went back to the dictionary type because I still can't find a way to make QueryResponse work, and I think it's a minor issue anyway. Let's keep it like this for now.

…i/haystack into query_response_without_answers

julian-risch

LGTM. As discussed, Dict[str, Any] instead of QueryResponse is okay as we don't expose it to the user.

ZanSara added 2 commits February 10, 2022 15:20

Handle no answers and no documents scenarios in '_process_request'

8545e87

Whitespace

c1ed84c

ZanSara added journey:intermediate topic:rest_api topic:pipeline type:bug Something isn't working labels Feb 10, 2022

Update Documentation & Code Style

493102c

ZanSara added 2 commits February 10, 2022 15:49

Fix tests

3bdf9a9

Merge branch 'query_response_without_answers' of github.com:deepset-a…

b6bc3b5

…i/haystack into query_response_without_answers

ZanSara marked this pull request as ready for review February 10, 2022 14:54

ZanSara requested a review from julian-risch February 10, 2022 14:54

Change return type in '_process_request'

ec20cc5

julian-risch requested changes Feb 11, 2022

View reviewed changes

ZanSara added 3 commits February 11, 2022 11:11

Remove changes from tutorial6

da1cd11

Rename test

914e9c7

Remove tutorial6 changes in the docs

c2fa65f

ZanSara requested a review from julian-risch February 11, 2022 10:18

github-actions bot and others added 9 commits February 11, 2022 10:25

Update Documentation & Code Style

b5eea53

Remove changes from tutorial6

f412a05

Update Documentation & Code Style

3b92ced

Use again QueryResponse to showcase validation failure

96d825d

Merge branch 'query_response_without_answers' of github.com:deepset-a…

38945d1

…i/haystack into query_response_without_answers

Merge branch 'master' into query_response_without_answers

a8b93c9

Update Documentation & Code Style

a0d66f0

Return to use dicts

a20ff72

Merge branch 'query_response_without_answers' of github.com:deepset-a…

06cb00a

…i/haystack into query_response_without_answers

julian-risch approved these changes Feb 16, 2022

View reviewed changes

ZanSara merged commit 13a9bc6 into master Feb 16, 2022

ZanSara deleted the query_response_without_answers branch February 16, 2022 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query response without answers #2161

Query response without answers #2161

ZanSara commented Feb 10, 2022 •

edited

Loading

review-notebook-app bot commented Feb 10, 2022

julian-risch left a comment

julian-risch Feb 11, 2022

julian-risch Feb 11, 2022

julian-risch Feb 11, 2022

julian-risch Feb 11, 2022

julian-risch Feb 11, 2022

julian-risch Feb 11, 2022

ZanSara Feb 11, 2022 •

edited

Loading

julian-risch Feb 11, 2022

ZanSara Feb 11, 2022 •

edited

Loading

tholor Feb 11, 2022

ZanSara Feb 11, 2022 •

edited

Loading

ZanSara Feb 11, 2022

ZanSara Feb 16, 2022

julian-risch left a comment

		@@ -215,6 +215,23 @@ def test_query_with_invalid_filter(populated_client: TestClient):
		assert len(response_json["answers"]) == 0


		def test_query_with_no_documents():

Query response without answers #2161

Query response without answers #2161

Conversation

ZanSara commented Feb 10, 2022 • edited Loading

review-notebook-app bot commented Feb 10, 2022

julian-risch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanSara Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanSara Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanSara Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julian-risch left a comment

Choose a reason for hiding this comment

ZanSara commented Feb 10, 2022 •

edited

Loading

ZanSara Feb 11, 2022 •

edited

Loading

ZanSara Feb 11, 2022 •

edited

Loading

ZanSara Feb 11, 2022 •

edited

Loading