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

ai chat: fix summary prompt for pages is being translated #21063

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

petemill
Copy link
Member

@petemill petemill commented Nov 21, 2023

(whereas we're not translating the summary prompt for videos, or any other system prompt)

I think this must have regressed at some point. It was at least planned for.

This does also change the summarisation prompt to suggest bullet points, like we do with video summarisation requests. We can see if there's negative or positive feedback on that.

Resolves brave/brave-browser#34624

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:

@petemill petemill self-assigned this Nov 21, 2023
@petemill petemill requested a review from a team as a code owner November 21, 2023 01:18
Copy link
Contributor

@nullhook nullhook left a comment

Choose a reason for hiding this comment

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

lgtm ++

@@ -44,7 +44,8 @@ Here is the conversational history (between the user and you) prior to the quest
<message name="IDS_AI_CHAT_CLAUDE_QUESTION_PROMPT_SEGMENT" translateable="false">
Propose up to 3 very short questions that a reader may ask about the content. Consider intriguing or unusual elements of the content, or structurally important. Separate each suggested question with the character "|". End the question list with a &lt;/response&gt; tag.</message>

<message name="IDS_AI_CHAT_QUESTION_SUMMARIZE_VIDEO_BULLETS" translateable="false">Provide a concise list of up to 6 bullets of the most important points of the video.</message>
<message name="IDS_AI_CHAT_QUESTION_SUMMARIZE_PAGE" translateable="false">Provide a concise list of up to 6 bullets on the most important points of the page.</message>
Copy link
Member

@LorenzoMinto LorenzoMinto Nov 21, 2023

Choose a reason for hiding this comment

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

Is this meant to make the summaries' formats more consistent? Before we were just using "Summarise the page", correct?

One possible suggestion is that we currently treat the page as an article (<article> tags), so referring to it as an article might help. We could also switch that article tag with a page tag since our goal seems to be summarizing/answering on more general types of content, and switch all the language to "page" to be consistent

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes I was fixing this issue where we're translating this part of the prompt, but then was wondering why we would differ so much from the video summary request. But perhaps I should just fix this issue whereby we don't translate, but keep it as "Summarize this page"?

Unless we think there's definitely an improvement to changing <article to <page, and/or using this more descriptive request for summary?

Now I'm wondering - was there actually value to keeping this part of the prompt translated to the user's UI language? This is the part of the prompt which pretends to be the user, so maybe we should translate those parts? Not sure if that's reliable. We also don't translate the generated suggested questions. And if they're not in a nicely supported language then it could have weird results. Do you agree that we should stop translating this part of the prompt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed to <page everywhere, and it seems to be pleasant results in both llama and claude

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

strings++

…mmary prompt for videos, or any other system prompt)
@petemill petemill force-pushed the ai-chat-summary-prompt-force-english branch from 29ae0ad to 0ef0d33 Compare November 29, 2023 16:40
Copy link
Member

@LorenzoMinto LorenzoMinto left a comment

Choose a reason for hiding this comment

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

prompts++

@petemill petemill merged commit 6d93c11 into master Nov 29, 2023
17 checks passed
@petemill petemill deleted the ai-chat-summary-prompt-force-english branch November 29, 2023 18:34
@github-actions github-actions bot added this to the 1.63.x - Nightly milestone Nov 29, 2023
nullhook pushed a commit that referenced this pull request Dec 8, 2023
…lish

ai chat: fix summary prompt for pages is being translated
kjozwiak pushed a commit that referenced this pull request Dec 9, 2023
…1314)

* Merge pull request #21063 from brave/ai-chat-summary-prompt-force-english

ai chat: fix summary prompt for pages is being translated

* AIChat: new welcome opt-in ux and contextual toggle (#20966)

* aichat: new opt-in ux

* simplify siteInfo

* aichat: toggle page context responses from LLM

* moved build site info to conversation driver

* special summarize button visibility depends on toggle

* MakeAPIRequestWithConversationHistoryUpdate accepts a flag

* show model intro on all pages

* AI Chat: Don't show suggested questions when conversation isn't associated with content

* AI Chat: conversation entry should also be pending if content isn't ready yet

---------

Co-authored-by: Pete Miller <[email protected]>

---------

Co-authored-by: Pete Miller <[email protected]>
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 summary prompts are being translated
4 participants