diff --git a/.claude/skills/daily-closeout/SKILL.md b/.claude/skills/daily-closeout/SKILL.md index cfc9591..fa87070 100644 --- a/.claude/skills/daily-closeout/SKILL.md +++ b/.claude/skills/daily-closeout/SKILL.md @@ -144,7 +144,7 @@ recortar, reformular). No escribís en el archivo hasta tener OK claro. 1. Insertá la entrada nueva en `docs/work_log.md` al final del archivo, respetando el formato de separadores. 2. Preparás el mensaje de commit. - - Si el repo tiene `.claude/skills/git-commits/SKILL.md`, seguí sus + - Si el repo tiene `.claude/skills/git-workflow/SKILL.md`, seguí sus convenciones estrictamente. - Si no la tiene, usá Conventional Commits: `docs(work-log): update for YYYY-MM-DD`. @@ -210,7 +210,7 @@ Esta skill funciona en cualquier repo con git y con al menos un 2. Verificá si el nuevo proyecto tiene `docs/work_log.md` — si no, la skill ofrecerá crearlo en el primer uso. 3. Opcional: si el nuevo proyecto tiene convenciones de commit distintas, - asegurate de tener también `.claude/skills/git-commits/SKILL.md` con esas + asegurate de tener también `.claude/skills/git-workflow/SKILL.md` con esas convenciones. Sin esa skill, `daily-closeout` cae al default Conventional Commits. diff --git a/.claude/skills/git-commits/SKILL.md b/.claude/skills/git-commits/SKILL.md deleted file mode 100644 index 476870a..0000000 --- a/.claude/skills/git-commits/SKILL.md +++ /dev/null @@ -1,70 +0,0 @@ ---- -name: git-commits -description: Convenciones de commits del proyecto (Conventional Commits en inglés, scopes válidos, cuerpo del mensaje, cadencia y agrupación de cambios). Usar SIEMPRE antes de redactar un mensaje de commit o de decidir cómo agrupar los cambios de una sesión en commits. ---- - -# Convenciones de commits - -Reglas permanentes (también en CLAUDE.md): commit por tarea lógica, 3–6 por -jornada, mensaje en inglés, y **nunca** commitear sin confirmación del usuario. - -## Formato del mensaje - -`(): ` — Conventional Commits, en inglés. - -**Types:** - -- `feat` — new feature or capability -- `fix` — bug fix -- `refactor` — code change that neither fixes a bug nor adds a feature -- `test` — adding or updating tests -- `docs` — documentation only -- `chore` — tooling, dependencies, CI, config -- `style` — formatting, whitespace (no logic change) -- `perf` — performance improvement - -**Scopes** (lowercase, una palabra, según capas y componentes del proyecto), como: -`domain`, `application`, `infrastructure`, `llm`, `retrieval`, `ocr`, -`privacy`, `estimator`, `api`, `agent`, `workflow`, `prompts`, `config`, -`deps`, `ci`, `tests`, `docs`, `mlops`, `notebooks`, `rag`. - -**Ejemplos:** - -``` -feat(llm): add streaming support to provider -fix(retrieval): handle empty search results -refactor(agent): switch from inheritance to composition -test(domain): add unit tests for Protocol implementations -docs(architecture): add ADR for prompt loading decision -chore(deps): upgrade pydantic to v2.9 -``` - -## Cuerpo del mensaje (opcional) - -Es completamente opcional y debe colocarse únicamente en el caso en que el mensaje -del commit no sea lo suficientemente claro para entender los cambios. Procura solo -emplear el mensaje del commit y el cuerpo solo úsalo cuando los cambios son realmente -grandes o tocan muchos componentes y/o archivos. - -Explica el **por qué**, no el qué — el diff ya muestra el qué. Línea en blanco -entre título y cuerpo. Si la decisión no es obvia, explícala: - -``` -refactor(agent): switch from inheritance to composition - -Base class was creating coupling between ResearchAgent and PQRSAgent -because streaming behavior differed. Composition via agent_utils.py -keeps each agent self-contained. -``` - -En commits que tocan superficie LLM, documentar riesgo OWASP-LLM y mitigación -en el cuerpo (tabla de riesgos en CLAUDE.md). - -## Cadencia y agrupación - -- Commit por tarea lógica, no por archivo: un provider y sus tests son UN - commit; el provider y un typo del README son DOS commits (feat + docs). -- Un commit debe poder revertirse sin romper otras cosas y tener un propósito - claro. -- 3–6 commits por jornada típica: menos = commits demasiado grandes para - revisar; más = micro-commits que ensucian el historial. diff --git a/.claude/skills/git-workflow/SKILL.md b/.claude/skills/git-workflow/SKILL.md new file mode 100644 index 0000000..f632b3f --- /dev/null +++ b/.claude/skills/git-workflow/SKILL.md @@ -0,0 +1,224 @@ +--- +name: git-workflow +description: Convenciones de git del proyecto — formato y cadencia de commits, scopes válidos, estrategia de ramas (GitHub Flow), duración máxima de ramas, pull requests y tags de versión. Usar SIEMPRE antes de redactar un mensaje de commit, decidir cómo agrupar los cambios de una sesión, crear o cerrar una rama, abrir un PR, o etiquetar el cierre de una versión del roadmap. +--- + +# Convenciones de git + +Fuente única de verdad para todo lo relacionado con git en este proyecto. +`CLAUDE.md` solo conserva dos reglas de comportamiento (nunca commitear sin +confirmación, mensajes en inglés) y apunta a este archivo para el resto. + +--- + +## Commits + +### Formato del mensaje + +`(): ` — Conventional Commits, en inglés. + +**Types:** + +- `feat` — new feature or capability +- `fix` — bug fix +- `refactor` — code change that neither fixes a bug nor adds a feature +- `test` — adding or updating tests +- `docs` — documentation only +- `chore` — tooling, dependencies, CI, config +- `style` — formatting, whitespace (no logic change) +- `perf` — performance improvement + +**Scopes** (lowercase, una palabra). Los de este proyecto, agrupados por +naturaleza: + +*Capas de la arquitectura:* `domain`, `application`, `infrastructure` + +*Componentes:* `llm`, `retrieval`, `memory`, `bot`, `api`, `agent`, +`services`, `prompts`, `ingestion` + +*Proyecto y tooling:* `config`, `deps`, `ci`, `tests`, `docs`, `scripts`, +`skills`, `system`, `roadmap`, `claude` + +Si un cambio no encaja en ningún scope existente, es señal de que el cambio +toca demasiadas cosas o de que falta un scope. Preferí dividir el commit antes +de inventar un scope nuevo. + +**Ejemplos reales de este repo:** + +``` +feat(bot): add Telegram bot adapter wired to vector-only RAG +feat(services): add hybrid search and its tests +refactor(services): split chunking from retrieval service +test(domain): add unit tests for Protocol implementations +docs(roadmap): mark T6-T10 complete, V1 done +docs(claude): document branching strategy and tag convention +chore(skills): add daily-closeout skill +chore(deps): upgrade pydantic to v2.9 +``` + +### Cuerpo del mensaje (opcional) + +Completamente opcional. Úsalo solo cuando el título no alcanza para entender +los cambios — típicamente cuando el cambio es grande, toca muchos archivos, o +la decisión detrás no es obvia. + +Explica el **por qué**, no el qué: el diff ya muestra el qué. Línea en blanco +entre título y cuerpo. + +``` +refactor(agent): switch from inheritance to composition + +Base class was creating coupling between ResearchAgent and PQRSAgent +because streaming behavior differed. Composition via agent_utils.py +keeps each agent self-contained. +``` + +Desde V4, los commits que toquen superficie expuesta al usuario (guardrails, +validación de input, PII masking, rate limiting) documentan en el cuerpo qué +riesgo de OWASP LLM Top 10 mitigan y cómo. + +### Cadencia y agrupación + +Commit por **tarea lógica**, no por archivo. + +- Un provider y sus tests son UN commit. +- Un provider y un typo del README son DOS commits (`feat` + `docs`). + +Criterio: un commit debe poder revertirse sin romper otras cosas y tener un +propósito único y claro. + +**3 a 6 commits por jornada típica.** Menos significa commits demasiado grandes +para revisar; más significa micro-commits que ensucian el historial. Con +sesiones de una hora, 1 o 2 commits por sesión es normal. + +**Nunca commitear sin confirmación explícita del usuario.** Al terminar una +tarea: "This task is complete. Suggested commit: ``. Shall +I proceed?" + +--- + +## Ramas + +### Estrategia: GitHub Flow + +- `main` siempre en estado desplegable. Desde V4, GitHub Actions despliega a + Cloud Run desde `main`. +- Las ramas de feature salen de `main` y vuelven vía pull request. +- **No hay `develop` ni `certification`.** Esas ramas existen en los proyectos + corporativos de Protección porque hay ambientes reales de staging y + certificación detrás. ResearchOS es un proyecto de un solo desarrollador sin + esos ambientes, así que las ramas extra son ceremonia sin beneficio. + +### Nomenclatura + +`feature/v{N}-{tema}` en minúsculas con guiones. + +Ejemplos: `feature/v2-langgraph-core`, `feature/v2-tools`, +`feature/v3-observability`, `feature/v4-guardrails`. + +Para trabajo no atado a una versión del roadmap: `fix/{tema}` o +`refactor/{tema}`. + +El nombre debe seguir siendo cierto al final de la rama. Si el alcance cambió +tanto que el nombre ya no describe el contenido, es señal de que la rama creció +más de lo que debía. + +### Duración máxima: dos semanas + +Si un entregable no cabe en dos semanas, se divide en varias ramas. + +Precedente que motiva la regla: V1 se desarrolló en una sola rama +(`feature/v1-infrastructure-setup`) que acumuló 56 commits entre abril y agosto +de 2026 sin mergear. Eso convirtió una rama de feature en una rama de larga +vida, dejó `main` congelada en el esqueleto inicial durante cuatro meses, y +volvió el merge final un evento grande y difícil de revisar. El nombre además +quedó desactualizado: terminó conteniendo el pipeline RAG completo, el sistema +de estudio y el bot de Telegram. + +Las ramas ahora corresponden a los bloques de dos semanas del roadmap. + +### Crear una rama nueva + +```bash +git checkout main +git pull origin main +git checkout -b feature/v2-langgraph-core +``` + +Siempre desde `main` actualizada. Nunca desde otra rama de feature. + +--- + +## Pull requests + +Cada rama cierra con un PR contra `main`, aunque haya un solo desarrollador. + +La razón no es revisión de código — es registro. La descripción del PR +documenta qué se construyó, qué decisiones arquitectónicas se tomaron, qué +deuda queda conocida y qué quedó fuera de alcance. Ese registro es material de +portafolio y alimenta el banco de preguntas de entrevista. + +### Estructura de la descripción + +```markdown +## Scope +{una o dos frases: qué cierra este PR} + +## What was built +{agrupado por capa o por componente, con rutas de archivo} + +## Key architectural decisions +{cada decisión con su justificación, no solo el qué} + +## Known debt +{cada ítem con la versión en que se piensa pagar} + +## Out of scope +{lo que deliberadamente no se hizo} +``` + +### Merge + +**Merge commit, no squash.** Los commits individuales son el historial de +aprendizaje del proyecto; aplastarlos destruye la trazabilidad de la que se +alimenta el banco de preguntas. + +Después del merge, borrar la rama en remoto y local: + +```bash +git branch -d feature/v2-langgraph-core +git push origin --delete feature/v2-langgraph-core +``` + +--- + +## Tags + +Cada versión del roadmap cierra con un tag anotado sobre `main`: + +```bash +git checkout main +git pull origin main +git tag -a v2.0.0 -m "V2: LangGraph agent + morning briefing" +git push origin v2.0.0 +``` + +Los números siguen el roadmap (`v1.0.0` … `v5.0.0`), no versionado semántico +de un paquete publicado. No hay tags intermedios: una rama de dos semanas que +se mergea no genera tag; solo el cierre de versión. + +--- + +## Anti-patrones + +- **Push tardío.** Cerrar tarea → commit → **push** → recién entonces reportar + que está terminado. Ocurrió dos veces en este proyecto que se reportó trabajo + completo que no estaba en el remoto. Trabajo que no está en el remoto no + existe para efectos de revisión. +- **Ramas que sobreviven su nombre.** Si al final de la rama el nombre ya no + describe el contenido, la rama creció demasiado. +- **Squash en cierres de versión.** Destruye el historial de aprendizaje. +- **Commits de "wip" o "cambios varios".** Si no puedes nombrar el propósito + del commit, el commit agrupa cosas que no van juntas. +- **Ramas salidas de otras ramas de feature.** Genera dependencias de merge + innecesarias. Siempre desde `main`. diff --git a/CLAUDE.md b/CLAUDE.md index 2343be0..281d06c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -140,60 +140,15 @@ When the user requests a session summary (or uses similar commands like "Generat ## Git Workflow -### Commit cadence -- Claude Code should suggest a commit after each complete logical task, - not after each modified file. -- A "logical task" is: an implemented feature, a passing test, a fixed bug, - a finished refactor, a completed docs section. -- When finishing a task, Claude should say: "This task is complete. - Suggested commit: ``. Shall I proceed?" -- Claude NEVER commits automatically without user confirmation. - -### Commit message format -Use Conventional Commits. Messages must be written in English. - - **Format:** `(): ` - - **Types:** - - `feat` — new feature or capability - - `fix` — bug fix - - `refactor` — code change that neither fixes a bug nor adds a feature - - `test` — adding or updating tests - - `docs` — documentation only - - `chore` — tooling, dependencies, CI, config - - `style` — formatting, whitespace (no logic change) - - `perf` — performance improvement - - **Scopes** match the project's architectural layers and components. - Use lowercase, one word. Common scopes for agent projects: - `domain`, `application`, `infrastructure`, `llm`, `retrieval`, `memory`, - `api`, `agent`, `prompts`, `config`, `deps`, `ci`, `tests`, `docs`. - - **Examples:** - feat(llm): add streaming support to provider - fix(retrieval): handle empty search results - refactor(agent): switch from inheritance to composition - test(domain): add unit tests for Protocol implementations - docs(architecture): add ADR for prompt loading decision - chore(deps): upgrade pydantic to v2.9 - -### Commit body (optional) -Use the body to explain **why**, not **what**. The diff already shows the what. -Leave a blank line between the title and the body. Example: - - ``` - refactor(agent): switch from inheritance to composition - - Base class was creating coupling between ResearchAgent and PQRSAgent - because streaming behavior differed. Composition via agent_utils.py - keeps each agent self-contained. - ``` - -#### Commit cadence -One commit per logical task, not per file. If you implement AnthropicLLM and its tests, that is ONE commit with both files, not two. If you implement the arXiv client and also fix a typo in the README, those are TWO separate commits (feat + docs). The rule: a commit should be revertable without breaking other things and must have a clear purpose. - -For a typical development session, aim for 3–6 commits per day. Fewer means commits are too large (hard to review); more means micro-commits that clutter the history. - -#### Two additional important rules -1. Commit messages are written in English even if the project code has comments in Spanish. This is industry standard convention and keeps the repo professional. -2. The commit body (optional, after the title) is used to explain the why, not the what. The diff already shows the what. If the decision is not obvious, explain it in the body. +Git conventions — commit format, cadence, branching strategy, pull requests +and tags — live in `.claude/skills/git-workflow/SKILL.md`. Consult that skill +before writing a commit message, creating a branch, or closing a roadmap +version. + +Two rules that always apply, regardless of the skill being loaded: + +1. **Never commit without explicit user confirmation.** When a task is + complete, say: "This task is complete. Suggested commit: + ``. Shall I proceed?" +2. Commit messages are written in English, even though code comments and + project docs may be in Spanish. diff --git a/docs/interview_prep/bank.md b/docs/interview_prep/bank.md index cbdefc1..7275535 100644 --- a/docs/interview_prep/bank.md +++ b/docs/interview_prep/bank.md @@ -15,6 +15,7 @@ proyecto. Meta: 80–100 preguntas al final de V5. - [Guardrails y seguridad (GR)](#guardrails-y-seguridad) — 0 preguntas - [LLM providers y SDKs (LM)](#llm-providers-y-sdks) — 0 preguntas - [Ingesta (IN)](#ingesta) — 0 preguntas +- [Proceso y colaboración (PC)](#proceso-y-colaboración) — 1 pregunta --- @@ -401,6 +402,41 @@ ResearchOS. --- +## Proceso y colaboración + +#### [PC-001] Nivel: intermedio +**Pregunta:** Trabajaste cuatro meses en una sola rama de feature sin +mergearla a `main`. ¿Qué problemas concretos genera eso y cuál es el criterio +para decidir cuánto debe vivir una rama? + +**Respuesta esperada:** Genera cuatro problemas. Primero, `main` no refleja +nada de lo que existe: si alguien clona el repo por defecto, obtiene un +esqueleto vacío. Segundo, el merge final es un evento grande y riesgoso — 56 +commits de una sola vez son imposibles de revisar con atención. Tercero, no +hay ningún punto en el historial que marque "acá terminó una versión", así que +no se puede hacer rollback a un estado conocido ni etiquetar hitos. Cuarto, en +equipo la rama divergiría de `main` y acumularía conflictos, aunque en +solitario ese riesgo no se materializa. El criterio de duración es el tamaño +del entregable, no el tamaño de la versión: una rama debe cerrar algo +mostrable y revertible en una o dos semanas. Si un entregable no cabe en dos +semanas, se divide. + +**Trampa común:** Justificar la rama larga con "estaba trabajando en una +versión completa". La versión es la unidad del roadmap, no la unidad de la +rama. V2 son siete semanas de roadmap y cuatro ramas de dos semanas cada una. +Otra trampa es proponer squash merge para "limpiar" el historial: en un +proyecto de aprendizaje los commits individuales son el registro del proceso. + +**Ejemplo en el proyecto:** `feature/v1-infrastructure-setup` acumuló 56 +commits entre abril y agosto de 2026 sin mergear, `main` permaneció congelada en `e3a6ac2` (el esqueleto inicial) hasta el merge del PR #1 el 10/08/2026 + +. El nombre además quedó desactualizado: la +rama se llamaba `infrastructure-setup` pero terminó conteniendo el pipeline +RAG completo, el sistema de estudio y el bot de Telegram. V2 se estructuró en +cuatro ramas de dos semanas para evitar repetirlo. + +--- + ## Testing _(sin preguntas todavía)_ diff --git a/docs/interview_prep/by_topic/proceso_colaboracion.md b/docs/interview_prep/by_topic/proceso_colaboracion.md new file mode 100644 index 0000000..9e9acfd --- /dev/null +++ b/docs/interview_prep/by_topic/proceso_colaboracion.md @@ -0,0 +1,33 @@ +# Proceso y colaboración — banco de preguntas + +1 pregunta. Fuente: `docs/interview_prep/bank.md`. + +#### [PC-001] Nivel: intermedio +**Pregunta:** Trabajaste cuatro meses en una sola rama de feature sin +mergearla a `main`. ¿Qué problemas concretos genera eso y cuál es el criterio +para decidir cuánto debe vivir una rama? + +**Respuesta esperada:** Genera cuatro problemas. Primero, `main` no refleja +nada de lo que existe: si alguien clona el repo por defecto, obtiene un +esqueleto vacío. Segundo, el merge final es un evento grande y riesgoso — 56 +commits de una sola vez son imposibles de revisar con atención. Tercero, no +hay ningún punto en el historial que marque "acá terminó una versión", así que +no se puede hacer rollback a un estado conocido ni etiquetar hitos. Cuarto, en +equipo la rama divergiría de `main` y acumularía conflictos, aunque en +solitario ese riesgo no se materializa. El criterio de duración es el tamaño +del entregable, no el tamaño de la versión: una rama debe cerrar algo +mostrable y revertible en una o dos semanas. Si un entregable no cabe en dos +semanas, se divide. + +**Trampa común:** Justificar la rama larga con "estaba trabajando en una +versión completa". La versión es la unidad del roadmap, no la unidad de la +rama. V2 son siete semanas de roadmap y cuatro ramas de dos semanas cada una. +Otra trampa es proponer squash merge para "limpiar" el historial: en un +proyecto de aprendizaje los commits individuales son el registro del proceso. + +**Ejemplo en el proyecto:** `feature/v1-infrastructure-setup` acumuló 56 +commits entre abril y agosto de 2026 sin mergear, con `main` congelada en +`e3a6ac2` (el esqueleto inicial). El nombre además quedó desactualizado: la +rama se llamaba `infrastructure-setup` pero terminó conteniendo el pipeline +RAG completo, el sistema de estudio y el bot de Telegram. V2 se estructuró en +cuatro ramas de dos semanas para evitar repetirlo. diff --git a/docs/learnings.md b/docs/learnings.md index bab8049..be4b375 100644 --- a/docs/learnings.md +++ b/docs/learnings.md @@ -302,3 +302,49 @@ Regla simple para ResearchOS: - Guardé el retorno de `await update.message.reply_text(...)` en una variable sin usar. --- + +**Fecha:** 11/08/2026 + +### ¿Qué aprendí? + +- **`RetrieveFn = Callable[[str], Awaitable[list[Document]]]` es el mismo patrón de ayer, una capa más abajo.** Ayer inyecté al bot una función que responde (`AnswerFn`); hoy inyecté al servicio una función que recupera. La simetría no es casual: cuando lo que necesito inyectar es *un solo comportamiento*, un tipo de función alcanza. Protocol o clase solo cuando son varios comportamientos que comparten estado. + +- **Por qué un parámetro tipo `strategy="hybrid"` habría sido peor.** La firma quedaría `answer_query(query, llm, store=None, retrievers=None, strategy="vector", top_k=5)`: dos parámetros opcionales que son *condicionalmente obligatorios* según el valor de un tercero. Puedo llamar `strategy="hybrid"` pasando solo `store` y compila perfecto — explota en runtime. El type checker no puede expresar "si strategy es hybrid, retrievers es obligatorio". Inyectar la función elimina el problema: la dependencia correcta ya está capturada en el closure. + +- **`retrieve_and_generate` es una composición de conveniencia, no el único camino.** Su paso 1 (`retrieve_context`) está soldado a búsqueda vectorial y exige un `store`. Cuando necesito otra estrategia no la parametrizo: armo mi propia composición con las mismas piezas primitivas — `retrieve(query)` → `build_rag_messages(...)` → `llm.generate(...)`. Por eso `build_rag_messages` está expuesta como función independiente y no escondida dentro de `retrieve_and_generate`. + +- **Este es el pago concreto de composición sobre herencia** (la pregunta 1.4 del taller que dejé en blanco). Con herencia tendría un `BaseRagAgent` con un método `retrieve()` que habría que sobrescribir, y cambiar de estrategia significaría crear una subclase. Con composición elijo otra función para ese paso. El costo de cambiar la estrategia bajó de "nueva clase" a "otra línea". + +- **Por qué NO modifiqué `retrieve_context`.** Tres razones: su firma recibe `store: VectorStore` mientras hybrid necesita `retrievers: list[Retriever]` (dependencias distintas); `hybrid_search` ya existe en `retrieval_service.py` y duplicarla en `agent_utils` pondría la misma lógica en dos lugares; y `retrieve_context` sigue siendo útil tal como está — si mañana quiero volver a vectorial puro, mi closure sería `retrieve_context(q, chroma, K)`. La pieza no muere, solo se mueve de ser llamada dentro del service a ser llamada en el composition root. + +- **Las dependencias se construyen una vez, al arrancar, no dentro del closure.** Mi primer intento construía `LocalEmbedder()` dentro de la función de recuperación. `LocalEmbedder` carga un modelo de sentence-transformers en memoria: se habría recargado en cada pregunta que llegara al bot. Eso es literalmente lo que significa "composition root" — el lugar donde se arma, no donde se usa. + +- **El patrón de dispatch ya existía en mi propio repo.** El dict `strategies` de `scripts/eval_retrieval.py` (línea 88) tiene cuatro lambdas que son, cada una, un `RetrieveFn`. Escribí cuatro instancias del tipo antes de nombrarlo. Si mañana quiero elegir estrategia por configuración, muevo ese dict al script y leo `settings.retrieval_strategy` — sin cadena de `if`. + +- **Pasar una función vs. llamarla.** `retrieve=retrieve_hybrid_rerank` pasa la función; `retrieve=retrieve_hybrid_rerank(query)` la ejecuta y pasa una corrutina. Los paréntesis significan "ejecuta ahora y dame el resultado". Cuando el parámetro se va a llamar más tarde (dentro de `answer_query`, dentro de `_handle_message`), va el nombre desnudo. + +- **El bot no se tocó.** El cambio de estrategia del motor no requirió ni una línea en `infrastructure/bot/telegram_bot.py`. Es la validación del diseño de ayer: el adaptador solo conoce `AnswerFn`, así que cambiar lo que hay detrás le es invisible. + +### ¿Qué no entendí bien / queda abierto? + +- No medí el costo de latencia del rerank. Ahora hay una llamada extra al LLM por cada pregunta, y eso es un trade-off real que introduje sin cuantificar. +- No sé todavía si hybrid+rerank mejora las respuestas en la práctica. El eval actual tiene data leakage y da ~1.000 en las cuatro estrategias, así que no discrimina. Solo lo voy a saber con queries reales acumuladas del bot (V3). +- Me costó ver que "reemplazar `retrieve_and_generate`" significaba escribir sus tres pasos, no llamarla con otros argumentos. Intenté tres veces reusarla antes de entender que su paso 1 era justo lo que quería cambiar. + +### Decisiones de diseño + +- `answer_query` recibe `retrieve: RetrieveFn` en lugar de `store: VectorStore`. `top_k` desaparece de la firma porque queda capturado en el closure. +- Un solo closure hoy (hybrid+rerank), no las dos estrategias con un `if`. Construir un interruptor que nadie va a mover es generalización prematura. El dispatch por configuración se hará cuando haga falta alternar de verdad. +- `retrieve_and_generate` se deja en su lugar aunque quedó sin llamadores en producción. Eliminarla hoy habría mezclado dos cambios en un commit. Se evalúa en V2, al construir el grafo, donde `retrieve` y `generate` se vuelven nodos separados. +- Se acepta la latencia extra del rerank sin optimizar. Medirla primero, decidir después. + +### Errores interesantes + +- Escribí `RetrieveFn = Callable[[str]], Awaitable[list[Document]]` — corchetes mal cerrados. `Callable[[str]]` se cierra solo y la coma convierte todo en una **tupla** de dos elementos, no en un tipo de función. `Callable` recibe sus dos argumentos dentro de un solo par de corchetes. +- Pasé `retrieve=retrieve_hybrid_rerank(query)` con paréntesis. Habría fallado con `TypeError: 'coroutine' object is not callable` más un `RuntimeWarning` de corrutina nunca esperada. Curioso: tres líneas abajo pasé `answer_fn=answer` correctamente, sin paréntesis. +- Primer intento de los closures: sin parámetro `query` (no cumplían `Callable[[str], ...]`), construyendo el embedder adentro, y sin `return`. +- Nombré mi closure `hybrid_search`, colisionando con el import de `retrieval_service`. El `def` local habría tapado el import. +- Import de `SAMPLES_DIR` sin uso. +- Tercer error mecánico de escritura de Python en una semana (los anteriores: `:` en vez de `=` en una asignación, `self` omitido en firmas de Protocol). No son conceptuales — es un hueco de automatismo que se cierra con repetición. + +--- diff --git a/docs/work_log.md b/docs/work_log.md index ce869a6..903df56 100644 --- a/docs/work_log.md +++ b/docs/work_log.md @@ -193,3 +193,32 @@ no trunca ni divide respuestas largas antes de `reply_text` --- + +## 2026-08-11 + +### Trabajo desarrollado +- `rag_service.answer_query` refactorizado: recibe `retrieve: RetrieveFn` + (`Callable[[str], Awaitable[list[Document]]]`) inyectado en vez de + `store: VectorStore`; compone directamente `retrieve(query)` → + `build_rag_messages(...)` → `llm.generate(...)` en lugar de llamar a + `retrieve_and_generate` +- `scripts/run_telegram_bot.py` rearmado como composition root: construye + `ChromaVectorStore` y `BM25Retriever`, y un closure `retrieve_hybrid_rerank` + que encadena `hybrid_search` + `hybrid_rerank_search`; el bot ahora + responde con hybrid+rerank en vez de vector-only +- `scripts/eval_retrieval.py`: `_rerank` movida antes de `main` para mejorar + legibilidad, y su `k` hardcodeado (`10`) reemplazado por `K * 2` (`152d651`) +- `TelegramBot` no se modificó — el cambio de estrategia de retrieval quedó + completamente aislado del adaptador, validando el diseño de `AnswerFn` + +### Próximos pasos +- Evaluar eliminación de `retrieve_and_generate` al construir el grafo de + V2 — sin llamadores en producción; al eliminar, actualizar también el + ejemplo del docstring de `agent_utils.py` (línea 18) y su test en + `tests/unit/application/test_agent_utils.py` +- Arreglar `test_extract_text_pdf`: depende de un PDF no versionado + (`data/samples/sample_pdf.pdf` no está en git), falla en clon limpio +- Manejar el límite de 4096 caracteres por mensaje de Telegram +- Medir la latencia añadida por el paso de rerank + +--- diff --git a/scripts/eval_retrieval.py b/scripts/eval_retrieval.py index 9ec7e03..f857433 100644 --- a/scripts/eval_retrieval.py +++ b/scripts/eval_retrieval.py @@ -62,6 +62,14 @@ def _reciprocal_rank(results: list[Document], source_paper: str) -> float: return 0.0 +async def _rerank( + query: str, chroma: ChromaVectorStore, bm25: BM25Retriever, llm: AnthropicLLM +) -> list[Document]: + """Run hybrid search then rerank the top-10 with Claude.""" + candidates = await hybrid_search(query, retrievers=[chroma, bm25], k=K * 2) + return await hybrid_rerank_search(query=query, llm=llm, documents=candidates, k=K) + + async def main() -> None: """Run the full evaluation loop and print a strategy comparison table.""" with open(path_examples, encoding="utf-8") as f: @@ -120,13 +128,5 @@ async def main() -> None: print(f"{name:<15} {avg_p:>6.3f} {avg_mrr:>6.3f}") -async def _rerank( - query: str, chroma: ChromaVectorStore, bm25: BM25Retriever, llm: AnthropicLLM -) -> list[Document]: - """Run hybrid search then rerank the top-10 with Claude.""" - candidates = await hybrid_search(query, retrievers=[chroma, bm25], k=10) - return await hybrid_rerank_search(query=query, llm=llm, documents=candidates, k=K) - - if __name__ == "__main__": asyncio.run(main()) diff --git a/scripts/run_telegram_bot.py b/scripts/run_telegram_bot.py index 05c9c63..59a4c05 100644 --- a/scripts/run_telegram_bot.py +++ b/scripts/run_telegram_bot.py @@ -4,20 +4,47 @@ __import__("pysqlite3") sys.modules["sqlite3"] = sys.modules.pop("pysqlite3") +import chromadb + from researchos.application.services.rag_service import answer_query +from researchos.application.services.retrieval_service import hybrid_rerank_search, hybrid_search from researchos.config import settings +from researchos.domain.models import Document from researchos.infrastructure.bot.telegram_bot import TelegramBot from researchos.infrastructure.llm.anthropic_llm import AnthropicLLM +from researchos.infrastructure.retrieval.bm25 import BM25Retriever from researchos.infrastructure.retrieval.chroma import ChromaVectorStore from researchos.infrastructure.retrieval.embedder import LocalEmbedder +from researchos.paths import CHROMA_DIR + +# ── Build retrievers ── +COLLECTION_NAME = "papers" +K = 5 + embedder = LocalEmbedder() -chroma = ChromaVectorStore(embedder=embedder, collection_name="papers") +chroma = ChromaVectorStore(embedder=embedder, collection_name=COLLECTION_NAME) llm = AnthropicLLM() +raw = ( + chromadb.PersistentClient(path=str(CHROMA_DIR)) + .get_collection(COLLECTION_NAME) + .get(include=["documents", "metadatas"]) +) +all_docs = [ + Document(doc_id=doc_id, text=text, metadata=metadata) + for doc_id, text, metadata in zip(raw["ids"], raw["documents"], raw["metadatas"], strict=False) +] +bm25 = BM25Retriever(documents=all_docs) + + +async def retrieve_hybrid_rerank(query: str) -> list[Document]: + candidates = await hybrid_search(query, retrievers=[chroma, bm25], k=K * 2) + return await hybrid_rerank_search(query=query, llm=llm, documents=candidates, k=K) + async def answer(query: str) -> str: - return await answer_query(query, llm=llm, store=chroma) + return await answer_query(query, llm, retrieve=retrieve_hybrid_rerank) bot = TelegramBot(token=settings.telegram_bot_token, answer_fn=answer) diff --git a/src/researchos/application/services/rag_service.py b/src/researchos/application/services/rag_service.py index 896975a..9d8b8dc 100644 --- a/src/researchos/application/services/rag_service.py +++ b/src/researchos/application/services/rag_service.py @@ -1,13 +1,16 @@ -from researchos.application.agents.agent_utils import retrieve_and_generate -from researchos.domain.interfaces import LLMProvider, VectorStore +from collections.abc import Awaitable, Callable + +from researchos.application.agents.agent_utils import build_rag_messages +from researchos.domain.interfaces import LLMProvider +from researchos.domain.models import Document from researchos.domain.prompts import PromptTemplate +RetrieveFn = Callable[[str], Awaitable[list[Document]]] + -async def answer_query( - query: str, - llm: LLMProvider, - store: VectorStore, - top_k: int = 5, -) -> str: +async def answer_query(query: str, llm: LLMProvider, retrieve: RetrieveFn) -> str: system_prompt = PromptTemplate("system", "agent").render() - return await retrieve_and_generate(query, llm, store, system_prompt, top_k) + + docs = await retrieve(query) + messages = build_rag_messages(query, docs, system_prompt) + return await llm.generate(messages)