awslabs / awslabs/synthetically_engineered_evaluation_data
Structured quality gate can't fail on duplicate keys, and reports success after exhausting retries
- Dominant language
- Python
- Stars
- 9
- Forks
- 1
- PR merge metrics
- No merged PRs in 30d
Description
## Context
Split out of the review of #15 (see the plan doc `docs/planning/PR15_REVIEW_RESPONSE.md` §6). #15 fixes the validator gaps it found; this issue covers a class of problem the review surfaced that is **not** a validator bug and shouldn't be fixed under a hardening PR: places where the structured-generation quality gate is *structurally unable to fail*, so a bad dataset is reported as a passing one.
Each item below is verified against `main` + #15, with the arithmetic shown.
## 1. The uniqueness penalty is capped below the failure threshold
`src/seed_data/evaluation/structural.py:156`:
```python
uniqueness_penalty = min(total_unique_violations * 0.05, 0.5)
results["overall_score"] = (
0.4 * results["referential_integrity"] + 0.4 * avg_type + 0.2 * (1.0 - uniqueness_penalty)
)
```
The penalty caps at `0.5` and is weighted `0.2`, so it can subtract at most **0.1** from the structural score. With referential integrity and type conformance perfect:
| duplicate values | penalty | structural score | vs `structural: 0.7` |
|---|---|---|---|
| 0 | 0.00 | 1.00 | PASS |
| 5 | 0.25 | 0.95 | PASS |
| ≥10 | 0.50 (capped) | **0.90** | **PASS** |
A table whose "unique" primary key is the *same value in every row* scores 0.90 against a 0.70 threshold. The score cannot reach 0.70 on duplicates alone — it would need referential integrity or type conformance to fail as well.
#15 makes this much less likely to bite in practice: `Validator._validate_uniqueness` now emits `uniqueness_violation`, the corrector repairs it, and unrepaired records get filtered — so duplicates are usually gone before scoring. But the *scorer* is still unable to fail on them, which matters for any path that reaches it with duplicates intact.
**Options** (not a recommendation — worth a design discussion):
- Make duplicates in a `unique=True` field a hard gate failure rather than a weighted penalty; uniqueness is boolean, unlike the continuous metrics it's averaged with.
- Or scale the penalty by the *fraction* of the column duplicated instead of a raw count with a cap, so a fully-duplicated key column drives the term to 0.
## 2. Exhausting retries reports success
`src/seed_data/structured/pipeline.py:294-298`:
```python
else:
# Accept what we have — max revisions exhausted
ps.quality_passed = True
ps.gen_result_json = json.dumps({"data": data})
logger.info("Max revisions exhausted — accepting best result")
return json.dumps({..., "note": "accepted after max revisions"})
```
After `MAX_GENERATION_RETRIES` (3) × `MAX_SCHEMA_REVISIONS` (2) are spent, the gate flips `quality_passed = True` for data that **just failed it**. Accepting the best available result is a defensible choice — silently relabelling it as passing is the problem:
- The `"note": "accepted after max revisions"` is returned but never read — nothing downstream consumes it (`grep -rn '"note"' src/` matches only the line that writes it).
- `StructuredResult` has `success: bool` and `evaluation: dict[str, float]`, but no field distinguishing "passed the gate" from "accepted after exhausting retries". A caller checking `result.success` cannot tell the difference.
- The distinction survives only as a `logger.info("Max revisions exhausted — accepting best result")`, which is invisible at default log levels. The CLI prints `Quality: {scores}` either way, with no indication the scores shown are ones that failed.
**Suggested fix:** carry the distinction onto `StructuredResult` (e.g. `quality_passed: bool` separate from `success`, or a `warnings: list[str]`) and print it. Programmatic consumers gating on data quality currently have nothing to gate on.
## 3. Row count — what remains after #15
#15 added a `completeness` dimension to `QUALITY_THRESHOLDS` at `0.75` of target, scored by `completeness_score(row_count, target_count)` and gated per-dimension. That closes the original "a quarter of the requested rows reported PASS" hole: a 100-row request now fails the gate below 75 rows.
Two residuals:
- **The 0.75 floor is deliberate slack.** A 100-row request satisfied with 75 rows passes. That's intentional (the LLM routinely lands a few rows under target, and an exact-count demand would burn every retry on a fine dataset), but it means "success" can still mean a 25% shortfall. `row_counts` is on `StructuredResult`, so a caller *can* check — worth documenting that they should.
- **It interacts with item 2.** A run that fails `completeness` through every retry is accepted and reported as passing, so the floor can be bypassed entirely by exhaustion.
## Acceptance criteria
- [ ] Duplicates in a `unique=True` field can fail the quality gate on their own.
- [ ] "Accepted after exhausting retries" is distinguishable from "passed the gate" — on `StructuredResult` and in printed output.
- [ ] The `completeness` slack (0.75) is documented as a deliberate tolerance, with `row_counts` named as the exact check.
- [ ] Tests: a fully-duplicated key column fails; an exhausted run is not reported as a clean pass.
## Not in scope
Validator/corrector correctness (`unique` scanning, non-integral integers, `pattern` enforcement, `re.fullmatch`) — all fixed in #15.
Contributor guide
Research direction
Start with src/seed_data/evaluation/structural.py:156 and src/seed_data/structured/pipeline.py:294-298, then trace StructuredResult, QUALITY_THRESHOLDS, and the CLI quality output. Review the existing quality-gate tests before choosing how uniqueness and exhausted retries should be represented. Done means duplicated unique fields can fail independently, exhausted retries are distinct from a clean pass, and the 0.75 completeness tolerance is documented with row_counts as the exact check.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100