Skip to content

[REFACTOR] Refactored the core code - #695

Merged
marcvergees merged 4 commits into
development-approach-d-benchmarkfrom
development-approach-d-benchmark-adapted
Sep 30, 2026
Merged

marcvergees merged 4 commits into
development-approach-d-benchmarkfrom
development-approach-d-benchmark-adapted

Conversation

@vharkins1

Copy link
Copy Markdown
Collaborator

Replaces the Approach C/D fill code and the old benchmark with a small form-filling core (app/services/form_filler/) and a flat benchmark that runs directly against it. The goal is a working core to build outward from: this PR wires in the fill path only.

The benchmark for this specific code (qwen2.5:1.5b, 56 ICS narratives): 75.99% average accuracy (52 scored, 4 skipped because the model call failed or its output was cut off).

How filling works now

app/services/form_filler/filler.py fill(pdf_path, narrative, out_path, model):

  1. Schema (template.py, geometry.py): reads the PDF's widgets with pypdf and finds printed tables with pdfplumber, so table rows become JSON arrays instead of dozens of separate fields. Each field's /TU tooltip becomes its description.
  2. Prompt: prompt.txt + the narrative + the schema.
  3. One Ollama call, with format set to the schema so the output is always valid JSON of that shape (temperature 0, seed 42, num_ctx 32768).
  4. Map back: expand_output turns table rows back into widget names.
  5. Write: pypdf fills the PDF.

The app reaches it through FileManipulator.fill_form, so /forms/fill and the Celery fill_form_task both use the new core. Ollama settings come from app/core/config.py; OLLAMA_TIMEOUT is new (default 300s).

Main Changes:

Added

  • app/services/form_filler/: filler.py, template.py, geometry.py, prompt.txt
  • tests/test_form_filler.py: template/table detection, fill with a mocked model, default model, FileManipulator routing
  • pdfplumber in requirements.txt

Benchmark (benchmark/, replaces the old one entirely; see benchmark/README.md)

  • python3 run.py runs all 56 narratives; -d picks a subset, -m another model.
  • Scoring: exact match for short values, word overlap for long ones, tables matched row by row, and an LLM judge (qwen3.5:4b) for right-but-differently-worded answers.
  • Each run writes summary.csv, wrong_fields.txt, a review.pdf per document (wrong fields marked in red, partial in yellow, missing in orange) and run_info.json. That file records the git commit, model digest, options, prompt hash, judge settings and package versions, so a run can be recreated.
  • --baseline publishes a full run's small text results to benchmark/baselines// so they can be committed; results/ is gitignored.
  • Settings live in benchmark/.env (gitignored; copy .env.example), so private server addresses aren't committed.
  • Data renamed to data/{pdfs,narratives,ground_truth}, keyed by widget name.

Docker: use an Ollama outside the container

  • make fireform-native / make up-native start the stack without the Ollama container and pull no model. Point OLLAMA_HOST at the host (http://host.docker.internal:11434) or another server.
  • The ollama service is now behind the bundled-ollama profile. make fireform / make up still include it, so existing behaviour is unchanged.
  • The app and worker no longer depends_on ollama. Documented in docker/.env.example and docs/1. SETUP.md.

Removed

  • Old fill path: app/services/filler.py, llm.py, prompt.txt, approach_d.py, and the scratch inputs/, outputs/, test/ folders
  • tests/test_filler.py (replaced)
  • Old benchmark (datasets, evaluators, pipelines, runners, reports) and .github/workflows/benchmark.yml

Small

  • Unused select import in routes/templates.py; pdf_utils._extract_pdf_fields docstring; .gitignore allows *.env.example and ignores benchmark/results/

Testing

  • pytest tests/test_form_filler.py
  • Full benchmark: 75.99% on qwen2.5:1.5b
  • End-to-end through Docker (make fireform-native, remote Ollama, fresh database), for ICS 205A and ICS 201. Each run went through upload → create template → text input → sync fill → async job → submissions list.

… reworked Template creation and LLM calls to increase output accuracy

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6603d9120

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread benchmark/accuracy.py Outdated
Comment on lines +171 to +172
best = max(scores, default=0.0)
matched = remaining.pop(scores.index(best)) if best > 0 else None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Consume zero-scoring rows before counting extras

When a populated output row has zero similarity to its expected row, this leaves the output row in remaining while also recording the expected row as unmatched. calculate_accuracy then penalizes the same mistake twice—once as a zero-scoring expected row and again as an unsupported extra row. For example, one correct and one wholly incorrect row score 1 / (2 + 1) = 33% rather than 1 / 2 = 50%. Match an available row even when its best score is zero, or exclude rows corresponding to unmatched expectations from the extra-row penalty.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to discuss this @vharkins1. Should we get rid of the benchmark workflow?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we generalize a little bit this prompt removing explicitly the fact saying "You're a FEMA ICS form-filling engine". What would it happen if we export this to another country?


SIGNATURE_TYPE = 6

TYPE_TO_SCHEMA = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's that @vharkins1 ?

Comment thread benchmark/llm_judge.py
from accuracy import _is_blank, match_rows
from config import JUDGE_CONTEXT_TOKENS, JUDGE_HOST, JUDGE_MODEL, JUDGE_TIMEOUT_SECONDS

JUDGE_THRESHOLD = 0.99

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

Comment thread benchmark/llm_judge.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not clear to me if we're batching the whole bunch of cases sent to LLM or not, could you clarify me this?

@marcvergees
marcvergees merged commit 692539e into development-approach-d-benchmark Sep 30, 2026
2 checks passed
@marcvergees
marcvergees deleted the development-approach-d-benchmark-adapted branch September 30, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants