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

feat: add fasthtml demo-app #1446

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Conversation

thomasht86
Copy link
Contributor

@thomasht86 thomasht86 commented Aug 22, 2024

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Adds a sample app with a fastHTML-frontend.
All details should be in the README.

Deployed to https://huggingface.co/spaces/vespa-engine/fasthtml-vespa
Slack-msg if you need access. Intention is to make it public after merge.

The real test would be if someone else tried to deploy by following the README.

@lperry25 / @ldalves : Feel free to suggest some UI changes, if you see something that could be improved without adding too much complexity.
cc: @kkraune @jobergum

@thomasht86 thomasht86 requested review from lperry25 and ldalves August 22, 2024 12:45
@thomasht86
Copy link
Contributor Author

Guess the space needs to be publicly available for the link check to pass, so did that now.

@jobergum
Copy link

Great!

Minor comments:

  • Provide search examples to understand what the system is capable of
  • Make hitting Enter fire the query
  • It would be great to expose some settings like rank profile etc to illustrate that this is not a one default search/ranking to rule them all.

Copy link

@lperry25 lperry25 left a comment

Choose a reason for hiding this comment

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

I think it looks mostly great. Just one small UI feedback point :)

Article(
Header(
H2(
A(

Choose a reason for hiding this comment

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

I find the UI a bit weird that this is displayed as a Link but it doesn't take you anywhere. Is the idea that a full application would implement this link?
Maybe you can just set it as H2 to not be so confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it should open the document when clicked.
It does not show the actual links in browser though, as it is running in hf.

Choose a reason for hiding this comment

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

Oh I see now I am getting an internal server error when I try to access the links so nothing happens in the UI. This link gives a 500. https://vespa-engine-fasthtml-vespa.hf.space/document/MED-2434

Am I just missing permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, should not require permissions, but I got the same from my mobile, when not logged in to hf. Will investigate!

@thomasht86
Copy link
Contributor Author

Added sample queries and radio button to select rank profile.

@jobergum
Copy link

Sorry for being picky about this PR :)

image

  • When I pick sample query, it fires the query, but the form text input stays the same
  • When a click radio button nothing happens, it seems odd to press search (for me)

@thomasht86
Copy link
Contributor Author

If you're picky it means you care, so no worries :)

  • Didn't think about sample query updating form input. Will fix!
  • About radio button, you think it should fire query on radio button selection? Isn't it more natural to hit Enter or "Search" after selection? Can see if it is simple to do.

@ldalves
Copy link
Member

ldalves commented Aug 23, 2024

For a demo, I think the overall design is good. :)

Fixing all the points above, I think it would be nice to also focus the user on the search field when the page loads.

@thomasht86
Copy link
Contributor Author

Fixed filling of sample queries.
Did you also check out the "admin" login to display the query logs? (admin, admin)
Could be expanded to include results, clicks and maybe LLM-judgments for easy eval bootstrapping.

@kkraune
Copy link
Member

kkraune commented Sep 17, 2024

should we just merge and interate on this?

@thomasht86
Copy link
Contributor Author

I paused the live huggingface-demo, so I can remove the link pointing to that in the README first at least.
The next iteration could be replacing cord19 UI, but not sure when I will get around to that.

@thomasht86
Copy link
Contributor Author

Removed the link now. If noone else objects, I think we can merge.

@thomasht86 thomasht86 merged commit abcbf0f into master Sep 23, 2024
2 of 3 checks passed
@thomasht86 thomasht86 deleted the thomasht86/add-fasthtml-demo-app branch September 23, 2024 12:47
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.

5 participants