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

use bokeh_fastapi through panel #503

Merged
merged 17 commits into from
Dec 11, 2024
Merged

use bokeh_fastapi through panel #503

merged 17 commits into from
Dec 11, 2024

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented Aug 30, 2024

holoviz/panel#7205 adds support for bokeh_fastapi inside panel. This PR is here to test if everything works. cc @philippjfr

.github/actions/setup-env/action.yml Outdated Show resolved Hide resolved
Copy link
Member Author

@pmeier pmeier Aug 30, 2024

Choose a reason for hiding this comment

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

Changes in this file are drivebys of stuff missed in d869bd0.

Comment on lines +87 to +92
for dir in ["css", "imgs"]:
app.mount(
f"/{dir}",
StaticFiles(directory=str(Path(__file__).parent / "_ui" / dir)),
name=dir,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably go into BokehFastapi under a static_dirs keyword similar to what pn.serve does.

@pmeier
Copy link
Member Author

pmeier commented Aug 30, 2024

Re #503 (comment). The only difference I found is that the chat interface now seems to fill the whole vertical space of its container whereas before it only occupied the middle area.

panel==1.5.* / PR

image

panel==1.4.4 / main

image

@pmeier
Copy link
Member Author

pmeier commented Aug 30, 2024

I just realized that the document pill in the top-right corner is also missing for panel==1.5.*.

@pmeier
Copy link
Member Author

pmeier commented Sep 13, 2024

Document upload is also broken:

panel == 1.5

image

panel == 1.4.4

image

@pmeier
Copy link
Member Author

pmeier commented Sep 13, 2024

@pierrotsmnrd We can use the FileDropper widget going forward. But if the old one can easily be fixed, we can also do this in a follow-up PR.

@kcpevey
Copy link
Contributor

kcpevey commented Nov 26, 2024

@pmeier the document pills are missing because we never pass this check. metadata is not the list of dict keys. If you remove and "metadata" in self.current_chat, things start working again.

As for why the metadata field is missing...
It looks like the metadata field is set in LocalDocument, but I'm struggling to follow the connection between the UI interface for selecting a document, all the way down to the reader. It doesn't look like LocalDocument is used here, but I don't understand how the DocumentHandler works so maybe its not expected to?

@kcpevey
Copy link
Contributor

kcpevey commented Nov 26, 2024

For reference, the current document_upload widget is a FileInput. It used to be a custom widget, as is shown here - https://github.com/Quansight/ragna/blob/main/ragna/deploy/_ui/modal_configuration.py#L90-L94

Can I get some backstory on why it was removed? Should I just be adding in the new FileDropper widget from Panel and styling it to look similar to the custom one? Or should I be trying to make the old one work (looks like there has already been code changes in a separate PR to move beyond the custom one)?

@pmeier
Copy link
Member Author

pmeier commented Dec 3, 2024

@kcpevey re #503 (comment)

I broke that check in #441. And at that time I was well aware of it 🫠

Note that the change to the documents currently leaves the UI defunct.

Anyway, I pushed the necessary changes in 2d59794 to get the document pills back.

@pmeier
Copy link
Member Author

pmeier commented Dec 6, 2024

@kcpevey re #503 (comment)

We had a custom widget because we previously used a complex system to upload documents. In the first step we registered the document with the API and got back a location where to upload the document to. This was usually our server, but could also be an S3 bucket or the like.

While very flexible, that also brought a lot of complex handling. We removed that in favor of always uploading to local in #441. I've hot-patched the UI code in #443 to make it work again. The code is messy right now, but that is my problem not yours.

Should I just be adding in the new FileDropper widget from Panel

I've asked myself the same in #503 (comment). I guess this is a good move going forward, but I don't want to keep this PR up forever.

and styling it to look similar to the custom one?

My vote is out for trying to get the FileInput to look like what we had before. Hopefully that is no too complicated. If it turns out to not be simple or a lot of work that we might throw away when going for the FileDropper, we can reconsider.

@kcpevey
Copy link
Contributor

kcpevey commented Dec 10, 2024

Filedropper UI changes have been added, the widget now looks like this:
image

Copy link
Member Author

@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.

Thanks Kim for helping us out here. There are some CI failures remaining, but I'll fix them in follow-up PRs to avoid piling in to many things here!

@pmeier pmeier marked this pull request as ready for review December 11, 2024 09:18
@pmeier pmeier merged commit f41aa30 into deploy-dev Dec 11, 2024
6 of 11 checks passed
@pmeier pmeier deleted the deploy-dev-alpha branch December 11, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants