Skip to content

Auth + async answer + loading indicator in placeholder#135

Merged
pmeier merged 20 commits intomainfrom
async_api_wrapper
Nov 1, 2023
Merged

Auth + async answer + loading indicator in placeholder#135
pmeier merged 20 commits intomainfrom
async_api_wrapper

Conversation

@pierrotsmnrd
Copy link
Contributor

@pierrotsmnrd pierrotsmnrd commented Oct 30, 2023 •
edited
Loading

Async answer for a better, non-blocking UX when asking a question.

Making it async revealed the default loading indicator of Panel's chat message, so I replaced it with one with Ragna's colors.

UPDATE : also contains Auth after a rebase, see PR #137

@pierrotsmnrd pierrotsmnrd requested a review from pmeier October 30, 2023 16:49
pmeier
pmeier reviewed Oct 30, 2023
View reviewed changes
@pmeier
Copy link
Member

pmeier commented Oct 31, 2023

@pierrotsmnrd I've pushed 094105c to get rid of all the sync API calls. Unfortunately, this PR seems to be mixed with #137 and I'm facing issues there. Meaning, I can't properly review this one.

@pierrotsmnrd
Copy link
Contributor Author

pierrotsmnrd commented Nov 1, 2023 •
edited
Loading

Thanks for that, that's the piece I have been looking for. I wish asyncio's docs were clearer.

One issue though : Now the RagnaAuthTokenExpiredException is not caught anymore using asyncio.ensure_future.
This prevents the broken UI you had described in PR137 after restarting the API (the reason is an invalid token).

The two PRs are not "mixed", the branch async_api_wrapper has been rebased on auth to reduce the conflicts on api_wrapper. I'm going to close PR #135 as this use supersedes it.

@pierrotsmnrd pierrotsmnrd mentioned this pull request Nov 1, 2023
Closed
@pierrotsmnrd pierrotsmnrd changed the title async answer + loading indicator in placeholder Auth + async answer + loading indicator in placeholder Nov 1, 2023
pierrotsmnrd and others added 3 commits November 1, 2023 06:18
@pierrotsmnrd
Merge branch 'async_api_wrapper' of https://github.com/Quansight/ragna …
1877a3c 
…into async_api_wrapper
@pierrotsmnrd
Squashed commit of the following:
3cd9a20 
commit 9b721fd
Author: Philip Meier <github.pmeier@posteo.de>
Date:   Wed Nov 1 14:08:20 2023 +0100

    allow API token secret to be set through env var (#141)

commit 17c227e
Author: Philip Meier <github.pmeier@posteo.de>
Date:   Wed Nov 1 10:13:08 2023 +0100

    dont create DB session as dependency (#138)
@pmeier
Merge branch 'main' into async_api_wrapper
acd3915
pmeier
pmeier reviewed Nov 1, 2023
View reviewed changes
pmeier
pmeier approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

One minor comment for simplification, but we can do that in a follow-up PR. Thanks Pierre!

ragna/_ui/api_wrapper.py
Comment on lines +85 to +86
async def get_components(self):
return (await self.client.get("/components")).raise_for_status().json()
Copy link
Member

@pmeier pmeier Nov 1, 2023

Choose a reason for hiding this comment

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

We can do that in a follow-up, but we shouldn't need this anymore, right? We could just set this as an instance variable in __init__ and access it from there. The components will never change at runtime.

@pmeier pmeier merged commit 728b88a into main Nov 1, 2023
@pmeier pmeier deleted the async_api_wrapper branch November 1, 2023 16:05
@pmeier pmeier mentioned this pull request Mar 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pmeier pmeier pmeier approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pierrotsmnrd @pmeier