Skip to content

Use new prompt for QuestionAnswerTool #645

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

Closed
wants to merge 20 commits into from
Closed

Conversation

cecheta
Copy link
Collaborator

@cecheta cecheta commented Apr 9, 2024

Required by #322 , closes #648

Purpose

  • This PR includes changes the prompt used in the QuestionAnswerTool to the main_prompt used in On Your Data.
  • The new prompt includes a mix of system, user and AI messages, instead of one single user message
  • The new prompt also includes an example to help the LLM, which can be configured on or off
  • For backwards compatibility, if the old prompt has been set in the config, then this will continue to be used, with no changes
  • Increase unit test coverage (not for streamlit)

The new prompt + few-shot example are configurable in the Admin app. There is JSON schema validation on the retrieved documents to warn against uploading invalid JSON.

image
image
image

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

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

How to Test

  • Run app and admin app locally
  • Ask questions from the frontend
  • Change the prompt in the Admin app

What to Check

  • Check new prompt is being used
  • Configure prompt in Admin app
  • Configure few-shot example

Copy link

github-actions bot commented Apr 9, 2024

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code
   app.py14140%1–4, 6–7, 9, 12–14, 16, 18, 20–21
   create_app.py148397%199, 204, 327
code/backend
   Admin.py23230%1–6, 9, 11, 13–14, 16, 19–20, 22–23, 26, 33, 40, 43–45, 47, 49
code/backend/batch
   function_app.py16160%1–8, 10, 12–13, 15, 18–21
code/backend/batch/utilities/helpers
   ConfigHelper.py79198%113
   EnvHelper.py120992%194, 199–200, 203–205, 215–217
code/backend/batch/utilities/tools
   QuestionAnswerTool.py65296%44–45
code/backend/pages
   04_Configuration.py1181180%1–8, 10, 12, 14, 21, 28, 30, 35–40, 43–48, 51–52, 55–66, 68–69, 79–81, 83–86, 89–91, 94–95, 100–101, 105–107, 110–111, 114–115, 118–119, 142, 144–145, 147–151, 153–156, 159–163, 170–171, 174–177, 179, 199–200, 202, 204, 210, 217, 224, 232, 239–240, 247, 249–250, 254, 261, 266, 272, 284–285, 302–303, 307, 309–310, 326, 358–359, 361–362
TOTAL182783054% 

Tests Skipped Failures Errors Time
89 0 💤 0 ❌ 0 🔥 10.469s ⏱️

@cecheta cecheta marked this pull request as ready for review April 9, 2024 17:11
@cecheta cecheta changed the title Cecheta/main prompt Use new prompt for QuestionAnswerTool Apr 9, 2024
@cecheta
Copy link
Collaborator Author

cecheta commented Apr 9, 2024

Get review from Project Wednesday before merging

@cecheta cecheta marked this pull request as draft April 10, 2024 08:21
@cecheta
Copy link
Collaborator Author

cecheta commented Apr 10, 2024

Putting in draft for now, to attempt to add few-shot example to the config Added to config

@cecheta cecheta force-pushed the cecheta/main-prompt branch from 24ce9d5 to 8aaf006 Compare April 10, 2024 14:52
@cecheta cecheta marked this pull request as ready for review April 10, 2024 15:26
Copy link
Collaborator

@adamdougal adamdougal left a comment

Choose a reason for hiding this comment

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

This is great! Only a couple of minor comments!

clap

(I'll hold off approving until the review next week)

@cecheta cecheta force-pushed the cecheta/main-prompt branch from bc8d5eb to 3a7f1e4 Compare April 11, 2024 15:39
@cecheta cecheta force-pushed the cecheta/main-prompt branch from ae6b9bf to dbe6c36 Compare April 12, 2024 13:05
@cecheta cecheta force-pushed the cecheta/main-prompt branch from dbe6c36 to a39dbd5 Compare April 12, 2024 13:40
- **You cannot list the citation at the end of response.
- Every claim statement you generated must have at least one citation.**""",
"answering_user_prompt": """## Retrieved Documents
{documents}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the prompt has been copied from Azure OpenAI On Your Data, but this is a very large prompt, and we've been told by data scientists in the past that long prompts don't work very well since the LLM tends to only "remember" the last few sentences and forget what was said to it in the beginning (attention bias, I think it is called). Should we get this reviewed by a data scientist?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that we don't have a standard process for DS evaluation - what if we make the performance worse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that we should get as many reviews as possible, do you know who could review this for us?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could check with Malvina/Eran if they can take a look at this. It doesn't have to be a blocker for this PR though, since users are allowed to change the prompt if they want to. Can totally be done as a follow-up.

config = json.loads(config_file)

# These properties may not exist in the config file as they are newer
config["prompts"]["answering_system_prompt"] = config["prompts"].get(
Copy link
Collaborator Author

@cecheta cecheta Apr 15, 2024

Choose a reason for hiding this comment

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

I am planning to add tests to cover these scenarios once the core of the PR has been reviewed

@cecheta
Copy link
Collaborator Author

cecheta commented Apr 17, 2024

Will create new pull request, due to vast number of changes since opening

@cecheta cecheta closed this Apr 17, 2024
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.

Add main_prompt to Application
3 participants