AI-powered marking #1248
AI-powered marking #1248
Conversation
There was a problem hiding this comment.
Thanks for this feature! Quite a bit of comments. Please look through, resolve them. I do have some clarification comments as well so please answer those as well.
One question I have is when are these comments used? I couldn't find anytime in which the comments are returned/retrieved to/by the FE.
| Retrieves an AI comment for a specific submission and question. | ||
| Returns `nil` if no comment exists. | ||
| """ | ||
| def get_ai_comments_for_submission(submission_id, question_id) do |
There was a problem hiding this comment.
Naming implies you are getting all AI comments. Also what is the use case for getting only one of the comments?
There was a problem hiding this comment.
It appears this function is not used either
There was a problem hiding this comment.
I think we can remove this function then.
There was a problem hiding this comment.
Will keep this for eventual AI comments retrieval when loading a submission
…add-AI-generated-comments-grading
|
@sentry review |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an AI-powered marking feature that leverages LLMs to automate feedback generation for code submissions in Source Academy, reducing grading workload for teaching assistants.
Key Changes:
- Added AI comment generation infrastructure with OpenAI API integration and AES-GCM encrypted API key storage at course level
- Created database schema and logging system for AI-generated comments with new endpoints for generating, saving, and managing AI feedback
- Extended existing models with LLM-related fields including course-level, assessment-level, and question-level prompts
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/cadet_web/controllers/generate_ai_comments.ex | New controller implementing AI comment generation, LLM API interaction, and comment persistence |
| lib/cadet/ai_comments.ex | New module handling CRUD operations for AI comments |
| lib/cadet/ai_comments/ai_comment.ex | Schema definition for ai_comment_logs table |
| lib/cadet/courses/course.ex | Added LLM configuration fields and AES-GCM encryption logic for API keys |
| lib/cadet/assessments/assessments.ex | Extended get_answers_in_submission to support AI comments and added assessment prompt retrieval |
| priv/repo/migrations/* | Database migrations for LLM features and ai_comment_logs table |
| test/cadet_web/controllers/ai_code_analysis_controller_test.exs | Test coverage for AI comment generation endpoints |
| config/test.exs | Added encryption key configuration for testing |
| lib/cadet_web/router.ex | Registered new AI comment endpoints |
| lib/cadet_web/admin_views/admin_grading_view.ex | Updated grading view to include AI comments |
| test/support/seeds.ex | Updated test fixtures with new fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
priv/repo/migrations/20240320000001_add_llm_api_key_to_courses.exs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ource-academy/backend into feat/add-AI-generated-comments-grading
|
@sentry review |
![sentry-io[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| prepend: "prepend", | ||
| solutionTemplate: "template", | ||
| postpend: "postpend", | ||
| llm_prompt: "llm_prompt", |
There was a problem hiding this comment.
Why do we need the LLM prompt returned to the FE?
There was a problem hiding this comment.
This is to allow the "Composed prompt" view in the grading page
| enableSourcecast: :enable_sourcecast, | ||
| enableStories: :enable_stories, | ||
| enableLlmGrading: :enable_llm_grading, | ||
| llmApiKey: :llm_api_key, |
There was a problem hiding this comment.
Why do we need these returned? Also, this is returning the encrypted value?
| create(index(:ai_comment_logs, [:submission_id])) | ||
| create(index(:ai_comment_logs, [:question_id])) |
There was a problem hiding this comment.
Are these indices strictly necessary? Since AI comments (+ logs nonetheless) are very often being written (and in massive volumes), adding an index will result in significantly degraded write performance. FK lookup is already really well-optimized on Postgres, I don't see a need for the index imo.
| def change do | ||
| create table(:ai_comment_logs) do | ||
| add(:submission_id, references(:submissions, on_delete: :delete_all), null: false) | ||
| add(:question_id, references(:questions, on_delete: :delete_all), null: false) |
There was a problem hiding this comment.
Why is it by question instead of by answer? (which has the submission + question)
| defp decrypt_llm_api_key(encrypted_key) do | ||
| case Application.get_env(:openai, :encryption_key) do | ||
| secret when is_binary(secret) and byte_size(secret) >= 16 -> | ||
| key = binary_part(secret, 0, min(32, byte_size(secret))) | ||
|
|
||
| case Base.decode64(encrypted_key) do | ||
| {:ok, decoded} -> | ||
| iv = binary_part(decoded, 0, 16) | ||
| tag = binary_part(decoded, 16, 16) | ||
| ciphertext = binary_part(decoded, 32, byte_size(decoded) - 32) | ||
|
|
||
| case :crypto.crypto_one_time_aead(:aes_gcm, key, iv, ciphertext, "", tag, false) do | ||
| plain_text when is_binary(plain_text) -> {:ok, plain_text} | ||
| _ -> {:decrypt_error, :decryption_failed} | ||
| end | ||
|
|
||
| _ -> | ||
| Logger.error( | ||
| "Failed to decode encrypted key, is it a valid AES-256 key of 16, 24 or 32 bytes?" | ||
| ) | ||
|
|
||
| {:decrypt_error, :decryption_failed} | ||
| end | ||
|
|
||
| _ -> | ||
| Logger.error("Encryption key not configured") | ||
| {:decrypt_error, :invalid_encryption_key} | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Should be abstracted away together with the encrypt function in a separate module for better abstraction/encapsulation
| ) | ||
|
|
||
| # Store both the IV, ciphertext and tag | ||
| encrypted = Base.encode64(iv <> tag <> ciphertext) |
There was a problem hiding this comment.
Nit: Please add a non-b64 character delimiter between the parts instead of just concatenating. That way we don't need to rely on offsets, and can just split using the delimiter (safer, more robust against variable-length payloads)
| # Get head of answers (should only be one answer for given submission | ||
| # and question since we filter to only 1 question) | ||
| case answers do | ||
| [] -> | ||
| conn | ||
| |> put_status(:not_found) | ||
| |> text("No answer found for the given submission and question_id") | ||
|
|
There was a problem hiding this comment.
We can just pull the answer directly if we store the answer as FK right? Is there a reason we store separate FKs instead?
| {:decrypt_error, err} -> | ||
| conn | ||
| |> put_status(:internal_server_error) | ||
| |> text("Failed to decrypt LLM API key: #{inspect(err)}") |
There was a problem hiding this comment.
We really shouldn't be returning a stack trace in HTTP responses, especially on a security critical thing like decryption
| **Additional Instructions for this Question:** | ||
| #{answer.question.question["llm_prompt"] || "N/A"} | ||
| **Question:** | ||
| ``` | ||
| #{answer.question.question["content"] || "N/A"} | ||
| ``` | ||
| **Model Solution:** | ||
| ``` | ||
| #{answer.question.question["solution"] || "N/A"} | ||
| ``` | ||
| **Autograding Status:** #{answer.autograding_status || "N/A"} | ||
| **Autograding Results:** #{format_autograding_results(answer.autograding_results)} | ||
| The student answer will be given below as part of the User Prompt. |
There was a problem hiding this comment.
Beware of the unstripped leading indent
| ] | ||
|
|
||
| case call_llm_endpoint(llm_api_url, input, headers) do | ||
| {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> |
There was a problem hiding this comment.
Can we alias it for readabilty?
| HTTPoison.post(llm_api_url, input, headers, | ||
| timeout: 60_000, | ||
| recv_timeout: 60_000 | ||
| ) | ||
| end |
There was a problem hiding this comment.
Why are we using manual POST/etc instead of OpenAI client library like chat_controller.ex? Is it to make it provider agnostic? If it is, then it doesn't really make sense since the downstream response parsing expects an OpenAI-compatible provider.
But if it's already an OpenAI-compatible provider, then just set the base URL in the OpenAI client library?
Description
Created a new feature to automate feedback generation of code submissions in Source Academy, with the help of LLMs. This will help save TAs some time in grading code submissions from students!
generate_ai_commentsendpoint to fetch question details and generate AI-generated comments for submissions.save_chosen_commentsendpoint to save multiple chosen comments for a submission and question for logging purposes.save_final_commentendpoint to save the final comment chosen for a submission for logging purposes.ai_comment_logstable to log various data points from inputs, original student's code, outputs generated by LLM, comments chosen, and final comment.AICommentsmodule to handle creation, retrieval, and updates for AI comments, including saving final and chosen comments.AICodeAnalysisControllerto handle AI comment generation, saving final comments, and saving chosen comments.generate_ai_comments,save_final_comment, andsave_chosen_commentsendpoints inAICodeAnalysisControllerTest.Note: This may require changes to the DB diagram in README.md.
Type of Change
Checklist