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: fix parser #95

Merged
merged 21 commits into from
Oct 31, 2023
Merged

feat: fix parser #95

merged 21 commits into from
Oct 31, 2023

Conversation

anfibiacreativa
Copy link
Member

Purpose

Fix the broken parsing for the follow up questions in current version. This version does not decouple the components or introduces code optimizations.
It minimally varies color scheme and the rendering format of follow up questions (as shown in GIF)

Does this introduce a breaking change?

[ ] Yes
[ x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ x] Other... Please describe: cosmetic changes


How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code
    No test was added

What to Check

Follow up questions are properly rendered, as displayed in the GIF

followupquestions

Copy link
Contributor

@sinedied sinedied left a comment

Choose a reason for hiding this comment

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

Does it work when it's not streaming?
As I recall there is a separate path for non-streaming mode

@anfibiacreativa
Copy link
Member Author

Does it work when it's not streaming? As I recall there is a separate path for non-streaming mode

Yes! Uploading gif to teams since it's too large.

@@ -52,9 +52,10 @@ export async function parseStreamedMessages({
// followup questions are marked either with the word 'Next Questions:' or '<<text>>' or both at the same time
// these markers may be split across multiple chunks, so we need to buffer them!
// TODO: remove all this logic from the frontend and implement a solution on the backend or with TypeChat

// we start by creating a buffer when we match the first marker
const matchedFollowupQuestionMarker =
(!isFollowupQuestion && chunkValue.includes('Next')) || chunkValue.includes('<<');
Copy link
Contributor

Choose a reason for hiding this comment

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

btw should we still parse Next markers? Is the new model still using Next to format followup questions?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the updated model uses only << and >> to format followup questions, then I'd suggest changing the condition to:

const matchedFollowupQuestionMarker = !isFollowupQuestion && chunkValue.includes('<<');

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. @sinedied can you clarify?

} else if (chunkValue.includes('<<') && isFollowupQuestion) {
isFollowupQuestion = true;
continue;
// this updates the index, so we add each question to a different array entry
// to simplify styling
} else if (chunkValue.includes('?>') || chunkValue.includes('>')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we checking for ?> && > and not ?>> && >>?

Copy link
Member Author

@anfibiacreativa anfibiacreativa Oct 30, 2023

Choose a reason for hiding this comment

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

Because in all tests I ran, ?> and the last > were returned as part of 2 different chunks, and there was never a match.

@manekinekko
Copy link
Contributor

Love the comments, thank you!

anfibiacreativa and others added 8 commits October 30, 2023 10:28
* added initial test support for the playwright tests

* add snapshot and thought process test along with default network response

* playwright config update

* ux update

* test screenshot updates

* update tests

* no stream har files

* embed the body in the har files

* stream har file updated

* update the screenshot for response formatting

* add screenshots for firefox and webkit tests

* fix the testid attribute

* remove playwright workflow for now

* add the github action for running local tests and fix a broken test

* only upload artifacts on failure

* ensure playwright browsers are installed in devcontainer

* move the tests to a e2e

* update types

* keep playwright tests separate

* cleanup tests
* setup env for k6

* test: load tests to package.json

* test: add tests

* test: add load test

* test: granualize metrics

* test: have a standard workload

* test: load 50 users 1 request/sec/user

* test: log the response failures

* test: randomize the iteration interval

* test: adjust the load scenario

* test: doc update

* test: move globals to shared esconfig
@anfibiacreativa anfibiacreativa merged commit 42f29cd into main Oct 31, 2023
5 checks passed
@anfibiacreativa anfibiacreativa changed the title Feat/fix parser feat: fix parser Oct 31, 2023
anfibiacreativa added a commit that referenced this pull request Oct 31, 2023
* chore: update followup questions preliminary

* chore: update styles

* chore: restyle followup questions

* chore: fix parser for follow up questions (before TC)

* chore: fix bug and adapt styles

* chore: add margin to citations

* chore: remove commented regex

* fix: remove unnecessary regex and replace

* chore: update global config

* chore: add question icon

* chore: update styles

* chore: update comments to reflect decisions

* chore: refine cosmetic styles

* chore: replace icons with iconcloud set

* fix: fix minor bug with index replacement

* test: add E2E playwright tests for web app (#89)

* added initial test support for the playwright tests

* add snapshot and thought process test along with default network response

* playwright config update

* ux update

* test screenshot updates

* update tests

* no stream har files

* embed the body in the har files

* stream har file updated

* update the screenshot for response formatting

* add screenshots for firefox and webkit tests

* fix the testid attribute

* remove playwright workflow for now

* add the github action for running local tests and fix a broken test

* only upload artifacts on failure

* ensure playwright browsers are installed in devcontainer

* move the tests to a e2e

* update types

* keep playwright tests separate

* cleanup tests

* docs: update readme with auth info

* test: add load tests  (#93)

* setup env for k6

* test: load tests to package.json

* test: add tests

* test: add load test

* test: granualize metrics

* test: have a standard workload

* test: load 50 users 1 request/sec/user

* test: log the response failures

* test: randomize the iteration interval

* test: adjust the load scenario

* test: doc update

* test: move globals to shared esconfig

* test: add playwright test for ask interaction (#94)

* ci: disable playwright for demo

* ci: revert previous commit and disable from UI

---------

Co-authored-by: shibbas <26466942+shibbas@users.noreply.github.com>
@anfibiacreativa anfibiacreativa deleted the feat/fix-parser branch January 19, 2024 14:55
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.

4 participants