WIP: Feature/api list document #880

ankh6 · 2021-03-08T21:45:08Z

Hey !
Here's the pull request regarding issue #797
I am eagerly waiting for your feedback !

haystack/document_store/sql.py

lalitpagaria · 2021-03-08T22:25:20Z

rest_api/controller/documents.py

+from fastapi import APIRouter, HTTPException
+import logging
+
+sql = sql.SQLDocumentStore()


I think we already initialising document store at https://github.com/deepset-ai/haystack/blob/master/rest_api/controller/search.py#L38 , so we can skip initialisation here or we can refactor that piece of code a bit

lalitpagaria · 2021-03-08T22:26:36Z

rest_api/controller/documents.py

+sql = sql.SQLDocumentStore()
+now = datetime.now()
+SUCCESS: int = 200
+CREATED: int = 201


We can directly use HTTPStatus enum.

rest_api/controller/documents.py

lalitpagaria · 2021-03-08T22:28:58Z

rest_api/controller/documents.py

+    :param payload: input from the client to create a document.
+    """
+    logger.info("Constructing document object")
+    created_document = Document.from_dict(payload)


I think you yet to add implementation to add document

lalitpagaria · 2021-03-08T22:30:45Z

rest_api/controller/documents.py

+    """
+    try:
+        logger.info("Retrieving documents")
+        retrieved_documents = sql.get_all_documents()


We can build pagination on top of generator.

haystack/haystack/document_store/elasticsearch.py

Line 508 in 6d00eff

def get_all_documents_generator(

lalitpagaria · 2021-03-08T22:31:44Z

rest_api/controller/documents.py

+            raise Exception(f"Something went wrong while retrieving documents", e.message)
+
+
+@router.get("/documents/{id}", status_code=SUCCESS)


We can return response_model so open api spec will render correctly also to other APIs as well

lalitpagaria · 2021-03-08T22:35:54Z

rest_api/controller/documents.py

+    try:
+        retrieved_document = sql.get_document_by_id(id)
+        if not retrieved_document:
+            raise HTTPException(status_code=404, detail=f"No document with id {id} could be found")


Exception raise here will be caught by line no 63, and that will convert it into normal Exception at line no 67

HTTPException is not a generic Python exception, meaning an exception will be raised and the code will terminate with exit code != 0. We won't go further in the calling chain

Not sure about but FastAPI HTTPException is sub class of Exception.
https://github.com/tiangolo/fastapi/blob/4d208b2b9035e24bdf80505571b5b1bac8f9ae7a/fastapi/exceptions.py#L8
https://github.com/encode/starlette/blob/bfa61ad92b63b6929e485f2189fd299f94189b09/starlette/exceptions.py

lalitpagaria · 2021-03-08T22:37:33Z

rest_api/controller/documents.py

+    :param payload: input from the client to create a Document
+    """
+    try:
+        if not id:


id can never be None so no need to check for that here.

lalitpagaria · 2021-03-08T22:38:57Z

rest_api/controller/documents.py

+    :param id: UUID pointing to the document to retrieve
+    """
+    try:
+        if not id:


Same as https://github.com/deepset-ai/haystack/pull/880/files#r589799389

lalitpagaria · 2021-03-08T22:42:27Z

rest_api/controller/documents.py

+            raise HTTPException(status_code=404, detail=f"No document with id {id} could be found")
+        sql.delete_document_by_id(id)
+        cursor = sql.SQLDocumentStore(update_existing_documents=True)
+        document_to_be_updated = cursor.write_documents(payload.to_dict())


We can add update_existing_documents to write_documents as an optional param.
Also we need to make sure payload["id"] == id otherwise document store will generate random UUID

write_document() does not accept update_existing_document as an input parameter. That is the reason why I had to instantiate another instance of the SQLDocumentStore. Plus, setting it to True makes sure that any existing document with an id gets updated (per SQLDocumentStore docstring).
Is that correct ?

Yes I mean how about updating write_documents function which accept update_existing_documents as optional param

lalitpagaria · 2021-03-08T22:46:57Z

@ankh6 Thank you for working on it. I have few review comments.
Few general are -

We can add delete_document_by_id as abstract function in BaseDocumentStore
Can you please remove formatting changes from this PR as difficult to understand the actual code changes
We can add tests as well

@tholor We can fix coding style, which user can integrate with IDE. Also CI can also run style check. https://realpython.com/python-code-quality/

ankh6 · 2021-03-09T21:36:47Z

An error is raised when trying to run the test suite with pytest test -m "not elasticsearch"

ImportError while loading conftest '/home/andy/opensource/deepset_ai_haystack/haystack/test/conftest.py'.
test/conftest.py:8: in
from elasticsearch import Elasticsearch
E ModuleNotFoundError: No module named 'elasticsearch'

Timoeller · 2021-03-17T17:50:50Z

Hey @ankh6 thanks for the PR, this is a really useful feature.
Also big thanks to @lalitpagaria for the thorough review, the points make total sense.

Do you, ankh6 want to work on those suggestions? Otherwise @oryx1729 would take over soon.

ankh6 · 2021-03-17T20:01:35Z

@Timoeller I am working on it. I'll make another PR this evening.
I am writing some unit tests. I'll be done today :)

ankh6 · 2021-03-17T22:13:33Z

@Timoeller Changes have been pushed.
I am waiting for your feedback

Timoeller · 2021-03-18T16:56:43Z

Hey cool, thanks for the changes.
There are still a couple of Lalits comments left, would you also like to work on those?
Also the API test is failing related to some pydantic problem. Do you need help there?

If you work on those comments we can resolve them and let Lalit or @oryx1729 do another round of review. But we are getting into good shape here. Thanks again for the work!

ankh6 · 2021-03-18T21:28:27Z

@Timoeller. Yes, I'll keep working on that. I want my branch to be merged :)

All comments written by Lalit have been tackled in the latest commit. Which points are you referring to ?
Yes, the pipeline did break for a minor issue. I'd appreciate having guidance
As pointed in previous comment (9 days ago) I am not able to run the automated test suite locally. I pulled the latest code but still. Errors are raised. These error come from module I have not worked on:

Timoeller · 2021-03-19T12:26:22Z

There are still a couple of Lalits comments left, would you also like to work on those?

Hey, yes you are right, I must have looked at an older version of your code.

@oryx1729 could you please take a look at the pydantic error and rest api changes? We will need your review before we can merge here.

lalitpagaria · 2021-03-19T16:40:43Z

An error is raised when trying to run the test suite with pytest test -m "not elasticsearch"

ImportError while loading conftest '/home/andy/opensource/deepset_ai_haystack/haystack/test/conftest.py'.
test/conftest.py:8: in
from elasticsearch import Elasticsearch
E ModuleNotFoundError: No module named 'elasticsearch'

@ankh6 what error you get after you install elasticsearch>=7.7,<=7.10?
not elasticsearch here means you are not running any tests which need elasticsearch server. But you still need elasticsearch dependency

ankh6 · 2021-03-27T12:55:23Z

@lalitpagaria I'll install the elasticsearch server and create another PR.
Hope the previous PR will be reviewed in the meanwhile so that potential changes can be squeezed in one commit

lalitpagaria · 2021-03-27T18:12:34Z

@ankh6 Some test are failing in this PR. Otherwise looks fine to me.
Better to get review from @oryx1729, not sure whether it will conflict with this PR #922

ankh6 · 2021-03-30T19:46:08Z

@oryx1729 Have you had the time to review the PR ?
Tests are failing because Document class does not have validator:
est_rest_api.py:None (test_rest_api.py)
../venv/lib/python3.8/site-packages/fastapi/utils.py:65: in create_response_field
return response_field(field_info=field_info)
pydantic/fields.py:327: in pydantic.fields.ModelField.init
???
pydantic/fields.py:438: in pydantic.fields.ModelField.prepare
???
pydantic/fields.py:651: in pydantic.fields.ModelField.populate_validators
???
pydantic/validators.py:715: in find_validators
???
E RuntimeError: no validator found for <class 'haystack.schema.Document'>, see arbitrary_types_allowed in Config

oryx1729 · 2021-04-09T10:56:00Z

Hi @ankh6, thank you for working on this PR, and apologies for the delay in getting back on it.

This already looks in very good shape. It needs minor adjustments(mostly related to recently merged #922) and fixing the CI - I'm happy to take over from here to take it to the finish line in the next days!

ankh6 · 2021-04-10T13:01:49Z

@oryx1729 Sure thing!
I always look to better myself. Any advice/feedback on code implementation ?

…sers to interact with the document store through method requests DELETE, GET, POST, PUT

…ng document, updating write_documents method with a parameter

oryx1729 · 2021-05-03T10:13:18Z

Hi @ankh6, a quick update. I revisited the PR and realized it currently only has an implementation for the SQLDocumentStore. There is a bit more work to be done — extending it to other document stores, ensuring document embeddings(if present) gets updated when documents are updated/deleted. Please feel free if you'd like to contribute more to the PR, but in any case, we'll get back on it in the next few days 🙂

lalitpagaria · 2021-06-03T17:38:14Z

@ankh6 just checking if you able to work on it.

ZanSara · 2021-10-19T11:37:31Z

Getting and deleting documents through the REST API is now possible (#1546, #1580). Closing this. If anybody wants to continue working on the remaining features proposed here, let's open a new issue/PR and start fresh.

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

haystack/document_store/sql.py Show resolved Hide resolved

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

haystack/document_store/sql.py Show resolved Hide resolved

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

haystack/document_store/sql.py Outdated Show resolved Hide resolved

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

haystack/document_store/sql.py Show resolved Hide resolved

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

rest_api/controller/documents.py Outdated Show resolved Hide resolved

lalitpagaria reviewed Mar 8, 2021

View reviewed changes

Timoeller assigned oryx1729 Mar 17, 2021

Timoeller requested a review from oryx1729 March 19, 2021 12:26

oryx1729 changed the title ~~Feature/api list document~~ WIP: Feature/api list document Mar 24, 2021

ankh6 added 9 commits May 3, 2021 11:30

Added delete_document_by_id to interact with DELETE /documents/id

3ba2566

Added documents file into the router and formatting import statements

dd701cc

Added a file to interact with the document store. This file enables u…

f12458a

…sers to interact with the document store through method requests DELETE, GET, POST, PUT

Added abstract method to delete a document given its id

6aa2586

Modified delete_document_by_id by catching database api error, deleti…

715d77c

…ng document, updating write_documents method with a parameter

Added API to create a document, added better logic and error handling

90fc030

Added integration tests for api calls in documents module

1cfdcf6

Added update_exisiting_documents parameters to write_documents function

3e351c6

Added update_exisiting_documents parameters to write_documents function

5b50785

lalitpagaria mentioned this pull request Aug 30, 2021

Rest API to add (also delete) List[Document] to document store #797

Closed

This was referenced Oct 13, 2021

Allow document stores to delete documents by ID #1532

Closed

Delete documents by ID in all document stores #1606

Merged

ZanSara closed this Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Feature/api list document #880

WIP: Feature/api list document #880

ankh6 commented Mar 8, 2021

lalitpagaria Mar 8, 2021

lalitpagaria Mar 8, 2021

lalitpagaria Mar 8, 2021

lalitpagaria Mar 8, 2021 •

edited

Loading

lalitpagaria Mar 8, 2021 •

edited

Loading

lalitpagaria Mar 8, 2021

ankh6 Mar 9, 2021

lalitpagaria Mar 12, 2021

lalitpagaria Mar 8, 2021 •

edited

Loading

lalitpagaria Mar 8, 2021

lalitpagaria Mar 8, 2021

ankh6 Mar 9, 2021 •

edited

Loading

lalitpagaria Mar 12, 2021

lalitpagaria commented Mar 8, 2021

ankh6 commented Mar 9, 2021

Timoeller commented Mar 17, 2021

ankh6 commented Mar 17, 2021 •

edited

Loading

ankh6 commented Mar 17, 2021

Timoeller commented Mar 18, 2021

ankh6 commented Mar 18, 2021

Timoeller commented Mar 19, 2021

lalitpagaria commented Mar 19, 2021

ankh6 commented Mar 27, 2021

lalitpagaria commented Mar 27, 2021

ankh6 commented Mar 30, 2021

oryx1729 commented Apr 9, 2021 •

edited

Loading

ankh6 commented Apr 10, 2021

oryx1729 commented May 3, 2021

lalitpagaria commented Jun 3, 2021

ZanSara commented Oct 19, 2021

		raise Exception(f"Something went wrong while retrieving documents", e.message)


		@router.get("/documents/{id}", status_code=SUCCESS)

WIP: Feature/api list document #880

WIP: Feature/api list document #880

Conversation

ankh6 commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitpagaria Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

lalitpagaria Mar 8, 2021 • edited Loading

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

lalitpagaria Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankh6 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitpagaria commented Mar 8, 2021

ankh6 commented Mar 9, 2021

Timoeller commented Mar 17, 2021

ankh6 commented Mar 17, 2021 • edited Loading

ankh6 commented Mar 17, 2021

Timoeller commented Mar 18, 2021

ankh6 commented Mar 18, 2021

Timoeller commented Mar 19, 2021

lalitpagaria commented Mar 19, 2021

ankh6 commented Mar 27, 2021

lalitpagaria commented Mar 27, 2021

ankh6 commented Mar 30, 2021

oryx1729 commented Apr 9, 2021 • edited Loading

ankh6 commented Apr 10, 2021

oryx1729 commented May 3, 2021

lalitpagaria commented Jun 3, 2021

ZanSara commented Oct 19, 2021

lalitpagaria Mar 8, 2021 •

edited

Loading

lalitpagaria Mar 8, 2021 •

edited

Loading

lalitpagaria Mar 8, 2021 •

edited

Loading

ankh6 Mar 9, 2021 •

edited

Loading

ankh6 commented Mar 17, 2021 •

edited

Loading

oryx1729 commented Apr 9, 2021 •

edited

Loading