Feat/synthetic text generation - #14
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
1c6c84c to
0e4dd1d
Compare
fatemetkl
left a comment
fatemetkl
left a comment
There was a problem hiding this comment.
Awesome first steps toward the SLM alignment and distillation RI! 🎉
I've left a few comments, questions, and suggestions. Some can be addressed in follow-up PRs, but I think many could be incorporated into this one. Happy to discuss any of the comments once you've had a chance to look through them. Let me know if you have any questions!
| @@ -1,2 +1,22 @@ | |||
| ENV_VARIABLE_ONE=<your_environment_variable_value_one> | |||
| ENV_VARIABLE_TWO=<your_environment_variable_value_two> | |||
| # Strong model used for synthetic data generation (teacher / distillation) | |||
There was a problem hiding this comment.
I think it might be better to move this file under the specific use-case directory since each usecase might need different APIs and also for simplicity.
There was a problem hiding this comment.
If you did so, don't forget to update RI's README: cp .env.example .env # then add your API key
| @@ -1,33 +1,168 @@ | |||
| # [Implementation Name] Overview | |||
| # Policy-Document QA Alignment | |||
There was a problem hiding this comment.
I think we can use a more general name to cover different document types for this reference implementation. Something like "Domain Specific QA Alignment" or "Domain Specific SLM Alignment" as the title of this set of notebooks, especially since we want to also add other types of documents (other domains).
|
|
||
| - Python 3.12+ | ||
| - [uv](https://github.com/astral-sh/uv) for environment management | ||
| - OpenAI-compatible API key for teacher and judge models (see `.env.example`) |
There was a problem hiding this comment.
Based on my earlier comment, we might want to bring .env.example file under this reference implementation directory, and update the README accordingly.
| cp .env.example .env # then add your API key | ||
| ``` | ||
|
|
||
| ## Data layout |
There was a problem hiding this comment.
I think there is an old asset folder that we want to remove for this PR if we don't need it.
| ``` | ||
| data/ | ||
| ├── documents/ # source policy text (sample excerpts included) | ||
| ├── paragraphs.jsonl # chunked paragraphs with train/test split (Step 1) | ||
| ├── test/ | ||
| │ └── test_set.jsonl # held-out evaluation Q&A (Step 1) | ||
| ├── synthetic/ | ||
| │ ├── synthetic_raw.jsonl | ||
| │ ├── synthetic_filtered.jsonl | ||
| │ └── synthetic_train.jsonl | ||
| └── results/ | ||
| ├── baseline_predictions.jsonl | ||
| ├── baseline_scores.json | ||
| └── comparison_report.json | ||
| ``` |
There was a problem hiding this comment.
Assuming step here refers to the notebook number, we can also mention the steps at which other data files are generated.
| return [paragraph for _, paragraph in scored[:top_k]] | ||
|
|
||
|
|
||
| def grounded_qa_prompt(passage: str) -> str: |
There was a problem hiding this comment.
This is the main prompt design used for notebook 4.
There was a problem hiding this comment.
In this notebook, we generate QA pairs for each paragraph with a new prompt that asks the model to generate a QA whose answer can be supported by the paragraph: Write one instruction-following question and answer pair.\n" "The answer must be fully supported by the passage.
I feel like we cannot call this approach Retrieval-Augmented Synthetic Data. Previous approaches were also based on a paragraph with a different type of prompt, where we allowed the model to generate QAs however it wants based on the given paragraph.
I think we should call the approach in this method "Instruction back-translation". It is a well-known technique proposed in this work: https://openreview.net/forum?id=1oijHJBRsT
What they ask the LLM to do --> Given web text y, generate instruction x for which y would be a good response.
I think in the implementation of this approach, we should change the naming and also change the main prompt to generate only the question to which the paragraph provides the answer.
Also, I would suggest placing the prompts that are used in different techniques in a prompt-config Python file so that they are easy to find and change by the participants. Feel free to propose another code design for managing prompts as well.
There was a problem hiding this comment.
Maybe I mentioned this in another comment (sorry if it is repetitive): I think it would be nice to be able to let participants try different SLMs and to have that flexibility ( if it is not too much trouble). I think one way to do it is to start the Ollama server manually (by the participant) after the coder project is up and running.
General comments:
I think there are a few functionalities that are implemented but not used.
For example: retrieve_paragraphs() function in rag.py
Are we planning on using it? Seems like it could be really useful for a RAG-based generation method. I have some ideas and will share them with you.
In notebook 2, we generated training datasets with different approaches and filtered them in notebook3. Can we also do fine-tuning with each of those datasets and report performance? Mainly to see whether those generation methods are useful. How do they compare to the results of the 4th notebook?
We can do it one time on our side, report the results, and keep them as optional steps for participants to be considerate of the available compute.
Look into the differences between the LLM as judge used for evaluating the performance of the SLM (correctness and quality of responses) vs the LLM as judge that is used for quality filtering of synthesized QA pairs (no model response yet).
There was a problem hiding this comment.
In notebook 2, we generated training datasets with different approaches and filtered them in notebook3. Can we also do fine-tuning with each of those datasets and report performance? Mainly to see whether those generation methods are useful. How do they compare to the results of the 4th notebook?
We can do it one time on our side, report the results, and keep them as optional steps for participants to be considerate of the available compute.
Look into the differences between the LLM as judge used for evaluating the performance of the SLM (correctness and quality of responses) vs the LLM as judge that is used for quality filtering of synthesized QA pairs (no model response yet).
Sounds intersting, we can address in follow up PRs.
| logger.setLevel(logging.DEBUG) | ||
|
|
||
|
|
||
| def build_absolute_judge_prompt( |
There was a problem hiding this comment.
This prompt looks good for when we are using LLM as judge for evaluating the SLM performance. However, we should probably use a slightly different prompt for evaluating the quality of the generated QA pairs given the context of a paragraph or text around a paragraph.
| return parse_judge_score(sample.id, payload) | ||
|
|
||
|
|
||
| def judge_synthetic_sample(client: LLMClient, sample: QASample) -> JudgeScore: |
There was a problem hiding this comment.
This function is good for scoring because we actually have a model response that we compare against the gold answer, but we cannot use it for quality assessment of synthetic data. The quality assessment phase is done before inference, so we don't usually have the model response yet, and the goal is just to assess the quality of the data based on the question and the answer.
Decouple Ollama from Docker boot, document Coder-first setup, and load the RI .env via a stable helper so notebooks keep working after chdir. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds a reusable synthetic-data bootcamp package implementing end-to-end policy-document QA workflows with typed schemas, document processing, LLM clients, multiple generation strategies, filtering, grounded RAG, evaluation, optional LoRA fine-tuning, five workflow notebooks, finance source documents, environment templates, and workspace configuration. ChangesSynthetic QA workflow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook as Notebook
participant BuildSplits as build_paragraph_splits
participant GenerateTest as generate_test_qa_batch
participant TeacherModel as TeacherModel(LLM)
participant SmallModel as SmallModel(LLM)
participant JudgeModel as JudgeModel(LLM)
participant SaveResults as save_baseline_results
Notebook->>BuildSplits: load documents, chunk paragraphs, sample test/train
BuildSplits->>Notebook: return paragraph splits
Notebook->>GenerateTest: generate hard test Q&A for each test paragraph
GenerateTest->>TeacherModel: topic-controlled QA with failure modes
GenerateTest->>Notebook: return QASample list
Notebook->>SmallModel: run_inference on test samples
SmallModel->>Notebook: return predictions
Notebook->>JudgeModel: score_predictions against test samples
JudgeModel->>Notebook: return JudgeScore list
Notebook->>SaveResults: persist predictions and summaries
SaveResults->>Notebook: overall and by-failure-mode metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
.github/ISSUE_TEMPLATE/config.yml-4-4 (1)
4-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the broken Discussions link.
The configured Discussions URL returns 404, so the issue-template contact link is unusable. Point it to an enabled support channel instead. ()
🤖 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/ISSUE_TEMPLATE/config.yml at line 4, Update the contact URL in the issue-template configuration to point to an enabled, valid support channel instead of the broken GitHub Discussions address. Preserve the existing configuration structure and replace only the URL value.Dockerfile-6-7 (1)
6-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the container Ollama command runnable from its default directory.
The image starts in
/home/coder, while the repository is copied to/home/coder/aieng-synthetic-data; following either instruction as-is cannot findscripts/start-ollama.sh.
Dockerfile#L6-L7: usebash aieng-synthetic-data/scripts/start-ollama.sh, or make the repository directory the imageWORKDIR.scripts/setup.sh#L6-L8: document the same container-valid command.🤖 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 `@Dockerfile` around lines 6 - 7, The Ollama startup instruction uses a repository-relative path that is invalid from the image’s default /home/coder directory. Update Dockerfile lines 6-7 to use the container-valid aieng-synthetic-data/scripts/start-ollama.sh path or set the repository as WORKDIR, and update scripts/setup.sh lines 6-8 to document the same command; no other startup behavior needs to change.aieng-synthetic-data/aieng/syn_data/text/sft.py-101-101 (1)
101-101: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRemove the unneeded
trust_remote_code=Truefrom model/tokenizer loads.Qwen2.5-3B-Instruct is supported natively by modern
transformers, so this flag is not required and unnecessarily enables arbitrary Python execution from remote repositories for every load path.Also applies to: lines 115-116, 162-164, and 168-170.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/sft.py` at line 101, Remove trust_remote_code=True from the AutoTokenizer and model loading calls in the synthetic data flow, including the loads near lines 101, 115-116, 162-164, and 168-170. Keep the existing model and tokenizer arguments otherwise unchanged so native transformers support is used without enabling remote code execution.Source: Linters/SAST tools
implementations/qa_text_generation/05_finetune_and_compare.ipynb-31-31 (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTypo: "Prerequisit" → "Prerequisites".
🤖 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 `@implementations/qa_text_generation/05_finetune_and_compare.ipynb` at line 31, Correct the Markdown heading text from “Prerequisit” to “Prerequisites” in the notebook content.implementations/qa_text_generation/01_baseline_evaluation.ipynb-33-47 (1)
33-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEmpty markdown cell and unfenced Mermaid diagram.
The cell at Lines 27-32 is empty, and the flowchart at Lines 38-45 has no ```mermaid fence, so JupyterLab/Colab renders it as plain text instead of a diagram.
📝 Proposed fix
"source": [ - "flowchart LR\n", + "```mermaid\n", + "flowchart LR\n", " A[Pick domain & tasks] --> B[Build test set]\n", " B --> C[Run baseline model]\n", " C --> D[LLM judge scores answers]\n", " D --> E[Generate synthetic data]\n", " E --> F[Fine-tune model]\n", " F --> G[Re-run same test]\n", - " G --> H[Compare scores]\n" + " G --> H[Compare scores]\n", + "```" ]🤖 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 `@implementations/qa_text_generation/01_baseline_evaluation.ipynb` around lines 33 - 47, Remove the empty markdown cell preceding the flowchart and update the flowchart markdown cell to wrap the Mermaid content in a mermaid code fence. Add the opening mermaid fence before flowchart LR and the closing fence after the final diagram edge, preserving the existing diagram definition.implementations/qa_text_generation/04_grounded_data_augmentation.ipynb-16-23 (1)
16-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe documented "500–1,000 sample" SFT target is unreachable with the shipped 36 train paragraphs. Both places advertise a corpus size two orders of magnitude above what the pipeline actually produces (final: 36 samples after dedup), and notebook 04 also advertises a retrieval step that never runs.
implementations/qa_text_generation/04_grounded_data_augmentation.ipynb#L16-L23: correct the stated target to what the current document set yields, and drop or rename the "Retrieve relevant passages" objective since paragraphs are cycled directly rather than retrieved.implementations/qa_text_generation/README.md#L52-L52: update the notebook-04 row so it no longer claims "(500–1k)".🤖 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 `@implementations/qa_text_generation/04_grounded_data_augmentation.ipynb` around lines 16 - 23, The documented SFT corpus target and retrieval objective are inaccurate. In implementations/qa_text_generation/04_grounded_data_augmentation.ipynb lines 16-23, update the target to the 36 samples produced by the current 36 training paragraphs and remove or rename the “Retrieve relevant passages” learning objective to reflect direct paragraph cycling; in implementations/qa_text_generation/README.md line 52, remove the “500–1k” claim from the notebook-04 description.implementations/qa_text_generation/01_baseline_evaluation.ipynb-119-127 (1)
119-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
logging.basicConfig(level=DEBUG, force=True)plus committed outputs bloats these notebooks by thousands of lines. One root cause: DEBUG-level logging captures everyurllib3connection and every raw model payload, and the outputs were saved into version control (including hundreds of repeated).\"/.\"lines from truncated completions).
implementations/qa_text_generation/01_baseline_evaluation.ipynb#L119-L127: lower toINFOand seturllib3toWARNING, then clear outputs before commit (removes ~2,500 lines).implementations/qa_text_generation/02_synthetic_qa_generation.ipynb#L100-L108: apply the same log-level change; this also removes the ~1,000-line degenerate-output block at Lines 928-1863.03_quality_filtering.ipynbinherits the same stored-log noise (Lines 639-731, 1038-1130) and is fixed by clearing its outputs too.🤖 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 `@implementations/qa_text_generation/01_baseline_evaluation.ipynb` around lines 119 - 127, Reduce logging verbosity in the notebook initialization blocks at implementations/qa_text_generation/01_baseline_evaluation.ipynb lines 119-127 and implementations/qa_text_generation/02_synthetic_qa_generation.ipynb lines 100-108 by using INFO as the basicConfig level and configuring urllib3 at WARNING; then clear all saved outputs in both notebooks and implementations/qa_text_generation/03_quality_filtering.ipynb, with no direct logging change required in the latter.implementations/qa_text_generation/02_synthetic_qa_generation.ipynb-223-224 (1)
223-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
console = Console()discards the configured width.The setup cell already created
console = Console(width=100); reassigning here drops the fixed width, so subsequent output wraps unpredictably (visible in the wider panels of the stored output). Drop this line.📝 Proposed fix
-console = Console() console.print(Panel(table, title="First Train Paragraph Overview"))🤖 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 `@implementations/qa_text_generation/02_synthetic_qa_generation.ipynb` around lines 223 - 224, Remove the redundant console reassignment before the “First Train Paragraph Overview” panel; reuse the existing Console instance created with width=100 so output retains the configured fixed width.implementations/qa_text_generation/README.md-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced block (markdownlint MD040).
📝 Proposed fix
-``` +```text data/🤖 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 `@implementations/qa_text_generation/README.md` at line 25, Add the text language identifier to the fenced code block containing the data/ example in the README, changing the opening fence to use ```text while preserving the block contents.Source: Linters/SAST tools
implementations/qa_text_generation/01_baseline_evaluation.ipynb-1960-1971 (1)
1960-1971: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
rich.json.JSONfor the JSON preview
Syntax.from_json()is not part of Rich’s public API; withrich>=14.2.0,rich.json.JSON.from_data()supports dicts directly and acceptsindent, orconsole.print_json()can be used for direct output.
[functional_contract]🤖 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 `@implementations/qa_text_generation/01_baseline_evaluation.ipynb` around lines 1960 - 1971, Replace the `Syntax.from_json` preview logic with Rich’s public `rich.json.JSON.from_data()` API, passing `sample_dict` and the existing two-space indentation, then print the resulting JSON panel. Remove the availability check and standard-library JSON fallback while preserving the `First Prediction` title and existing sample conversion.aieng-synthetic-data/aieng/syn_data/text/rag.py-106-111 (1)
106-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback JSON parsing has no repair/extraction at all.
Unlike
OpenAICompatibleClient.complete_json(extract_json_text → repair_json → json.loads), this fallback branch (triggered whenclientlackscomplete_json) does a rawjson.loads(raw). Any markdown fence, leading prose, or minor JSON malformation from the fallback model will raise an uncaughtJSONDecodeError.🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/rag.py` around lines 106 - 111, Update the fallback branch in the RAG generation flow to parse the response using the same extraction and JSON-repair pipeline as OpenAICompatibleClient.complete_json, rather than calling json.loads directly on raw. Reuse the existing extract_json_text and repair_json helpers, then deserialize the repaired JSON while preserving the current prompt and payload behavior.aieng-synthetic-data/aieng/syn_data/text/generation.py-183-184 (1)
183-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback JSON parsing skips
extract_json_text, unlike thecomplete_jsonpath.When
clientlackscomplete_json,_generate_topicsparsesrawdirectly withjson.loads(raw). If the fallback model wraps its answer in prose or a markdown fence (a documented failure mode this codebase otherwise guards against viaextract_json_text/repair_json), this will raise an uncaughtJSONDecodeError.extract_json_textis already imported in this file.🐛 Proposed fix
raw = client.complete(prompt, system="You extract policy topics.") - return list(json.loads(raw).get("topics", [])) + return list(json.loads(extract_json_text(raw)).get("topics", []))🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/generation.py` around lines 183 - 184, Update the fallback branch in _generate_topics to run the raw response through the existing extract_json_text helper before json.loads, matching the complete_json parsing path and supporting prose or markdown-wrapped JSON responses.aieng-synthetic-data/aieng/syn_data/text/clients.py-140-163 (1)
140-163: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNo defensive handling for malformed API responses.
payload["choices"][0]andchoice["message"]assume a well-formed OpenAI-style response. Ifchoicesis missing or empty (e.g., misconfiguredbase_urlpointing at a non-compliant server), this raises an uncaughtKeyError/IndexErrorinstead of the clearValueErrorused for the null-content case just below.🛡️ Proposed fix
payload = response.json() - choice = payload["choices"][0] + choices = payload.get("choices") or [] + if not choices: + msg = f"Model {self.settings.model} returned no choices: {payload!r}" + raise ValueError(msg) + choice = choices[0] content = choice["message"].get("content")🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/clients.py` around lines 140 - 163, Update the response parsing in the retry loop around payload and choice extraction to defensively validate that the API response contains a non-empty choices collection and a message object before accessing them. Raise a clear ValueError, consistent with the existing null-content handling, for missing or malformed response fields while preserving normal content extraction.
🧹 Nitpick comments (10)
implementations/qa_text_generation/03_quality_filtering.ipynb (1)
1568-1569: 📐 Maintainability & Code Quality | 🔵 TrivialJudge-model TODO.
Using the same model family for generation and judging inflates scores — consistent with the all-5.0 summary in the stored output. Want me to open an issue to track swapping the judge to a different family?
🤖 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 `@implementations/qa_text_generation/03_quality_filtering.ipynb` around lines 1568 - 1569, Update the judge configuration around create_judge_client so it uses a model from a different family than the generation model, and remove the stale TODO. Preserve the existing judge initialization flow while selecting the alternate-family model through the established configuration or client options.implementations/qa_text_generation/02_synthetic_qa_generation.ipynb (1)
416-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
from rich.text import Textout of the loop.It re-imports on every strategy iteration; belongs with the other
richimports in the setup cell.🤖 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 `@implementations/qa_text_generation/02_synthetic_qa_generation.ipynb` around lines 416 - 421, Move the rich.text Text import from inside the strategy_outputs iteration to the notebook’s setup cell alongside the other rich imports, and leave the loop using that existing import without re-importing it per strategy.implementations/qa_text_generation/01_baseline_evaluation.ipynb (2)
136-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
%autoreloaddebug scaffolding from all five notebooks before merge. Each notebook carries the same# TODO: remove this before merging into maincell; the shared root cause is development-time scaffolding that shipped with the tutorial notebooks.
implementations/qa_text_generation/01_baseline_evaluation.ipynb#L136-L140: delete the TODO +%load_ext autoreload/%autoreload 2cell.implementations/qa_text_generation/02_synthetic_qa_generation.ipynb#L87-L91: delete the same cell.implementations/qa_text_generation/03_quality_filtering.ipynb#L85-L88: delete the same cell.implementations/qa_text_generation/04_grounded_data_augmentation.ipynb#L93-L96: delete the same cell.implementations/qa_text_generation/05_finetune_and_compare.ipynb#L94-L97: delete the same cell.🤖 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 `@implementations/qa_text_generation/01_baseline_evaluation.ipynb` around lines 136 - 140, Remove the development-time autoreload cell, including the TODO marker and both autoreload directives, from implementations/qa_text_generation/01_baseline_evaluation.ipynb lines 136-140, implementations/qa_text_generation/02_synthetic_qa_generation.ipynb lines 87-91, implementations/qa_text_generation/03_quality_filtering.ipynb lines 85-88, implementations/qa_text_generation/04_grounded_data_augmentation.ipynb lines 93-96, and implementations/qa_text_generation/05_finetune_and_compare.ipynb lines 94-97.
1622-1639: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis cell silently discards the freshly generated
test_samples.Reloading from
TEST_SET_PATHunconditionally means a participant who just ran section 3 overwrites in-memory results with the file (note the staleexecution_count: 5versus8on the generating cell). Guard it or mark it clearly as an alternative entry point.Also
load_typed_jsonlis imported here fromaieng.syn_data.text.iowhile the setup cell imports the rest from the package root — worth importing it at the top with the others for consistency.♻️ Proposed fix
-from aieng.syn_data.text.io import load_typed_jsonl - - -test_samples = load_typed_jsonl(TEST_SET_PATH, QASample.from_dict) +# Optional: skip generation and reuse a previously saved test set. +if "test_samples" not in globals(): + test_samples = load_typed_jsonl(TEST_SET_PATH, QASample.from_dict)🤖 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 `@implementations/qa_text_generation/01_baseline_evaluation.ipynb` around lines 1622 - 1639, Update the test_samples loading cell around load_typed_jsonl so it does not unconditionally overwrite freshly generated in-memory samples; guard the reload for an alternative entry point or clearly separate it from the generation path. Move the load_typed_jsonl import into the notebook’s setup imports alongside the other package-root imports, and remove the duplicate local import.aieng-synthetic-data/aieng/syn_data/text/config.py (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded ancestor-depth index is fragile vs. the existing robust root finder.
_REPO_ROOTis computed via a fixed.parents[4]index with no validation that the resolved directory is actually correct.paths.py'sfind_repo_root()already implements a safer walk-up (checks forpyproject.toml+implementations/qa_text_generation), but it can't be reused here due to the circular dependency (paths.pyimportsIMPLEMENTATION_DIRfrom this module). Consider duplicating that walk-up check directly inconfig.pyinstead of the magic index, so a future file move doesn't silently point_REPO_ROOTat the wrong directory.🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/config.py` around lines 15 - 16, Replace the fixed `_REPO_ROOT = ...parents[4]` calculation in config.py with a local upward directory search that identifies the repository root by requiring both `pyproject.toml` and `implementations/qa_text_generation`; avoid importing `paths.py` to preserve the existing dependency direction, and keep `IMPLEMENTATION_DIR` based on the validated root.aieng-synthetic-data/aieng/syn_data/__init__.py (1)
10-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLibrary logger disables propagation — will silently block external log capture.
Adding a handler to
aieng.syn_dataand settingpropagate = Falsemeans any code that captures logs via the root logger (e.g.pytest'scaplog, a host application's own logging config) will never see records from this package's submodules. The standard guidance for library code is to avoid configuring handlers/propagation itself and let the application own that.♻️ Suggested approach
def _setup_package_logger() -> None: pkg_logger = logging.getLogger(_PACKAGE_LOGGER_NAME) - if getattr(pkg_logger, "_aieng_configured", False): + if pkg_logger.handlers: return handler = logging.StreamHandler() handler.setFormatter(logging.Formatter(_LOG_FORMAT)) pkg_logger.addHandler(handler) pkg_logger.setLevel(logging.INFO) - pkg_logger.propagate = False - pkg_logger._aieng_configured = True # type: ignore[attr-defined] + # Leave propagate=True (default) so caplog/root handlers still see records.🤖 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 `@aieng-synthetic-data/aieng/syn_data/__init__.py` around lines 10 - 26, Update _setup_package_logger to stop attaching a StreamHandler and configuring propagation for the library logger; leave handler and propagation ownership to the consuming application so records from aieng.syn_data submodules reach external and root-logger capture. Remove only the package-level handler setup and related configuration while preserving the logger setup entry point as needed.aieng-synthetic-data/aieng/syn_data/text/clients.py (1)
17-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winModule-level
logger.setLevel(logging.DEBUG)hardcoded in two files. Bothclients.pyandjudge.pyforce their logger to DEBUG at import time, overriding whatever level the consuming application/notebook configures, and enabling verbose logging of full LLM payloads (extracted JSON, model answers) by default.
aieng-synthetic-data/aieng/syn_data/text/clients.py#L17-L19: remove the hardcodedsetLevel(logging.DEBUG)(per its own TODO) and let the application configure the logger, or gate it behind an env var.aieng-synthetic-data/aieng/syn_data/text/judge.py#L20-L21: same fix — remove the hardcodedsetLevel(logging.DEBUG).🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/clients.py` around lines 17 - 19, Remove the module-level logger.setLevel(logging.DEBUG) call in clients.py and the corresponding call in judge.py, allowing consuming applications or notebooks to configure logging levels; retain the existing logger definitions and avoid enabling verbose payload logging by default.aieng-synthetic-data/aieng/syn_data/text/rag.py (1)
106-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFallback JSON-parsing paths skip the robust extract/repair pipeline used elsewhere. When
clientlackscomplete_json, both sites fall back to parsing raw model text directly, unlikeOpenAICompatibleClient.complete_json(extract_json_text → repair_json → json.loads) orgeneration._parse_generation_response(extract_json_text → json.loads). A markdown-fenced or prose-wrapped fallback response will raise an uncaughtJSONDecodeErrorin either function.
aieng-synthetic-data/aieng/syn_data/text/rag.py#L106-L111: applyextract_json_text(and ideallyrepair_json) torawbeforejson.loads, matchingcomplete_json's robustness.aieng-synthetic-data/aieng/syn_data/text/generation.py#L183-L184: use the already-importedextract_json_textbeforejson.loads(raw).🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/rag.py` around lines 106 - 111, The fallback parsing in rag.py lines 106-111 and generation.py lines 183-184 must normalize model output before JSON decoding. In rag.py, update the fallback around client.complete to apply extract_json_text and, preferably, repair_json before json.loads; in generation.py, apply the already-imported extract_json_text before json.loads(raw). Ensure both paths handle fenced or prose-wrapped responses consistently with the existing robust parsing pipeline.aieng-synthetic-data/aieng/syn_data/text/pipeline.py (1)
264-296: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent shortfall when generation attempts are exhausted.
If more than ~1/3 of attempts fail the overlap filter or raise
(KeyError, ValueError, TypeError), the loop exits viaattempts >= max_attemptsand silently returns fewer samples thangoal, with no warning to the caller. Consider logging whenlen(samples) < goalafter the loop so callers/notebooks can detect an under-sized corpus instead of silently proceeding with less data than expected.🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py` around lines 264 - 296, The sampling flow after the while loop in the synthetic-data generation function should warn when attempts are exhausted before len(samples) reaches goal. Add a warning through the module’s existing logging mechanism that reports the requested goal, generated count, and exhausted-attempt condition, while preserving the current return value and successful-generation behavior.aieng-synthetic-data/aieng/syn_data/text/evaluation.py (1)
45-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBatch loops over external LLM calls lack per-item error isolation. Three loops issue repeated external LLM calls with no try/except, so a single transient failure aborts the whole batch and discards all prior progress — unlike
pipeline.generate_grounded_training_corpus, which already wraps its per-attempt LLM call intry/except (KeyError, ValueError, TypeError).
aieng-synthetic-data/aieng/syn_data/text/evaluation.py#L45-L69: wrap the per-sampleclient.complete()call inrun_inferencein a try/except, recording a placeholder/error entry and continuing rather than aborting the whole evaluation run.aieng-synthetic-data/aieng/syn_data/text/pipeline.py#L102-L128: wrap the per-sampletopic_controlled_generatecall ingenerate_test_qa_batchsimilarly.aieng-synthetic-data/aieng/syn_data/text/pipeline.py#L157-L201: wrap the per-paragraphcompare_generation_strategiescall ingenerate_raw_synthetic_corpussimilarly, reusing the guard pattern already present ingenerate_grounded_training_corpus.🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/evaluation.py` around lines 45 - 69, Isolate failures from repeated external LLM calls so one transient error does not abort batch processing. In aieng-synthetic-data/aieng/syn_data/text/evaluation.py lines 45-69, update run_inference to catch per-sample client.complete errors, record a placeholder/error result, and continue; apply the same guard in aieng-synthetic-data/aieng/syn_data/text/pipeline.py lines 102-128 around generate_test_qa_batch’s topic_controlled_generate call and lines 157-201 around generate_raw_synthetic_corpus’s compare_generation_strategies call, reusing the existing generate_grounded_training_corpus pattern.
🤖 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 @.github/workflows/code_checks.yml:
- Around line 61-70: Update the ignore-vulns block in the workflow to remove
GHSA-xm59-rqc7-hhvf and its obsolete nbconvert-related comments. Preserve the
bleach GHSA-g75f-g53v-794x rationale, TODO, and ignore entry unchanged.
In `@aieng-synthetic-data/aieng/syn_data/text/datasets.py`:
- Around line 15-45: Validate the source URL’s apparent suffix against the
destination suffix in download_document/download_url before writing response
bytes, especially when a configured local_path such as a .txt would receive a
.pdf. Raise a clear error on mismatches so invalid downloads are rejected at
download time; preserve valid downloads and existing-file behavior.
In `@aieng-synthetic-data/aieng/syn_data/text/evaluation.py`:
- Around line 45-69: Update run_inference to isolate each client.complete call
with per-sample exception handling, so one failed inference does not abort the
loop or discard earlier predictions. Preserve successful prediction records, and
append an appropriate failure record for samples whose external LLM call raises,
consistent with the existing evaluation result schema and error-handling pattern
in generate_grounded_training_corpus.
In `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py`:
- Around line 157-201: Wrap each paragraph’s compare_generation_strategies call
in generate_raw_synthetic_corpus with per-paragraph exception handling, so a
failed generation is skipped while samples from prior and subsequent paragraphs
remain collected. Reuse the error-isolation pattern established by
generate_grounded_training_corpus, preserving the existing seed creation and
successful samples.extend behavior.
In `@aieng-synthetic-data/aieng/syn_data/text/sft.py`:
- Line 59: Update the SFT training setup around the SFTTrainer construction to
pass the padded tokenizer as processing_class=tokenizer, ensuring padding is
available during training. Route the existing max_seq_length setting through the
supported SFTConfig sequence-length field, or preserve the kwarg only if the
configured TRL version explicitly maps it to that field; remove the currently
unused standalone parameter path.
In `@aieng-synthetic-data/aieng/syn_data/text/small_model.py`:
- Around line 31-38: Update the base_url construction in the LLMSettings
initialization so the final value from either SMALL_MODEL_BASE_URL or the
OPENAI_BASE_URL fallback is stripped of trailing slashes. Ensure the
.rstrip("/") operation applies to the entire environment-variable fallback
expression before OpenAICompatibleClient.complete uses it.
In `@implementations/qa_text_generation/03_quality_filtering.ipynb`:
- Around line 1567-1580: Update the quality-filtering flow around
filter_with_judge so heuristic rejections are retained in the final report. Pass
raw_samples to filter_with_judge so it owns both filtering stages, or preserve
distinct heuristic and judge rejection collections and combine them before
reporting; ensure the rejected count includes all samples removed from the
original input.
In `@implementations/qa_text_generation/04_grounded_data_augmentation.ipynb`:
- Around line 898-912: Cap the value passed as target_size in the notebook’s
grounded generation setup to the available train_paragraphs count, while
preserving the existing effective_synthetic_target calculation and logging.
Ensure generate_grounded_training_corpus is not asked to cycle paragraphs beyond
the pool size, avoiding duplicate teacher calls and discarded questions.
In `@implementations/qa_text_generation/05_finetune_and_compare.ipynb`:
- Around line 80-82: Update the RUN_SFT default in the notebook configuration to
"0" instead of "1", while preserving the existing environment-variable override
and boolean conversion so training only runs when explicitly enabled.
In `@pyproject.toml`:
- Around line 36-44: Update the CI dependency-audit workflow to install the
optional text-sft group defined by text-sft before auditing, or add a separate
audit environment for that group. Ensure pip-audit covers the SFT packages
installed when RUN_SFT=1 in addition to the existing .venv dependencies.
---
Minor comments:
In @.github/ISSUE_TEMPLATE/config.yml:
- Line 4: Update the contact URL in the issue-template configuration to point to
an enabled, valid support channel instead of the broken GitHub Discussions
address. Preserve the existing configuration structure and replace only the URL
value.
In `@aieng-synthetic-data/aieng/syn_data/text/clients.py`:
- Around line 140-163: Update the response parsing in the retry loop around
payload and choice extraction to defensively validate that the API response
contains a non-empty choices collection and a message object before accessing
them. Raise a clear ValueError, consistent with the existing null-content
handling, for missing or malformed response fields while preserving normal
content extraction.
In `@aieng-synthetic-data/aieng/syn_data/text/generation.py`:
- Around line 183-184: Update the fallback branch in _generate_topics to run the
raw response through the existing extract_json_text helper before json.loads,
matching the complete_json parsing path and supporting prose or markdown-wrapped
JSON responses.
In `@aieng-synthetic-data/aieng/syn_data/text/rag.py`:
- Around line 106-111: Update the fallback branch in the RAG generation flow to
parse the response using the same extraction and JSON-repair pipeline as
OpenAICompatibleClient.complete_json, rather than calling json.loads directly on
raw. Reuse the existing extract_json_text and repair_json helpers, then
deserialize the repaired JSON while preserving the current prompt and payload
behavior.
In `@aieng-synthetic-data/aieng/syn_data/text/sft.py`:
- Line 101: Remove trust_remote_code=True from the AutoTokenizer and model
loading calls in the synthetic data flow, including the loads near lines 101,
115-116, 162-164, and 168-170. Keep the existing model and tokenizer arguments
otherwise unchanged so native transformers support is used without enabling
remote code execution.
In `@Dockerfile`:
- Around line 6-7: The Ollama startup instruction uses a repository-relative
path that is invalid from the image’s default /home/coder directory. Update
Dockerfile lines 6-7 to use the container-valid
aieng-synthetic-data/scripts/start-ollama.sh path or set the repository as
WORKDIR, and update scripts/setup.sh lines 6-8 to document the same command; no
other startup behavior needs to change.
In `@implementations/qa_text_generation/01_baseline_evaluation.ipynb`:
- Around line 33-47: Remove the empty markdown cell preceding the flowchart and
update the flowchart markdown cell to wrap the Mermaid content in a mermaid code
fence. Add the opening mermaid fence before flowchart LR and the closing fence
after the final diagram edge, preserving the existing diagram definition.
- Around line 119-127: Reduce logging verbosity in the notebook initialization
blocks at implementations/qa_text_generation/01_baseline_evaluation.ipynb lines
119-127 and implementations/qa_text_generation/02_synthetic_qa_generation.ipynb
lines 100-108 by using INFO as the basicConfig level and configuring urllib3 at
WARNING; then clear all saved outputs in both notebooks and
implementations/qa_text_generation/03_quality_filtering.ipynb, with no direct
logging change required in the latter.
- Around line 1960-1971: Replace the `Syntax.from_json` preview logic with
Rich’s public `rich.json.JSON.from_data()` API, passing `sample_dict` and the
existing two-space indentation, then print the resulting JSON panel. Remove the
availability check and standard-library JSON fallback while preserving the
`First Prediction` title and existing sample conversion.
In `@implementations/qa_text_generation/02_synthetic_qa_generation.ipynb`:
- Around line 223-224: Remove the redundant console reassignment before the
“First Train Paragraph Overview” panel; reuse the existing Console instance
created with width=100 so output retains the configured fixed width.
In `@implementations/qa_text_generation/04_grounded_data_augmentation.ipynb`:
- Around line 16-23: The documented SFT corpus target and retrieval objective
are inaccurate. In
implementations/qa_text_generation/04_grounded_data_augmentation.ipynb lines
16-23, update the target to the 36 samples produced by the current 36 training
paragraphs and remove or rename the “Retrieve relevant passages” learning
objective to reflect direct paragraph cycling; in
implementations/qa_text_generation/README.md line 52, remove the “500–1k” claim
from the notebook-04 description.
In `@implementations/qa_text_generation/05_finetune_and_compare.ipynb`:
- Line 31: Correct the Markdown heading text from “Prerequisit” to
“Prerequisites” in the notebook content.
In `@implementations/qa_text_generation/README.md`:
- Line 25: Add the text language identifier to the fenced code block containing
the data/ example in the README, changing the opening fence to use ```text while
preserving the block contents.
---
Nitpick comments:
In `@aieng-synthetic-data/aieng/syn_data/__init__.py`:
- Around line 10-26: Update _setup_package_logger to stop attaching a
StreamHandler and configuring propagation for the library logger; leave handler
and propagation ownership to the consuming application so records from
aieng.syn_data submodules reach external and root-logger capture. Remove only
the package-level handler setup and related configuration while preserving the
logger setup entry point as needed.
In `@aieng-synthetic-data/aieng/syn_data/text/clients.py`:
- Around line 17-19: Remove the module-level logger.setLevel(logging.DEBUG) call
in clients.py and the corresponding call in judge.py, allowing consuming
applications or notebooks to configure logging levels; retain the existing
logger definitions and avoid enabling verbose payload logging by default.
In `@aieng-synthetic-data/aieng/syn_data/text/config.py`:
- Around line 15-16: Replace the fixed `_REPO_ROOT = ...parents[4]` calculation
in config.py with a local upward directory search that identifies the repository
root by requiring both `pyproject.toml` and
`implementations/qa_text_generation`; avoid importing `paths.py` to preserve the
existing dependency direction, and keep `IMPLEMENTATION_DIR` based on the
validated root.
In `@aieng-synthetic-data/aieng/syn_data/text/evaluation.py`:
- Around line 45-69: Isolate failures from repeated external LLM calls so one
transient error does not abort batch processing. In
aieng-synthetic-data/aieng/syn_data/text/evaluation.py lines 45-69, update
run_inference to catch per-sample client.complete errors, record a
placeholder/error result, and continue; apply the same guard in
aieng-synthetic-data/aieng/syn_data/text/pipeline.py lines 102-128 around
generate_test_qa_batch’s topic_controlled_generate call and lines 157-201 around
generate_raw_synthetic_corpus’s compare_generation_strategies call, reusing the
existing generate_grounded_training_corpus pattern.
In `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py`:
- Around line 264-296: The sampling flow after the while loop in the
synthetic-data generation function should warn when attempts are exhausted
before len(samples) reaches goal. Add a warning through the module’s existing
logging mechanism that reports the requested goal, generated count, and
exhausted-attempt condition, while preserving the current return value and
successful-generation behavior.
In `@aieng-synthetic-data/aieng/syn_data/text/rag.py`:
- Around line 106-111: The fallback parsing in rag.py lines 106-111 and
generation.py lines 183-184 must normalize model output before JSON decoding. In
rag.py, update the fallback around client.complete to apply extract_json_text
and, preferably, repair_json before json.loads; in generation.py, apply the
already-imported extract_json_text before json.loads(raw). Ensure both paths
handle fenced or prose-wrapped responses consistently with the existing robust
parsing pipeline.
In `@implementations/qa_text_generation/01_baseline_evaluation.ipynb`:
- Around line 136-140: Remove the development-time autoreload cell, including
the TODO marker and both autoreload directives, from
implementations/qa_text_generation/01_baseline_evaluation.ipynb lines 136-140,
implementations/qa_text_generation/02_synthetic_qa_generation.ipynb lines 87-91,
implementations/qa_text_generation/03_quality_filtering.ipynb lines 85-88,
implementations/qa_text_generation/04_grounded_data_augmentation.ipynb lines
93-96, and implementations/qa_text_generation/05_finetune_and_compare.ipynb
lines 94-97.
- Around line 1622-1639: Update the test_samples loading cell around
load_typed_jsonl so it does not unconditionally overwrite freshly generated
in-memory samples; guard the reload for an alternative entry point or clearly
separate it from the generation path. Move the load_typed_jsonl import into the
notebook’s setup imports alongside the other package-root imports, and remove
the duplicate local import.
In `@implementations/qa_text_generation/02_synthetic_qa_generation.ipynb`:
- Around line 416-421: Move the rich.text Text import from inside the
strategy_outputs iteration to the notebook’s setup cell alongside the other rich
imports, and leave the loop using that existing import without re-importing it
per strategy.
In `@implementations/qa_text_generation/03_quality_filtering.ipynb`:
- Around line 1568-1569: Update the judge configuration around
create_judge_client so it uses a model from a different family than the
generation model, and remove the stale TODO. Preserve the existing judge
initialization flow while selecting the alternate-family model through the
established configuration or client options.
🪄 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: c07bba16-1626-4bf2-b2d8-b139ed39ebbf
⛔ Files ignored due to path filters (2)
implementations/qa_text_generation/assets/example_image.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.env.example.github/ISSUE_TEMPLATE/config.yml.github/workflows/code_checks.yml.gitignore.pre-commit-config.yamlDockerfileaieng-synthetic-data/README.mdaieng-synthetic-data/aieng/syn_data/__init__.pyaieng-synthetic-data/aieng/syn_data/text/__init__.pyaieng-synthetic-data/aieng/syn_data/text/clients.pyaieng-synthetic-data/aieng/syn_data/text/config.pyaieng-synthetic-data/aieng/syn_data/text/datasets.pyaieng-synthetic-data/aieng/syn_data/text/documents.pyaieng-synthetic-data/aieng/syn_data/text/evaluation.pyaieng-synthetic-data/aieng/syn_data/text/generation.pyaieng-synthetic-data/aieng/syn_data/text/io.pyaieng-synthetic-data/aieng/syn_data/text/judge.pyaieng-synthetic-data/aieng/syn_data/text/paths.pyaieng-synthetic-data/aieng/syn_data/text/pipeline.pyaieng-synthetic-data/aieng/syn_data/text/quality.pyaieng-synthetic-data/aieng/syn_data/text/rag.pyaieng-synthetic-data/aieng/syn_data/text/schemas.pyaieng-synthetic-data/aieng/syn_data/text/sft.pyaieng-synthetic-data/aieng/syn_data/text/small_model.pyaieng-synthetic-data/pyproject.tomlaieng-synthetic-data/tests/test_text_utils.pyimplementations/qa_text_generation/.env.exampleimplementations/qa_text_generation/01_baseline_evaluation.ipynbimplementations/qa_text_generation/02_synthetic_qa_generation.ipynbimplementations/qa_text_generation/03_quality_filtering.ipynbimplementations/qa_text_generation/04_grounded_data_augmentation.ipynbimplementations/qa_text_generation/05_finetune_and_compare.ipynbimplementations/qa_text_generation/README.mdimplementations/qa_text_generation/data/documents/cfpb_credit_card_agreement.txtimplementations/qa_text_generation/data/documents/sec_investor_bulletin.txtimplementations/qa_text_generation/models/.gitkeepimplementations/qa_text_generation/topic_a_a.ipynbimplementations/qa_text_generation/topic_a_b.ipynbpyproject.tomlscripts/setup.shscripts/start-ollama.sh
💤 Files with no reviewable changes (3)
- implementations/qa_text_generation/topic_a_a.ipynb
- .env.example
- implementations/qa_text_generation/topic_a_b.ipynb
| # | ||
| # GHSA-g75f-g53v-794x: bleach linkify(parse_email=True) ReDoS CPU exhaustion | ||
| # bleach is a transitive dep via nbconvert (HTML sanitization); nbconvert does | ||
| # not call linkify(parse_email=True) on untrusted input, so the vulnerable code | ||
| # path is unreachable in this project. The bleach package is deprecated and no | ||
| # fixed version will be released. | ||
| # TODO: Remove this ignore once nbconvert migrates off bleach (e.g., to nh3). | ||
| ignore-vulns: | | ||
| GHSA-xm59-rqc7-hhvf | ||
| GHSA-g75f-g53v-794x |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
lockfile="$(fd -t f '^uv\.lock$' . | head -n 1)"
test -n "${lockfile}"
awk 'BEGIN { RS="\\[\\[package\\]\\]" } /name = "nbconvert"/ { print }' "${lockfile}"Repository: VectorInstitute/synthetic-data-bootcamp
Length of output: 4976
Remove the obsolete nbconvert vulnerability suppression.
The lockfile resolves nbconvert 7.17.1, which is past the release that fixed GHSA-xm59-rqc7-hhvf, so the ignore can be dropped. Keep the bleach rationale and ignore in place as they address a separate dependency concern.
🤖 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/code_checks.yml around lines 61 - 70, Update the
ignore-vulns block in the workflow to remove GHSA-xm59-rqc7-hhvf and its
obsolete nbconvert-related comments. Preserve the bleach GHSA-g75f-g53v-794x
rationale, TODO, and ignore entry unchanged.
| def download_url(url: str, destination: Path, *, timeout: int = 60) -> Path: | ||
| """Download a remote file to a local path.""" | ||
| ensure_parent(destination) | ||
| response = requests.get(url, timeout=timeout) | ||
| response.raise_for_status() | ||
| destination.write_bytes(response.content) | ||
| return destination | ||
|
|
||
|
|
||
| def download_document(spec: DocumentSpec, data_dir: Path | None = None) -> Path: | ||
| """Download a document spec to the implementation data directory. | ||
|
|
||
| Notes | ||
| ----- | ||
| PDFs are saved as-is. Notebooks should convert PDFs to plain text before | ||
| paragraph chunking, or provide pre-extracted ``.txt`` files locally. | ||
| """ | ||
| base_dir = data_dir or DOCUMENTS_DIR | ||
| if spec.local_path: | ||
| destination = Path(spec.local_path) | ||
| else: | ||
| destination = base_dir / f"{spec.doc_id}.bin" | ||
|
|
||
| if destination.exists(): | ||
| return destination | ||
|
|
||
| if not spec.source_url: | ||
| msg = f"No source URL configured for document '{spec.doc_id}'." | ||
| raise ValueError(msg) | ||
|
|
||
| return download_url(spec.source_url, destination) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Downloaded content type isn't validated against local_path's extension.
download_document/download_url write raw response bytes to destination unconditionally. For FINANCE_DOCUMENTS (config.py), source_url points at a .pdf while local_path is a .txt file. If this download path is ever exercised (missing pre-extracted .txt), it silently writes binary PDF content to a .txt path; load_document_text's later path.read_text(encoding="utf-8") will then fail with a confusing UnicodeDecodeError disconnected from the real cause.
Consider raising a clear error (or refusing to overwrite) when the URL's apparent content type/suffix doesn't match local_path's suffix, so the failure surfaces immediately and legibly at download time instead of downstream.
🐛 Proposed guard
def download_document(spec: DocumentSpec, data_dir: Path | None = None) -> Path:
...
if not spec.source_url:
msg = f"No source URL configured for document '{spec.doc_id}'."
raise ValueError(msg)
+ source_suffix = Path(spec.source_url.split("?")[0]).suffix.lower()
+ if source_suffix and destination.suffix.lower() != source_suffix:
+ msg = (
+ f"'{spec.doc_id}' source_url looks like '{source_suffix}' but "
+ f"local_path is '{destination.suffix}'. Provide a pre-extracted "
+ f"'{destination.suffix}' file or point local_path at '{source_suffix}'."
+ )
+ raise ValueError(msg)
+
return download_url(spec.source_url, destination)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def download_url(url: str, destination: Path, *, timeout: int = 60) -> Path: | |
| """Download a remote file to a local path.""" | |
| ensure_parent(destination) | |
| response = requests.get(url, timeout=timeout) | |
| response.raise_for_status() | |
| destination.write_bytes(response.content) | |
| return destination | |
| def download_document(spec: DocumentSpec, data_dir: Path | None = None) -> Path: | |
| """Download a document spec to the implementation data directory. | |
| Notes | |
| ----- | |
| PDFs are saved as-is. Notebooks should convert PDFs to plain text before | |
| paragraph chunking, or provide pre-extracted ``.txt`` files locally. | |
| """ | |
| base_dir = data_dir or DOCUMENTS_DIR | |
| if spec.local_path: | |
| destination = Path(spec.local_path) | |
| else: | |
| destination = base_dir / f"{spec.doc_id}.bin" | |
| if destination.exists(): | |
| return destination | |
| if not spec.source_url: | |
| msg = f"No source URL configured for document '{spec.doc_id}'." | |
| raise ValueError(msg) | |
| return download_url(spec.source_url, destination) | |
| def download_url(url: str, destination: Path, *, timeout: int = 60) -> Path: | |
| """Download a remote file to a local path.""" | |
| ensure_parent(destination) | |
| response = requests.get(url, timeout=timeout) | |
| response.raise_for_status() | |
| destination.write_bytes(response.content) | |
| return destination | |
| def download_document(spec: DocumentSpec, data_dir: Path | None = None) -> Path: | |
| """Download a document spec to the implementation data directory. | |
| Notes | |
| ----- | |
| PDFs are saved as-is. Notebooks should convert PDFs to plain text before | |
| paragraph chunking, or provide pre-extracted ``.txt`` files locally. | |
| """ | |
| base_dir = data_dir or DOCUMENTS_DIR | |
| if spec.local_path: | |
| destination = Path(spec.local_path) | |
| else: | |
| destination = base_dir / f"{spec.doc_id}.bin" | |
| if destination.exists(): | |
| return destination | |
| if not spec.source_url: | |
| msg = f"No source URL configured for document '{spec.doc_id}'." | |
| raise ValueError(msg) | |
| source_suffix = Path(spec.source_url.split("?")[0]).suffix.lower() | |
| if source_suffix and destination.suffix.lower() != source_suffix: | |
| msg = ( | |
| f"'{spec.doc_id}' source_url looks like '{source_suffix}' but " | |
| f"local_path is '{destination.suffix}'. Provide a pre-extracted " | |
| f"'{destination.suffix}' file or point local_path at '{source_suffix}'." | |
| ) | |
| raise ValueError(msg) | |
| return download_url(spec.source_url, destination) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 17-17: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(url, timeout=timeout)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/datasets.py` around lines 15 - 45,
Validate the source URL’s apparent suffix against the destination suffix in
download_document/download_url before writing response bytes, especially when a
configured local_path such as a .txt would receive a .pdf. Raise a clear error
on mismatches so invalid downloads are rejected at download time; preserve valid
downloads and existing-file behavior.
| def run_inference( | ||
| client: InferenceClient, | ||
| samples: list[QASample], | ||
| *, | ||
| system: str = DEFAULT_EVAL_SYSTEM, | ||
| ) -> list[dict[str, Any]]: | ||
| """Run the small model on a list of evaluation samples.""" | ||
| predictions: list[dict[str, Any]] = [] | ||
| for sample in samples: | ||
| prompt = build_eval_prompt(sample) | ||
| model_answer = client.complete(prompt, system=system, temperature=0.0) | ||
| predictions.append( | ||
| { | ||
| "id": sample.id, | ||
| "question": sample.question, | ||
| "gold_answer": sample.gold_answer, | ||
| "model_answer": model_answer, | ||
| "failure_mode": ( | ||
| sample.failure_mode.value if sample.failure_mode else None | ||
| ), | ||
| "doc_id": sample.doc_id, | ||
| "para_id": sample.para_id, | ||
| }, | ||
| ) | ||
| return predictions |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No per-sample error isolation around external LLM calls.
run_inference calls client.complete() once per sample in a loop with no exception handling. A single transient failure (timeout, malformed response) aborts the entire evaluation run and discards all previously computed predictions - notable since this is the core loop for baseline/fine-tuned model evaluation over the full test set. Contrast with pipeline.generate_grounded_training_corpus, which already wraps its per-item LLM call in try/except.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/evaluation.py` around lines 45 - 69,
Update run_inference to isolate each client.complete call with per-sample
exception handling, so one failed inference does not abort the loop or discard
earlier predictions. Preserve successful prediction records, and append an
appropriate failure record for samples whose external LLM call raises,
consistent with the existing evaluation result schema and error-handling pattern
in generate_grounded_training_corpus.
| per_device_train_batch_size: int = 1, | ||
| gradient_accumulation_steps: int = 4, | ||
| learning_rate: float = 2e-4, | ||
| max_seq_length: int = 1024, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What trl version is pinned in aieng-synthetic-data/pyproject.toml, and does that version's SFTTrainer/SFTConfig require max_length instead of max_seq_length and processing_class instead of tokenizer?
💡 Result:
The aieng-synthetic-data repository does not have a public, authoritative record of a specific pinned trl version in its pyproject.toml that can be verified as a single canonical version [1]. You should verify the version directly in your local copy of the file (usually under the [tool.poetry.dependencies] or [project.dependencies] section). Regarding your questions about SFTTrainer and SFTConfig API changes: Yes, recent versions of trl have introduced these breaking changes: 1. max_length vs. max_seq_length: SFTConfig uses max_length [2][3]. The parameter max_seq_length has been deprecated and removed in favor of max_length [2]. Using max_seq_length will trigger a warning or error [2]. 2. processing_class vs. tokenizer: In the SFTTrainer constructor, the argument previously named tokenizer has been renamed to processing_class [4]. This change was implemented in trl v0.12.0 to better reflect that the argument can accept either a tokenizer or a processor [4]. Using the deprecated tokenizer argument may result in a TypeError or warning in newer versions of the library [4]. If you are maintaining or updating this project, it is recommended to ensure that SFTConfig uses max_length and that the SFTTrainer is initialized with the processing_class parameter [3][4].
Citations:
- 1: https://huggingface.co/datasets/shakods/synthetic-data/blob/main/pyproject.toml
- 2: https://github.com/huggingface/trl/blob/v0.18.1/trl/trainer/sft_config.py
- 3: https://github.com/sickn33/agentic-awesome-skills/blob/main/skills/hugging-face-model-trainer/references/troubleshooting.md
- 4: https://openillumi.com/en/en-sfttrainer-tokenizer-typeerror/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a '^(sft\.py|pyproject\.toml|poetry\.lock|uv\.lock|requirements.*\.txt)$' . | sed 's#^\./##' | head -100
echo "== pyproject trl deps =="
if [ -f aieng-synthetic-data/pyproject.toml ]; then
rg -n "trl|transformers|peft" aieng-synthetic-data/pyproject.toml
elif [ -f pyproject.toml ]; then
rg -n "trl|transformers|peft" pyproject.toml
fi
echo "== lockfile trl entries =="
for f in aieng-synthetic-data/poetry.lock aieng-synthetic-data/uv.lock poetry.lock uv.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n -A 3 -B 3 '^\[\[package\]\]|name = "trl"|trl' "$f" | head -80; }
done
echo "== target file outline =="
ast-grep outline aieng-synthetic-data/aieng/syn_data/text/sft.py --match 'train_model' --view expanded || true
echo "== relevant target lines =="
cat -n aieng-synthetic-data/aieng/syn_data/text/sft.py | sed -n '1,180p'Repository: VectorInstitute/synthetic-data-bootcamp
Length of output: 413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== aieng-synthetic-data pyproject dependency sections =="
cat -n aieng-synthetic-data/pyproject.toml | sed -n '1,220p' | rg -n "tool|dependencies|extras|trl|transformers|peft|datasets|accelerate|torch|python|project|poetry" -C 3 || true
echo "== root dependency sections =="
cat -n pyproject.toml | sed -n '1,220p' | rg -n "tool|dependencies|extras|trl|transformers|peft|datasets|accelerate|torch|python|project|poetry" -C 3 || true
echo "== lockfile trl/package entries =="
python3 - <<'PY'
from pathlib import Path
import re
for p in [Path("uv.lock")]:
print(f"--- {p}")
s=p.read_text(errors="ignore")
lines=s.splitlines()
for i,l in enumerate(lines):
if re.search(r'^name\s*=\s*["\']trl["\']', l) or l.startswith("[[package]]"):
if 'trl' in l:
lo=max(0,i-5); hi=min(len(lines), i+50)
for n in range(lo,hi):
print(f"{n+1}: {lines[n]}")
print("---")
PY
echo "== target sft.py relevant section =="
cat -n aieng-synthetic-data/aieng/syn_data/text/sft.py | sed -n '1,180p'
echo "== static usage scan for imported names in target file =="
python3 - <<'PY'
from pathlib import Path
p=Path("aieng-synthetic-data/aieng/syn_data/text/sft.py")
text=p.read_text()
for name in ["max_seq_length","pad_token","SFTTrainer","SFTConfig","TrainingArguments","tokenizer"]:
print(name, [(i+1,line.strip()) for i,line in enumerate(text.splitlines()) if name in line])
PYRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 17494
🌐 Web query:
trl 1.6.0 SFTTrainer SFTConfig TrainingArguments max_length processing_class tokenizer
💡 Result:
In TRL 1.6.0, the SFTTrainer is the standard class for Supervised Fine-Tuning, and SFTConfig is the dedicated configuration class for its specific parameters [1][2]. SFTConfig inherits from and complements the standard Transformers TrainingArguments, allowing you to manage SFT-specific settings while retaining full access to standard training configurations [1][2]. Key configuration details regarding your query include: max_length: Defined in SFTConfig, this parameter sets the maximum length of the tokenized sequence, with a default value of 1024 [1][2]. When packing is enabled, this value determines the fixed sequence length [1]. Sequences exceeding this length are truncated based on the truncation_mode [1]. processing_class and tokenizer: In modern TRL versions (including 1.6.0), the SFTTrainer uses a processing_class argument (which accepts a PreTrainedTokenizerBase or ProcessorMixin) to handle data processing [3]. It is strongly recommended to configure your tokenizer directly—specifically setting tokenizer.pad_token if it is not already defined—and passing the tokenizer as the processing_class to the SFTTrainer [4][3][5]. The use of a dedicated pad_token parameter within SFTConfig is deprecated; you should instead ensure the padding token is correctly configured within the tokenizer object itself [4][3][5]. Relationship to TrainingArguments: SFTConfig serves as a wrapper for SFT-specific parameters that works alongside TrainingArguments [1]. While SFTConfig handles SFT tasks (like packing, completion-only loss, and max_length), all other standard training parameters (such as learning_rate, num_train_epochs, or logging_steps) are passed via the TrainingArguments object, which is then provided to the SFTTrainer via its args parameter [1][2].
Citations:
- 1: https://github.com/huggingface/trl/blob/v1.6.0/trl/trainer/sft_config.py
- 2: https://huggingface.co/docs/trl/en/sft_trainer
- 3: https://huggingface.co/docs/trl/main/en/sft%5Ftrainer
- 4: https://github.com/huggingface/trl/blob/v1.3.0/trl/trainer/sft_config.py
- 5: https://github.com/huggingface/trl/blob/v1.8.0/trl/trainer/sft_config.py
Pass the padded tokenizer into SFTTrainer and route max_seq_length through SFTConfig.
max_seq_length is unused, and tokenizer.pad_token is only saved after training; pass it to SFTTrainer as processing_class=tokenizer. Use SFTConfig for the sequence-length setting instead of putting max_seq_length on SFTConfig, unless the configured TRL version still maps that kwarg to its length argument.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/sft.py` at line 59, Update the SFT
training setup around the SFTTrainer construction to pass the padded tokenizer
as processing_class=tokenizer, ensuring padding is available during training.
Route the existing max_seq_length setting through the supported SFTConfig
sequence-length field, or preserve the kwarg only if the configured TRL version
explicitly maps it to that field; remove the currently unused standalone
parameter path.
| settings = LLMSettings( | ||
| api_key=api_key, | ||
| model=( | ||
| os.getenv("SMALL_MODEL_NAME") or os.getenv("OPENAI_MODEL") or default_model | ||
| ), | ||
| base_url=os.getenv("SMALL_MODEL_BASE_URL") | ||
| or os.getenv("OPENAI_BASE_URL", "https://api.openai.com/v1").rstrip("/"), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Trailing slash not stripped from SMALL_MODEL_BASE_URL.
.rstrip("/") only binds to the OPENAI_BASE_URL fallback branch, not to os.getenv("SMALL_MODEL_BASE_URL"), because method calls bind tighter than or. A SMALL_MODEL_BASE_URL with a trailing slash (common for local Ollama-style endpoints) will produce a doubled // before chat/completions in OpenAICompatibleClient.complete.
🐛 Proposed fix
settings = LLMSettings(
api_key=api_key,
model=(
os.getenv("SMALL_MODEL_NAME") or os.getenv("OPENAI_MODEL") or default_model
),
- base_url=os.getenv("SMALL_MODEL_BASE_URL")
- or os.getenv("OPENAI_BASE_URL", "https://api.openai.com/v1").rstrip("/"),
+ base_url=(
+ os.getenv("SMALL_MODEL_BASE_URL")
+ or os.getenv("OPENAI_BASE_URL", "https://api.openai.com/v1")
+ ).rstrip("/"),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settings = LLMSettings( | |
| api_key=api_key, | |
| model=( | |
| os.getenv("SMALL_MODEL_NAME") or os.getenv("OPENAI_MODEL") or default_model | |
| ), | |
| base_url=os.getenv("SMALL_MODEL_BASE_URL") | |
| or os.getenv("OPENAI_BASE_URL", "https://api.openai.com/v1").rstrip("/"), | |
| ) | |
| settings = LLMSettings( | |
| api_key=api_key, | |
| model=( | |
| os.getenv("SMALL_MODEL_NAME") or os.getenv("OPENAI_MODEL") or default_model | |
| ), | |
| base_url=( | |
| os.getenv("SMALL_MODEL_BASE_URL") | |
| or os.getenv("OPENAI_BASE_URL", "https://api.openai.com/v1") | |
| ).rstrip("/"), | |
| ) |
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/small_model.py` around lines 31 -
38, Update the base_url construction in the LLMSettings initialization so the
final value from either SMALL_MODEL_BASE_URL or the OPENAI_BASE_URL fallback is
stripped of trailing slashes. Ensure the .rstrip("/") operation applies to the
entire environment-variable fallback expression before
OpenAICompatibleClient.complete uses it.
| "source": [ | ||
| "# TODO: change judge model to another family of models (e.g claude or gpt-4o)\n", | ||
| "judge = create_judge_client()\n", | ||
| "\n", | ||
| "filtered_samples, judge_scores, rejected = filter_with_judge(\n", | ||
| " judge,\n", | ||
| " kept,\n", | ||
| " threshold=DEFAULT_JUDGE_THRESHOLD,\n", | ||
| ")\n", | ||
| "console.print(\n", | ||
| " f\"[bold green]After judge filter:[/bold green] [yellow]{len(filtered_samples)}[/yellow] kept, \"\n", | ||
| " f\"[red]{len(rejected)}[/red] rejected [dim](heuristics + judge)[/dim]\"\n", | ||
| ")\n", | ||
| "summarize_judge_scores(judge_scores)" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
rejected loses the heuristic rejections, so the quality report under-counts.
filter_with_judge re-runs apply_heuristic_filters internally (aieng-synthetic-data/aieng/syn_data/text/pipeline.py Lines 204-212). Since kept was already filtered at Line 114, the second pass rejects nothing, and this assignment shadows the earlier rejected list — the 8 samples dropped by the first pass vanish. The stored output shows the contradiction: 0 rejected even though input_count=144 and after_heuristics=136.
Pass raw_samples and let filter_with_judge own both stages, or keep separate names and combine them.
🐛 Proposed fix
-filtered_samples, judge_scores, rejected = filter_with_judge(
+filtered_samples, judge_scores, all_rejected = filter_with_judge(
judge,
- kept,
+ raw_samples,
threshold=DEFAULT_JUDGE_THRESHOLD,
)
console.print(
f"[bold green]After judge filter:[/bold green] [yellow]{len(filtered_samples)}[/yellow] kept, "
- f"[red]{len(rejected)}[/red] rejected [dim](heuristics + judge)[/dim]"
+ f"[red]{len(all_rejected)}[/red] rejected [dim](heuristics + judge)[/dim]"
)and in the report cell:
- "rejected": rejected,
+ "rejected": all_rejected,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "source": [ | |
| "# TODO: change judge model to another family of models (e.g claude or gpt-4o)\n", | |
| "judge = create_judge_client()\n", | |
| "\n", | |
| "filtered_samples, judge_scores, rejected = filter_with_judge(\n", | |
| " judge,\n", | |
| " kept,\n", | |
| " threshold=DEFAULT_JUDGE_THRESHOLD,\n", | |
| ")\n", | |
| "console.print(\n", | |
| " f\"[bold green]After judge filter:[/bold green] [yellow]{len(filtered_samples)}[/yellow] kept, \"\n", | |
| " f\"[red]{len(rejected)}[/red] rejected [dim](heuristics + judge)[/dim]\"\n", | |
| ")\n", | |
| "summarize_judge_scores(judge_scores)" | |
| "source": [ | |
| "# TODO: change judge model to another family of models (e.g claude or gpt-4o)\n", | |
| "judge = create_judge_client()\n", | |
| "\n", | |
| "filtered_samples, judge_scores, all_rejected = filter_with_judge(\n", | |
| " judge,\n", | |
| " raw_samples,\n", | |
| " threshold=DEFAULT_JUDGE_THRESHOLD,\n", | |
| ")\n", | |
| "console.print(\n", | |
| " f\"[bold green]After judge filter:[/bold green] [yellow]{len(filtered_samples)}[/yellow] kept, \"\n", | |
| " f\"[red]{len(all_rejected)}[/red] rejected [dim](heuristics + judge)[/dim]\"\n", | |
| ")\n", | |
| "summarize_judge_scores(judge_scores)" |
🤖 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 `@implementations/qa_text_generation/03_quality_filtering.ipynb` around lines
1567 - 1580, Update the quality-filtering flow around filter_with_judge so
heuristic rejections are retained in the final report. Pass raw_samples to
filter_with_judge so it owns both filtering stages, or preserve distinct
heuristic and judge rejection collections and combine them before reporting;
ensure the rejected count includes all samples removed from the original input.
| "MODELS_DIR = ROOT / \"implementations\" / \"qa_text_generation\" / \"models\" / \"lora_adapter\"\n", | ||
| "BASE_MODEL = os.getenv(\"SFT_BASE_MODEL\", \"Qwen/Qwen2.5-3B-Instruct\")\n", | ||
| "RUN_SFT = os.getenv(\"RUN_SFT\", \"1\") == \"1\"\n", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
RUN_SFT defaults to "1", contradicting .env.example and the prerequisites.
.env.example ships RUN_SFT=0 and Line 37 tells participants to set it to 1 explicitly. With this default, anyone running without an .env (or with the variable unset) triggers 4-bit LoRA training and fails on a CPU-only machine. Default to "0".
🐛 Proposed fix
-RUN_SFT = os.getenv("RUN_SFT", "1") == "1"
+RUN_SFT = os.getenv("RUN_SFT", "0") == "1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "MODELS_DIR = ROOT / \"implementations\" / \"qa_text_generation\" / \"models\" / \"lora_adapter\"\n", | |
| "BASE_MODEL = os.getenv(\"SFT_BASE_MODEL\", \"Qwen/Qwen2.5-3B-Instruct\")\n", | |
| "RUN_SFT = os.getenv(\"RUN_SFT\", \"1\") == \"1\"\n", | |
| "MODELS_DIR = ROOT / \"implementations\" / \"qa_text_generation\" / \"models\" / \"lora_adapter\"\n", | |
| "BASE_MODEL = os.getenv(\"SFT_BASE_MODEL\", \"Qwen/Qwen2.5-3B-Instruct\")\n", | |
| "RUN_SFT = os.getenv(\"RUN_SFT\", \"0\") == \"1\"\n", |
🤖 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 `@implementations/qa_text_generation/05_finetune_and_compare.ipynb` around
lines 80 - 82, Update the RUN_SFT default in the notebook configuration to "0"
instead of "1", while preserving the existing environment-variable override and
boolean conversion so training only runs when explicitly enabled.
| text-sft = [ | ||
| "accelerate", | ||
| "bitsandbytes", | ||
| "datasets", | ||
| "peft", | ||
| "torch", | ||
| "transformers", | ||
| "trl", | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C2 'uv sync|pip-audit' .github/workflows/code_checks.yml
sed -n '/^\[dependency-groups\]/,/^\[/p' pyproject.tomlRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 928
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== relevant workflow =="
sed -n '35,70p' .github/workflows/code_checks.yml
echo
echo "== pyproject dependency groups/options =="
sed -n '/^\[project\]/,/^\[tool\.uv\]/p' pyproject.toml | sed -n '1,140p'
echo
echo "== RUN_SFT usages =="
rg -n -C3 'RUN_SFT|text-sft|all-groups|all-extras' .Repository: VectorInstitute/synthetic-data-bootcamp
Length of output: 14590
Add coverage for the optional SFT dependency group.
text-sft is a uv dependency group, while the CI audit uses uv sync --all-extras --dev and audits only .venv/. Users installing with RUN_SFT=1 get extra packages that are not included in the current pip-audit run; add a separate SFT audit or install/audit the group alongside the existing environment.
🤖 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 `@pyproject.toml` around lines 36 - 44, Update the CI dependency-audit workflow
to install the optional text-sft group defined by text-sft before auditing, or
add a separate audit environment for that group. Ensure pip-audit covers the SFT
packages installed when RUN_SFT=1 in addition to the existing .venv
dependencies.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/judge.py`:
- Around line 105-109: Update the INFO logging in the scoring flow to stop
including the complete model answer; retain sample.id and log only the answer’s
length using the existing model_answer value. Keep the surrounding scoring
behavior unchanged.
- Around line 89-92: Update the JSON parsing flow in the surrounding judge
response function to require that the repaired payload is a JSON object, not
merely non-None. Raise a clear ValueError at this parsing boundary for lists,
scalars, or other non-object values, while preserving successful object payloads
for parse_judge_score.
In `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py`:
- Around line 297-302: Update the goal calculation in the pipeline around
target_size and effective_synthetic_target to check whether target_size is None,
preserving an explicit target_size=0 without invoking effective_synthetic_target
or generating LLM requests; use the fallback only when no target was provided.
- Around line 225-233: The exception guards in generate_synthetic_corpus() and
generate_grounded_training_corpus() must also catch requests.HTTPError from
exhausted upstream retries. Update both try/except blocks at the identified
sites to handle the request failure consistently, logging it and applying the
existing paragraph fallback/skip behavior so transient 429/5xx errors do not
abort corpus generation.
In `@aieng-synthetic-data/aieng/syn_data/text/rag.py`:
- Around line 127-130: Update the gold_answer assignment in the back-translation
sample construction to always derive the label from paragraph.text, removing the
payload["gold_answer"] fallback path. Keep the existing stripping behavior and
question handling unchanged.
🪄 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: 8be02122-d2c7-4f46-b9af-d94c497e2fac
📒 Files selected for processing (14)
aieng-synthetic-data/README.mdaieng-synthetic-data/aieng/syn_data/text/__init__.pyaieng-synthetic-data/aieng/syn_data/text/judge.pyaieng-synthetic-data/aieng/syn_data/text/pipeline.pyaieng-synthetic-data/aieng/syn_data/text/prompts.pyaieng-synthetic-data/aieng/syn_data/text/quality.pyaieng-synthetic-data/aieng/syn_data/text/rag.pyaieng-synthetic-data/aieng/syn_data/text/schemas.pyaieng-synthetic-data/tests/test_text_utils.pyimplementations/qa_text_generation/01_baseline_evaluation.ipynbimplementations/qa_text_generation/02_synthetic_qa_generation.ipynbimplementations/qa_text_generation/03_quality_filtering.ipynbimplementations/qa_text_generation/04_grounded_data_augmentation.ipynbimplementations/qa_text_generation/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- aieng-synthetic-data/aieng/syn_data/text/init.py
- aieng-synthetic-data/aieng/syn_data/text/schemas.py
- implementations/qa_text_generation/README.md
- implementations/qa_text_generation/03_quality_filtering.ipynb
- implementations/qa_text_generation/02_synthetic_qa_generation.ipynb
- implementations/qa_text_generation/01_baseline_evaluation.ipynb
| payload = json.loads(repair_json(raw)) | ||
| if payload is None: | ||
| raise ValueError(f"Failed to parse model JSON: {raw}") | ||
| return payload |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the fallback response is a JSON object.
Line 89 accepts valid non-object JSON, unlike LLMClient.complete_json; a list or scalar then reaches parse_judge_score and fails outside the parsing boundary. Reject it here with a clear ValueError.
Proposed fix
payload = json.loads(repair_json(raw))
- if payload is None:
- raise ValueError(f"Failed to parse model JSON: {raw}")
+ if not isinstance(payload, dict):
+ raise ValueError(
+ f"Model response did not contain a JSON object: {raw[:300]!r}"
+ )
return payload📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| payload = json.loads(repair_json(raw)) | |
| if payload is None: | |
| raise ValueError(f"Failed to parse model JSON: {raw}") | |
| return payload | |
| payload = json.loads(repair_json(raw)) | |
| if not isinstance(payload, dict): | |
| raise ValueError( | |
| f"Model response did not contain a JSON object: {raw[:300]!r}" | |
| ) | |
| return payload |
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/judge.py` around lines 89 - 92,
Update the JSON parsing flow in the surrounding judge response function to
require that the repaired payload is a JSON object, not merely non-None. Raise a
clear ValueError at this parsing boundary for lists, scalars, or other
non-object values, while preserving successful object payloads for
parse_judge_score.
| logger.info( | ||
| "Scoring model answer for sample: %s with model answer: %s", | ||
| sample.id, | ||
| model_answer, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log complete model answers at INFO.
Line 108 persists every generated answer in routine logs; deployments adapted to sensitive documents can leak source-derived or user-provided content. Log the sample ID and answer length only.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/judge.py` around lines 105 - 109,
Update the INFO logging in the scoring flow to stop including the complete model
answer; retain sample.id and log only the answer’s length using the existing
model_answer value. Keep the surrounding scoring behavior unchanged.
| try: | ||
| generated = compare_generation_strategies( | ||
| teacher, | ||
| paragraph, | ||
| seed_example=seed, | ||
| few_shot_examples=[seed], | ||
| ) | ||
| except (KeyError, ValueError, TypeError, RuntimeError): | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)pipeline\.py$|(^|/)clients\.py$|(^|/)text/' || true
echo "== pipeline relevant lines =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/pipeline.py ]; then
nl -ba aieng-synthetic-data/aieng/syn_data/text/pipeline.py | sed -n '200,335p'
fi
echo "== clients relevant lines =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/clients.py ]; then
nl -ba aieng-synthetic-data/aieng/syn_data/text/clients.py | sed -n '150,270p'
fi
echo "== imports and exception types in pipeline =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/pipeline.py ]; then
nl -ba aieng-synthetic-data/aieng/syn_data/text/pipeline.py | sed -n '1,80p'
rg -n "except \(KeyError|except \(.*Exception|HTTPError|request" aieng-synthetic-data/aieng/syn_data/text/pipeline.py
fi
echo "== all LLM complete_json call sites =="
rg -n "complete_json|generate_grounded_qa|compare_generation_strategies" -S aieng-synthetic-data/aieng/syn_data/text || trueRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 1140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pipeline relevant lines =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/pipeline.py ]; then
awk '{printf "%7d\t%s\n", NR, $0}' aieng-synthetic-data/aieng/syn_data/text/pipeline.py | sed -n '200,335p'
fi
echo "== clients relevant lines =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/clients.py ]; then
awk '{printf "%7d\t%s\n", NR, $0}' aieng-synthetic-data/aieng/syn_data/text/clients.py | sed -n '150,270p'
fi
echo "== imports and exception types in pipeline =="
if [ -f aieng-synthetic-data/aieng/syn_data/text/pipeline.py ]; then
awk '{printf "%7d\t%s\n", NR, $0}' aieng-synthetic-data/aieng/syn_data/text/pipeline.py | sed -n '1,80p'
rg -n "except \(KeyError|except \(.*Exception|HTTPError|request" aieng-synthetic-data/aieng/syn_data/text/pipeline.py || true
fi
echo "== all targeted call sites =="
rg -n "complete_json|generate_grounded_qa|compare_generation_strategies" -S aieng-synthetic-data/aieng/syn_data/text || trueRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 16810
Handle retried requests.HTTPError in corpus generation.
complete_json() still re-raises non-400/422 requests.HTTPError; the guards in generate_synthetic_corpus() and generate_grounded_training_corpus() only catch parsing/runtime errors, so transient 429/5xx can abort generation after LLMClient.complete() already exhausted retries and logged the failure. Include the upstream request exception type in both guards and add logging/paragraph fallback as needed.
📍 Affects 1 file
aieng-synthetic-data/aieng/syn_data/text/pipeline.py#L225-L233(this comment)aieng-synthetic-data/aieng/syn_data/text/pipeline.py#L317-L320
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py` around lines 225 - 233,
The exception guards in generate_synthetic_corpus() and
generate_grounded_training_corpus() must also catch requests.HTTPError from
exhausted upstream retries. Update both try/except blocks at the identified
sites to handle the request failure consistently, logging it and applying the
existing paragraph fallback/skip behavior so transient 429/5xx errors do not
abort corpus generation.
| goal = target_size or effective_synthetic_target( | ||
| train_paragraphs, | ||
| one_per_paragraph=True, | ||
| ) | ||
| # Avoid repeat visits that regenerate near-identical questions. | ||
| goal = min(goal, len(train_paragraphs)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect an explicit zero target.
target_size or ... treats target_size=0 as unspecified and generates the default corpus instead, causing unwanted LLM calls. Check for None explicitly.
Proposed fix
- goal = target_size or effective_synthetic_target(
- train_paragraphs,
- one_per_paragraph=True,
+ goal = (
+ target_size
+ if target_size is not None
+ else effective_synthetic_target(
+ train_paragraphs,
+ one_per_paragraph=True,
+ )
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| goal = target_size or effective_synthetic_target( | |
| train_paragraphs, | |
| one_per_paragraph=True, | |
| ) | |
| # Avoid repeat visits that regenerate near-identical questions. | |
| goal = min(goal, len(train_paragraphs)) | |
| goal = ( | |
| target_size | |
| if target_size is not None | |
| else effective_synthetic_target( | |
| train_paragraphs, | |
| one_per_paragraph=True, | |
| ) | |
| ) | |
| # Avoid repeat visits that regenerate near-identical questions. | |
| goal = min(goal, len(train_paragraphs)) |
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/pipeline.py` around lines 297 - 302,
Update the goal calculation in the pipeline around target_size and
effective_synthetic_target to check whether target_size is None, preserving an
explicit target_size=0 without invoking effective_synthetic_target or generating
LLM requests; use the fallback only when no target was provided.
| question = str(payload["question"]).strip() | ||
| # Prefer a model-provided short answer when present; otherwise use the | ||
| # passage as the response (standard instruction back-translation). | ||
| gold_answer = str(payload.get("gold_answer") or paragraph.text).strip() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the gold answer fixed to the source passage.
The prompt requests only question, but an unsolicited gold_answer replaces the passage label here. That can admit partial or hallucinated labels; the later 0.15 overlap threshold does not establish entailment. Always use paragraph.text for back-translation samples.
Proposed fix
- # Prefer a model-provided short answer when present; otherwise use the
- # passage as the response (standard instruction back-translation).
- gold_answer = str(payload.get("gold_answer") or paragraph.text).strip()
+ gold_answer = paragraph.text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| question = str(payload["question"]).strip() | |
| # Prefer a model-provided short answer when present; otherwise use the | |
| # passage as the response (standard instruction back-translation). | |
| gold_answer = str(payload.get("gold_answer") or paragraph.text).strip() | |
| question = str(payload["question"]).strip() | |
| gold_answer = paragraph.text |
🤖 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 `@aieng-synthetic-data/aieng/syn_data/text/rag.py` around lines 127 - 130,
Update the gold_answer assignment in the back-translation sample construction to
always derive the label from paragraph.text, removing the payload["gold_answer"]
fallback path. Keep the existing stripping behavior and question handling
unchanged.
2 participants
Summary
This PR is the initial working version of synthetic text generation use case, including the notebooks and the supporting libraries. Further improvements and detailed tests are planned for follow-up PRs.
Clickup Ticket(s): Link(s) if applicable.
Type of Change
Changes Made
Testing
uv run pytest tests/)uv run mypy <src_dir>)uv run ruff check src_dir/)Manual testing details:
Notebooks are running without errors.
Checklist
Summary by CodeRabbit