Add Python textbook vector RAG pipeline - #1362
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Vector RAG (Retrieval-Augmented Generation) pipeline for the chatbot, adding database schemas and migrations for storing textbook chunks and embeddings, a Python-based ingestion script, and integrating vector-based retrieval into the chat controller. The review feedback highlights critical improvements: resolving fallback paths dynamically at runtime instead of compile time to support production releases, optimizing the vector retriever by filtering similarity thresholds in Elixir rather than SQL to preserve index efficiency, and replacing deprecated Python datetime calls in the ingestion script.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Coverage is 86.349% — isha into master. No base build found for |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds a Python-focused vector RAG pipeline, including pgvector storage, textbook ingestion, embeddings, retrieval, language validation, prompt updates, course chatbot configuration, and chat endpoint integration. It also adds supporting tests, scripts, static notes, and operational documentation. ChangesRAG foundation and ingestion
Chat behavior and course configuration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
.github/workflows/ci.yml (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the pgvector service image.
pgvector/pgvector:pg18is a moving major tag, so CI may silently change PostgreSQL or extension patch versions. Pin a tested patch tag or immutable digest for reproducible migrations and tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 30, Update the pgvector service image reference in the CI workflow from the moving pg18 tag to a tested patch-level tag or immutable image digest, preserving the existing service configuration and ensuring migrations and tests use a reproducible image.lib/cadet/chatbot/textbook_ingestion.ex (3)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCredo: nested module could be aliased.
Four
Ecto.Adapters.SQL.*call sites (Lines 94, 322, 427, 431) trigger the same warning repeatedly.♻️ Proposed fix
alias Cadet.Chatbot.{Embeddings, VectorRag} alias Cadet.Repo + alias Ecto.Adapters.SQLthen replace each
Ecto.Adapters.SQL.query(...)/query!(...)call withSQL.query(...)/SQL.query!(...).Also applies to: 322-322, 427-427, 431-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/chatbot/textbook_ingestion.ex` at line 94, Alias the nested Ecto SQL module in the module definition, then update all four Ecto.Adapters.SQL call sites in the relevant ingestion functions to use the alias for both query and query! invocations.Source: Pipeline failures
362-393: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-chunk embedding calls could make deploys slow.
embed_chunks/2embeds every chunk one HTTP round trip at a time viaEnum.reduce_while. For a document with hundreds of chunks (the wiki dry-run reports 542 forsicpy.md), this runs serially duringdeployment/init.sh, blocking deploy completion for potentially minutes, compounded by per-chunk retry backoff on any transient failures.Consider bounded concurrency, e.g.
Task.async_stream(chunks, &embed_with_retry(&1.content, max_retries), max_concurrency: 8, ordered: true), to overlap network latency across chunks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/chatbot/textbook_ingestion.ex` around lines 362 - 393, Update embed_chunks/2 to embed chunks with bounded concurrency instead of serial Enum.reduce_while processing, using Task.async_stream with a suitable max_concurrency such as 8 and ordered results. Preserve embed_with_retry/3 retry behavior, maintain input chunk order, attach each returned embedding to its chunk, and propagate task or embedding errors as {:error, reason}.
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCredo: pipe chain starts with a function call.
Pipeline flags this as an error ("Pipe chain should start with a raw value").
♻️ Proposed fix
- defp checksum(text), do: :crypto.hash(:sha256, text) |> Base.encode16(case: :lower) + defp checksum(text), do: :sha256 |> :crypto.hash(text) |> Base.encode16(case: :lower)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/chatbot/textbook_ingestion.ex` at line 85, Update the private checksum/1 function so the SHA-256 hash result is passed directly to Base.encode16 without starting a pipe chain from the :crypto.hash/2 function call; preserve the existing lowercase hexadecimal output.Source: Pipeline failures
lib/cadet/release.ex (1)
17-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIngestion failure aborts the whole deploy script, not just ingestion.
raisehere propagates a non-zero exit to"$BASEDIR/bin/cadet" rpc Cadet.Release.ingest_sicpy_textbookindeployment/init.sh(Line 44), which runs underset -euxo pipefail. A transient failure (network fetch, OpenAI rate limit) would fail the entire deployment even though the service was already started and migrated successfully a few lines earlier.Confirm this fail-fast behavior is intentional (e.g., "chatbot must have textbook content before going live"). If not, consider logging and returning
:ok/non-raising on error so a flaky external fetch doesn't block unrelated deploys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/release.ex` around lines 17 - 32, Update Cadet.Release.ingest_sicpy_textbook so the {:error, reason} branch logs the ingestion failure and returns normally instead of raising, allowing deployment to continue after transient external failures; preserve the existing successful ingestion and already-ingested handling.lib/cadet_web/controllers/chat_controller.ex (2)
156-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApprove gating logic; consider log level for the disabled-course path.
The enable/disable gate itself is correct. Minor observability nit:
handle_chatbot_disabled/2logs atLogger.errorfor what is expected, routine access-control behavior (a disabled course), which can add noise to error-level alerting/dashboards compared toLogger.info/Logger.warn.Also applies to: 167-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/controllers/chat_controller.ex` around lines 156 - 165, Update handle_chatbot_disabled/2 to log expected disabled-course access at Logger.info or Logger.warn instead of Logger.error, while preserving the existing chatbot gating logic in ensure_chatbot_enabled/1.
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwagger docs are missing the new 403 response.
ensure_chatbot_enabled/handle_chatbot_disablednow return:forbiddenforchat/2, but the swagger spec still only documents 200/400/401/404/422/500.📝 Suggested doc addition
response(200, "OK") response(400, "Missing or invalid parameter(s)") response(401, "Unauthorized") + response(403, "Chatbot is not enabled for this course") response(404, "No conversation found for user") response(422, "Message exceeds the maximum allowed length") response(500, "When OpenAI API returns an error")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/controllers/chat_controller.ex` around lines 64 - 70, Update the Swagger response declarations for chat/2 in the controller to include a 403 Forbidden response, matching the :forbidden result returned by ensure_chatbot_enabled/handle_chatbot_disabled while preserving the existing documented responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/config.exs`:
- Around line 54-59: Update the VECTOR_RAG_MIN_SIMILARITY parsing case to use
Float.parse/1 instead of String.to_float/1, handling invalid or non-float values
explicitly with a clear configuration error or the established fallback while
preserving the nil default and empty-string behavior.
- Line 65: Update the VECTOR_RAG_EMBEDDING_MODEL configuration and the
associated ingestion/retrieval flows to ensure the selected embedding model
produces exactly 1536-dimensional vectors compatible with rag_chunks.embedding.
Restrict configuration to known 1536-dimensional models or validate the
generated vector dimension before insert and retrieval, rejecting incompatible
models or vectors before database operations.
In `@lib/cadet/chatbot/language_directory.ex`:
- Around line 28-33: Update sicpy_language?/2 and its sicpy_entry? validation so
only language IDs with corresponding semantic prompt profiles are accepted;
reject directory entries such as sicpy-custom when no profile exists.
Alternatively, add metadata-driven profile resolution and ensure PromptBuilder
receives the required sublanguage constraints. Add a regression test covering an
unsupported json_py/ entry.
In `@lib/cadet/chatbot/openai_embeddings.ex`:
- Around line 28-31: Update the HTTP request in Embeddings.embed to use a
substantially shorter receive timeout appropriate for synchronous embeddings,
and source that timeout from shared configuration alongside
VectorRag.embedding_model() and VectorRag.embedding_api_url() instead of
hardcoding 120_000. Keep the existing connection timeout and request flow
unchanged, and align the configuration with the shared OpenAI HTTP timeout
settings used by chat_controller.ex.
In `@lib/cadet/chatbot/textbook_ingestion.ex`:
- Around line 87-99: The ingestion flow in
lib/cadet/chatbot/textbook_ingestion.ex around existing_document/1 and the
corresponding flow in priv/rag/ingest_text.py lines 413-459 must handle
duplicate documents atomically. Wrap each SELECT, chunk construction, and
rag_documents/rag_chunks insertion sequence in one transaction, or remove the
pre-check and treat unique-constraint duplicate insert failures as already
ingested; ensure competing runs cannot leave duplicate rag_chunks.
In `@priv/chatbot_notes_py/sicpy_index_terms_chapter1.json`:
- Around line 2-4: Fix the source/parser that generates the index so extraction
artifacts are removed and distinct adjacent terms are not concatenated,
producing canonical keys such as !=, lambda, procedures, functions, lisp, and
python. Regenerate sicpy_index_terms_chapter1.json and add validation coverage
for the expected operator and term keys, including the affected entries.
In `@priv/rag/ingest_text.py`:
- Line 466: Update the zip call in the chunk ingestion loop to use strict=True,
ensuring mismatched chunk and embedding counts fail instead of silently
truncating. Preserve the existing chunk/embedding processing behavior when both
iterables have equal lengths.
In `@priv/rag/requirements.txt`:
- Around line 1-4: Define and enforce the minimum supported Python version for
the RAG dependencies listed in requirements.txt, using the project’s existing
configuration, CI, or lockfile mechanism. Ensure the version accommodates
langchain_openai and the other listed packages so incompatible environments fail
during resolution or installation rather than deployment.
In `@priv/repo/migrations/20260711000000_create_vector_rag_tables.exs`:
- Around line 24-33: Update the rag_chunks table definition so
rag_chunks.course_id is constrained to match its referenced
rag_documents.course_id, using a composite foreign key or equivalent database
constraint alongside the existing rag_document_id relationship. Preserve the
current cascade-delete behavior and retrieval columns while preventing
mismatched document/course pairs.
- Around line 39-42: Update the migration’s rag_chunks indexes to add a
supported pgvector ANN index on the embedding column used by VectorRetriever’s
nearest-neighbor ordering, choosing the project’s configured HNSW or IVFFlat
convention and compatible operator class. Keep the existing filtering indexes,
and verify the retrieval query plan with EXPLAIN to confirm the ANN index is
considered.
In `@tmp/pdfs/build_chat_backend_guide.py`:
- Around line 137-198: Remove the stale RAG architecture content from the
generated guide, including references to /v2/rag_chat, rag_chat_controller.ex,
rag_pipeline.ex, CourseDocuments, DocumentStore, and S3 retrieval. Align any
retained documentation with the PR’s actual /v2/chats/message flow using
PromptBuilder, VectorRetriever, and rag_chunks; if this tmp/pdfs build artifact
is not intended for source control, remove the file from the PR instead.
In `@wiki/vector-rag-textbook-ingestion.md`:
- Line 45: Replace the developer-specific /Users/isha/backend command in the
documentation with a portable working-directory instruction, such as the
repository’s documented relative path or a placeholder readers can substitute.
Ensure subsequent commands continue from that directory without assuming a
personal filesystem path.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 30: Update the pgvector service image reference in the CI workflow from
the moving pg18 tag to a tested patch-level tag or immutable image digest,
preserving the existing service configuration and ensuring migrations and tests
use a reproducible image.
In `@lib/cadet_web/controllers/chat_controller.ex`:
- Around line 156-165: Update handle_chatbot_disabled/2 to log expected
disabled-course access at Logger.info or Logger.warn instead of Logger.error,
while preserving the existing chatbot gating logic in ensure_chatbot_enabled/1.
- Around line 64-70: Update the Swagger response declarations for chat/2 in the
controller to include a 403 Forbidden response, matching the :forbidden result
returned by ensure_chatbot_enabled/handle_chatbot_disabled while preserving the
existing documented responses.
In `@lib/cadet/chatbot/textbook_ingestion.ex`:
- Line 94: Alias the nested Ecto SQL module in the module definition, then
update all four Ecto.Adapters.SQL call sites in the relevant ingestion functions
to use the alias for both query and query! invocations.
- Around line 362-393: Update embed_chunks/2 to embed chunks with bounded
concurrency instead of serial Enum.reduce_while processing, using
Task.async_stream with a suitable max_concurrency such as 8 and ordered results.
Preserve embed_with_retry/3 retry behavior, maintain input chunk order, attach
each returned embedding to its chunk, and propagate task or embedding errors as
{:error, reason}.
- Line 85: Update the private checksum/1 function so the SHA-256 hash result is
passed directly to Base.encode16 without starting a pipe chain from the
:crypto.hash/2 function call; preserve the existing lowercase hexadecimal
output.
In `@lib/cadet/release.ex`:
- Around line 17-32: Update Cadet.Release.ingest_sicpy_textbook so the {:error,
reason} branch logs the ingestion failure and returns normally instead of
raising, allowing deployment to continue after transient external failures;
preserve the existing successful ingestion and already-ingested handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7184d97c-3d77-46f9-bf78-17f2843b029f
⛔ Files ignored due to path filters (1)
tmp/pdfs/contact-sheet.pngis excluded by!**/*.png
📒 Files selected for processing (55)
.github/workflows/ci.yml.gitignoreconfig/config.exsconfig/test.exsdeployment/init.shlib/cadet/application.exlib/cadet/chatbot/conversation.exlib/cadet/chatbot/embeddings.exlib/cadet/chatbot/language_directory.exlib/cadet/chatbot/llm_conversations.exlib/cadet/chatbot/openai_embeddings.exlib/cadet/chatbot/prompt_builder.exlib/cadet/chatbot/rag_chunk.exlib/cadet/chatbot/rag_document.exlib/cadet/chatbot/sicp_notes_py.exlib/cadet/chatbot/textbook_ingestion.exlib/cadet/chatbot/vector_rag.exlib/cadet/chatbot/vector_retriever.exlib/cadet/courses/course.exlib/cadet/release.exlib/cadet_web/admin_controllers/admin_courses_controller.exlib/cadet_web/controllers/chat_controller.exlib/cadet_web/controllers/courses_controller.exlib/cadet_web/views/courses_view.exlib/cadet_web/views/user_view.expriv/chatbot_notes_py/README.mdpriv/chatbot_notes_py/sicpy_index_terms_chapter1.jsonpriv/chatbot_notes_py/sicpy_index_terms_chapter2.jsonpriv/chatbot_notes_py/sicpy_index_terms_chapter3.jsonpriv/chatbot_notes_py/sicpy_index_terms_chapter4.jsonpriv/chatbot_notes_py/sicpy_index_terms_chapter5.jsonpriv/chatbot_notes_py/sicpy_notes_chapter1.expriv/chatbot_notes_py/sicpy_notes_chapter2.expriv/chatbot_notes_py/sicpy_notes_chapter3.expriv/chatbot_notes_py/sicpy_notes_chapter4.expriv/chatbot_notes_py/sicpy_notes_chapter5.expriv/language_directory/README.mdpriv/language_directory/directory.jsonpriv/rag/ingest_text.pypriv/rag/requirements.txtpriv/repo/migrations/20260711000000_create_vector_rag_tables.exspriv/repo/migrations/20260714000000_add_language_id_to_llm_chats.exspriv/repo/migrations/20260720000000_add_louis_chatbot_config_to_courses.exstest/cadet/chatbot/embeddings_test.exstest/cadet/chatbot/language_directory_test.exstest/cadet/chatbot/llm_conversations_test.exstest/cadet/chatbot/prompt_builder_test.exstest/cadet/chatbot/sicp_notes_py_test.exstest/cadet/chatbot/vector_rag_test.exstest/cadet_web/admin_controllers/admin_courses_controller_test.exstest/cadet_web/controllers/chat_controller_test.exstest/cadet_web/controllers/courses_controller_test.exstest/cadet_web/views/chat_view_test.exstmp/pdfs/build_chat_backend_guide.pywiki/vector-rag-textbook-ingestion.md
4 participants
Summary
Validation