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

Update doc API #48

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update doc API #48

wants to merge 1 commit into from

Conversation

vporton
Copy link
Contributor

@vporton vporton commented Oct 10, 2023

API (PUT - is it a good choice?) for updating a doc.

Not tested. Please either test or explain me which part of the UI to use for testing.

@vporton
Copy link
Contributor Author

vporton commented Oct 10, 2023

Fixes #8.

@apssouza22
Copy link
Owner

API (PUT - is it a good choice?) for updating a doc.

Not tested. Please either test or explain me which part of the UI to use for testing.

Yes. PUT is fine

you can just use the Postman or curl to test it.

PUT http://localhost:8880/api/v1/docs/{pk}

If you don't want to bother with the token, just comment out the current_user: User = Depends(get_current_user) or take the token from the browser(console)

@vporton
Copy link
Contributor Author

vporton commented Oct 11, 2023

you can just use the Postman or curl to test it.

I am not sure how to test embeddings: Should I write a specialized testing code that check embeddings to be created/deleted as needed? Or is there an easier variant?

@apssouza22
Copy link
Owner

you can just use the Postman or curl to test it.

I am not sure how to test embeddings: Should I write a specialized testing code that check embeddings to be created/deleted as needed? Or is there an easier variant?

@vporton do you need to test embedding? You just need to be able to search for the new content through the /docs/search endpoint. That is the real test

@vporton
Copy link
Contributor Author

vporton commented Oct 12, 2023

I fixed some, but not all, bugs.

Please help:

The browser outputs

GET http://127.0.0.1:8880/api/v1/admin/user/docs
405 Method Not Allowed

I don't understand how this can happen: The string user/docs appears in the sources only twice, the first time is handling a POST request, and the second time is:

	{
		"item_id": 48123,
		"title": "Add doc",
		"article_type": "api",
		"application": "chat",
		"text": "To add a new documentation a given application you can use the following request. \n Params: - title \n - text \n- app_key \n Example: `curl -L  -X POST  -H \"Accept: application/json\"  -H \"Authorization: Bearer <YOUR-TOKEN>\"  http://localhost:8880/api/v1/admin/user/docs  -d '{\"title\":\"my doc title\", \"text\":\"my doc text\", \"app_key\":\"application key\"}'` \n\n Request render: ` request_render:{'text':{'field_type': 'textarea'}}` \n\n #action"
	},

It is a post request.

From where can a GET request appear? Please help me to understand which piece of the source produces this GET. @apssouza22

@vporton
Copy link
Contributor Author

vporton commented Oct 12, 2023

@apssouza22 Note that this query is triggered by the prompt:

Search app docs for the word "text"

@apssouza22
Copy link
Owner

@apssouza22 Note that this query is triggered by the prompt:

Search app docs for the word "text"

We don't have a doc for the doc search. Probably the engine is guessing that search docs uses get. So the get could be generated by the chatGPT.

It would be easier just use Postman.

Another option is just prompt in the UI "your new text" and then look at the network tab of the browser. The first request is to retrieve the doc based on the input. Just check if it is finding your edited doc

@apssouza22
Copy link
Owner

apssouza22 commented Oct 13, 2023

There is basic issues.

File "/Users/alexsandro.souza/projects/my/chatflow/server/src/api/docs.py", line 65, in update_doc
    return await update_app_doc(request, pk=request.pk)
AttributeError: 'AddDocRequest' object has no attribute 'pk'

To test. call http://127.0.0.1:8880/api/v1/admin/applications/chat/docs get the PK for the doc you want to edit

calll http://127.0.0.1:8880/api/v1/docs/{pk} with the following body

{
    "text": "test delete this content",
    "app_key": "chat",
    "title": "test- delete me"
}

Then call http://localhost:8880/api/v1/docs/search to search for the new text

{
    "text": "delete this content",
    "app_key":"chat"
}

Find attached the postman collection to all the endpoints
Chat-commander.postman_collection.json

Here how to get the access token

Screenshot 2023-10-13 at 23 06 34

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

@apssouza22 My advice to you:
Add the following collection-level pre-request script in Postman:

pm.sendRequest({
    url: "http://localhost:8880/api/v1/admin/user/login",
    method: 'POST',
    header: {
        'content-type': 'application/json'
    },
    body: {
        mode: 'raw',
        raw: JSON.stringify({email: "[email protected]", password: "123"})
    }
}, (err, res) => pm.collectionVariables.set("token", res.json().access_token));

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

Currently the output of /api/v1/admin/applications/chat/docs looks like this:

{
    "total": 11,
    "data": [
        {
            "pk": "01HCQMGBSGA8SE52EWNTNTDB5S",
            "item_id": 30039,
            "item_metadata": {
                "pk": "01HCQMGBSG41NATRTPZ2TPRAV2",

Note that item and its item_metadata have two different pk values. It is probably my error.

Before fixing it, I want to make sure that @apssouza22 agrees with me which of the two pks to leave intact and which to delete. I'd vote for the outer key to be left intact and the inner one to be deleted. @apssouza22 ?

@apssouza22
Copy link
Owner

@vporton I agree that the item_metadata pk can be deleted

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

@vporton I agree that the item_metadata pk can be deleted

This double pk seems to be a feature of redis-om-python, that possibly cannot be turned off.

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

calll http://127.0.0.1:8880/api/v1/docs/{pk} with the following body

There is no /docs/{pk} POST/PUT endpoint.

@apssouza22
Copy link
Owner

@vporton I agree that the item_metadata pk can be deleted

This double pk seems to be a feature of redis-om-python, that possibly cannot be turned off.

I meant that u can delete the whole object and create it again

@apssouza22
Copy link
Owner

calll http://127.0.0.1:8880/api/v1/docs/{pk} with the following body

There is no /docs/{pk} POST/PUT endpoint.

That's your new endpoint, right?

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

That's your new endpoint, right?

Oh, sorry, right. Apparently I was on a wrong Git branch :-(

@vporton
Copy link
Contributor Author

vporton commented Oct 14, 2023

After every query to /docs/{pk}, the number of results returned by /admin/applications/chat/docs decreases by one. Items are deleted instead of updated :-~

Do you @apssouza22 have any idea why this may happen?

@apssouza22
Copy link
Owner

After every query to /docs/{pk}, the number of results returned by /admin/applications/chat/docs decreases by one. Items are deleted instead of updated :-~

Do you @apssouza22 have any idea why this may happen?

@vporton I have simplified the doc structure. Please refer to the latest version. It should be much simpler to edit the docs now

@vporton
Copy link
Contributor Author

vporton commented Oct 15, 2023

Please refer to the latest version.

git pull downloads no updates.

@apssouza22
Copy link
Owner

Please refer to the latest version.

git pull downloads no updates.

Make sure u are getting from the right repository. You are probably pulling from your own repo.

You need to add a new repository. Run the following

git remote add apssouza myrepo

Then git pull apssouza

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

@vporton I have simplified the doc structure. Please refer to the latest version. It should be much simpler to edit the docs now

It still (I don't understand why) deletes a doc instead of updating it on the PUT request.

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

Sorry, I screwed up the update-doc branch, trying to resolve the conflicts.

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

Sorry, I screwed up the update-doc branch, trying to resolve the conflicts.

Or you @apssouza22 screwed it? How come that you deleted ItemMetadata which is used in _save_app_doc, etc.?

We need to communicate in words before further trying to edit it together.

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

I think, we should revert to my commit ed44edebef9d94300d1b2edace440cc33871a568 and start merging anew.

I apologize for my mistakes, I don't have much experience in merging teamwork.

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

@apssouza22 I can't work on this until coordinating with you: revert to my commit ed44edebef9d94300d1b2edace440cc33871a568? or if not, then what will we do instead?

@apssouza22
Copy link
Owner

Just delete and create another one as part of update. It should be very easy since we have those operations already

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

delete and create another one

Delete and create another what? (branch?)

Maybe, better to use git revert?

@apssouza22
Copy link
Owner

apssouza22 commented Oct 16, 2023 via email

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

Docs update. The thing u were trying to do here

Sorry, I am sometimes a little dislexic. I don't understand what you refer to by "docs update" and "the thing".

Is it OK for you if I do git revert?

@apssouza22
Copy link
Owner

apssouza22 commented Oct 16, 2023 via email

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

OK, it seems I now understand what happened: I did some changes and after this you applied a patch (3fe1b6a4a83b46cd504970ce0d0d4f95832aede9.) that did not took into account my changes. The result was screwed up code that confused me, and I even more incorrectly did merging after this.

So, to put this in order, I did on my copy of repository git reset --hard to remove main branch changes and reverted the above mentioned patch.

After this I removed @r.post("/docs/metadata" and @app.on_event("startup"), because, it is apparently a right intention to removed them.

Now I am starting testing anew. Please, don't screw it again like that patch.

@apssouza22
Copy link
Owner

That is real life flow. We changed the same files and it will cause conflict. U need to pull the latest version, fix the conflicts and push your version. There was no patch. There were new commits to the repo, as it has to be.

If u are not able to fix the conflicts, I suggest starting with a new Branch

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

Oh, I did something wrong and my git reset --hard deleted all my changes :-( I should run reset --hard as a last resort. git merge in some reason not worked, so I did it.

Not that bad, I will re-create the branch from main and write @put code again.

@vporton
Copy link
Contributor Author

vporton commented Oct 16, 2023

This time it was easy. The @put method successfully added.

But note that despite of your comment hset returns int if the key already exists, it returns an awaitable that returns int (not just int). So, probably fix your comment, it may be misleading.

@vporton vporton reopened this Oct 16, 2023
@apssouza22
Copy link
Owner

@vporton I will test this and if it works fine I will merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants