[AI Tutor] CT-562: Add s3 system prompt #58486

ebeastlake · 2024-05-09T06:22:13Z

We want to keep our system prompt hidden from the user. I moved it to the cdo-ai/tutor bucket in S3. It now gets loaded server-side and appended to the conversation history before it gets sent to OpenAI.

So currently the flow is:

conversation history from client
 --> prepend system prompt on server
 --> system prompt & conversation history get sent to Open AI
 --> server returns most recent message
 --> client adds most recent message to history

☝️ We might want to modify this at some point, but since this is a last-minute change, I wanted to keep it as close to the original implementation as possible.

Other changes in this PR:

Level instructions need to go to the server so they can be added to the system prompt
There was an issue where 500 and other unexpected errors weren't failing gracefully in the UI; this PR changes that

Links

Jira ticket: https://codedotorg.atlassian.net/browse/CT-562

Testing story

Added unit tests for:

read_file_from_s3
prepend_system_prompt
add_content_to_system_prompt

I tested manually that the code compilation and validation cases were still working as expected.

And also that error states are being handled as expected (by blocking network requests from the console).

I also tested that the questionable answers in the AI Tutor Bug Bash doc are all resolved by the new system prompt, see comments here: https://docs.google.com/document/d/1PAT43dP4lhuhhecQIaeb2ZnsH8dzU-1UaDwA-N4QAC8/edit#heading=h.kgu6m7hz2xke

Deployment strategy

Follow-up work

I followed a file-up ticket to capture adding eyes tests for AI Tutor UI: https://codedotorg.atlassian.net/browse/CT-571

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…ingly

molly-moen

Couple questions, otherwise looks good

molly-moen · 2024-05-10T16:07:07Z

dashboard/app/controllers/openai_chat_controller.rb

+  end
+
+  private def read_file_from_s3(key_path)
+    if [:development, :test].include?(rack_env)


is this to make local testing easier? Why do we need this for test too?

Locally, this is so I can try out an updated system prompt without replacing the contents in production. (AI TA might have needed this in test for their test suite, but we don't.) I'm removing the :test env here for now.

nice catch and good call! the :test part looks like a mistake on our end because it would have no effect in automated testing and would potentially break running our tests locally.

molly-moen · 2024-05-10T16:07:32Z

dashboard/app/controllers/openai_chat_controller.rb

+        return File.read(local_path)
+      end
+    end
+    s3_client.get_object(bucket: S3_AI_BUCKET, key: key_path).body.read


will this cache the response? I'm guessing the system prompt won't change that much?

There's a thread relevant to this comment and the one above here: https://codedotorg.slack.com/archives/C051P2V2RN0/p1715274926003069?thread_ts=1715274404.871189&cid=C051P2V2RN0

I copied the dev/test logic from Teacher Tools. I originally had it cached and then disabled it because I was troubleshooting unrelated test failures that mentioned a cache, and Dave said it shouldn't be pilot-blocking. If it's okay with you, I'll reenable the caching, assuming it passes Drone. No strong feelings on the dev/test thing -- though it probably makes sense to disable it for test if we don't know the application :D

davidsbailey

great job on thorough test coverage!

davidsbailey · 2024-05-10T18:26:09Z

dashboard/app/controllers/openai_chat_controller.rb

+  end
+
+  private def read_file_from_s3(key_path)
+    if [:development, :test].include?(rack_env)


nice catch and good call! the :test part looks like a mistake on our end because it would have no effect in automated testing and would potentially break running our tests locally.

davidsbailey · 2024-05-10T18:28:27Z

dashboard/test/controllers/openai_chat_controller_test.rb

+  # Post request without a messages param returns a bad request
+  test_user_gets_response_for :chat_completion,
+  name: "no_messages_test",
+  user: :ai_tutor_access,
+  method: :post,
+  params: {},
+  response: :bad_request
+


ebeastlake · 2024-05-10T18:52:28Z

@molly-moen @davidsbailey Thoughts on this approach to caching? https://github.com/code-dot-org/code-dot-org/pull/58486/files/ff7859efb863474271ec287dfd9a66127a5b2a1a..6daa4f98764fed7b707937ceb5fe834ec799e2f2

I disabled it in dev because I feel like caching causes more problems than it solves locally, but the risk is this logic is only unit-tested before it hits test and prod. Should I be caching in the test environment? 🤔 Does it matter?

molly-moen

🎉

molly-moen · 2024-05-10T19:50:50Z

dashboard/app/controllers/openai_chat_controller.rb

@@ -72,14 +72,27 @@ def add_content_to_system_prompt(system_prompt, level_instructions, test_file_co
  end

  private def read_file_from_s3(key_path)
-    if [:development, :test].include?(rack_env)
+    cache_key = "s3_file_#{key_path}"


@ebeastlake I don't know much about caching besides "we should probably do it here". It makes sense to me to run it on prod and maybe test too? I don't see a downside to running caching on test.

davidsbailey

LGTM after addressing comments

davidsbailey · 2024-05-10T19:49:48Z

dashboard/app/controllers/openai_chat_controller.rb

@@ -72,14 +72,27 @@ def add_content_to_system_prompt(system_prompt, level_instructions, test_file_co
  end

  private def read_file_from_s3(key_path)
-    if [:development, :test].include?(rack_env)
+    cache_key = "s3_file_#{key_path}"


since we don't know how else the shared cache is being used, I would suggest including the bucket name in the cache key.

dashboard/app/controllers/openai_chat_controller.rb

ebeastlake added 9 commits May 8, 2024 11:35

bones of new route for system prompt

12d86bd

remove references to system prompt on client and refactor code accord…

dd4bb36

…ingly

pass level instructions to backend for use in system prompt

86a8916

revert unnecessary diff

fb9b9d2

remove caching logic

027a086

add some unit tests for new functionality

d4ad13b

update validation

282a959

Merge branch 'staging' into emily/ct-562/s3-system-prompt

9e72a7d

revert unnecessary diff

ff7859e

ebeastlake requested review from a team and davidsbailey May 10, 2024 04:21

molly-moen reviewed May 10, 2024

View reviewed changes

davidsbailey approved these changes May 10, 2024

View reviewed changes

ebeastlake added 3 commits May 10, 2024 11:34

implement caching for production

e369ee0

adjust caching

3a5c527

update cache freshness

6daa4f9

molly-moen approved these changes May 10, 2024

View reviewed changes

molly-moen reviewed May 10, 2024

View reviewed changes

davidsbailey approved these changes May 10, 2024

View reviewed changes

davidsbailey reviewed May 10, 2024

View reviewed changes

dashboard/app/controllers/openai_chat_controller.rb Outdated Show resolved Hide resolved

ebeastlake added 3 commits May 13, 2024 09:52

append bucket name to cache key

69f5927

update CDO_SHARED_CACHE reference and read/write on test and prod

f56574d

update unit tests

7a31387

ebeastlake merged commit 4d9875e into staging May 14, 2024
2 checks passed

ebeastlake deleted the emily/ct-562/s3-system-prompt branch May 14, 2024 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AI Tutor] CT-562: Add s3 system prompt #58486

[AI Tutor] CT-562: Add s3 system prompt #58486

ebeastlake commented May 9, 2024 •

edited

molly-moen left a comment

molly-moen May 10, 2024

ebeastlake May 10, 2024

davidsbailey May 10, 2024

molly-moen May 10, 2024

ebeastlake May 10, 2024

davidsbailey left a comment

davidsbailey May 10, 2024

davidsbailey May 10, 2024

ebeastlake commented May 10, 2024

molly-moen left a comment

molly-moen May 10, 2024

davidsbailey left a comment

davidsbailey May 10, 2024

[AI Tutor] CT-562: Add s3 system prompt #58486

[AI Tutor] CT-562: Add s3 system prompt #58486

Conversation

ebeastlake commented May 9, 2024 • edited

Links

Testing story

Deployment strategy

Follow-up work

PR Checklist:

molly-moen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebeastlake commented May 10, 2024

molly-moen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebeastlake commented May 9, 2024 •

edited