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

Fix ParseSSE function causing Leo to get cut off #21018

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Conversation

nvonpentz
Copy link
Contributor

@nvonpentz nvonpentz commented Nov 16, 2023

Resolves brave/brave-browser#34321

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This one is hard to reproduce without setting up the AI chat server. For now, I think it is sufficient to just make sure that you can send a chat (with any model) and you do get a response, and it's not cut off.

@nvonpentz nvonpentz enabled auto-merge (squash) November 17, 2023 19:59
@nvonpentz nvonpentz merged commit 935d1a4 into master Nov 17, 2023
@nvonpentz nvonpentz deleted the fix-parse-sse branch November 17, 2023 21:56
@github-actions github-actions bot added this to the 1.62.x - Nightly milestone Nov 17, 2023
brave-builds added a commit that referenced this pull request Nov 17, 2023
brave-builds added a commit that referenced this pull request Nov 17, 2023
@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.62.67 Chromium: 119.0.6045.163 (Official Build) nightly (64-bit)
-- | --
Revision | 223789bf687abf95feb51f6c9fb2acdfd8c3a359
OS | Windows 11 Version 22H2 (Build 22621.2715)

Basically used the STR/guidance from #21018 (comment) and went through the following:

  • ensured that you get a Premium prompt/modal within the sidebar panel when attempting to pick a premium model
  • ensured that you can run through the signup flow and retrieve the needed tokens for users with a subscription
Example Example
Leo2 Leo3

llama-2-13b-chat

  • ensured that you can ask the llama a question and get an answer that isn't cut off
  • ensured that you can summarize a page/ask follow up questions without the answers being cut off
Example Example Example Example Example
image image image image image

llama-2-70b-chat

  • ensured that you can ask the llama a question and get an answer that isn't cut off
  • ensured that you can summarize a page/ask follow up questions without the answers being cut off
Example Example Example Example Example
image image image image image

claude-instant-v1

  • ensured that you can ask the claude a question and get an answer that isn't cut off
  • ensured that you can summarize a page/ask follow up questions without the answers being cut off
Example Example Example Example Example
image image image image image

@@ -453,7 +453,7 @@ void APIRequestHelper::URLLoaderHandler::ParseSSE(
};

DVLOG(2) << "Going to call ParseJson";
GetDataDecoder()->ParseJson(std::move(json.data()),
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm curious how this behaved/s before and after.

std::move(json.data()) is unusual, doesn't make sense and behaved sporadically odd.

for a moment, let's examine this: json.data() returns a const char*, the consumer of this pointer reads char up to the null terminator string.

how did this end up with sentence cut-offs?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Leo sometimes gives incomplete answers/stops responding
4 participants