Skip to content

feat: replace MeaningUnitAspect enum with open-ended str - #206

Open
asteier2026 wants to merge 2 commits into
mainfrom
asteier2026/feature/meaning-unit-aspect-str
Open

feat: replace MeaningUnitAspect enum with open-ended str#206
asteier2026 wants to merge 2 commits into
mainfrom
asteier2026/feature/meaning-unit-aspect-str

Conversation

@asteier2026

@asteier2026 asteier2026 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Drops the MeaningUnitAspect enum from EntityDispositionSchema and schemas/__init__.py
  • Changes MeaningUnitSchema.aspect from MeaningUnitAspect to str
  • Updates tests to use string literals instead of enum values

Motivation

The enum was causing record loss: the model returns open-ended aspect values like "goal", "symptom", "diagnosis" that can't be exhaustively enumerated. Validation errors were silently dropping records.

@asteier2026
asteier2026 requested a review from a team as a code owner July 2, 2026 15:51
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the MeaningUnitAspect enum and replaces the aspect field with an open-ended str = Field(min_length=1) on both MeaningUnitSchema and QualityQAItemSchema, preventing silent record loss when the LLM returns aspect values outside a fixed vocabulary.

  • rewrite.py: Drops the 16-value MeaningUnitAspect enum; aspect on MeaningUnitSchema becomes str = Field(min_length=1). QualityQAItemSchema.aspect also gains the min_length=1 guard as a bonus hardening.
  • __init__.py: MeaningUnitAspect removed from the import and __all__ export list, keeping the public API consistent.
  • test_qa_generation.py: Stub fixtures updated from enum references to plain string literals with no logic changes.

Confidence Score: 5/5

  • This is a safe, well-scoped change that directly fixes silent record loss by relaxing an overly restrictive enum constraint.
  • The change is minimal and targeted: removing a fixed-vocabulary enum and replacing it with a validated open string. Both affected fields now carry min_length=1 guards, the public API export is kept consistent, and tests are updated to match. No logic changes, no new code paths introduced.
  • No files require special attention.

Important Files Changed

Filename Overview
src/anonymizer/engine/schemas/rewrite.py Removes MeaningUnitAspect enum and replaces aspect field with str = Field(min_length=1) on both MeaningUnitSchema and QualityQAItemSchema. No issues found.
src/anonymizer/engine/schemas/init.py Removes MeaningUnitAspect from the import and __all__ list, consistent with the class removal in rewrite.py.
tests/engine/test_qa_generation.py Removes MeaningUnitAspect import and replaces enum references with plain string literals in stub fixtures. Straightforward update with no issues.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class MeaningUnitSchema {
        +int id
        +str aspect
        +str unit
        +MeaningUnitImportance importance
    }
    class QualityQAItemSchema {
        +int id
        +str aspect
        +MeaningUnitImportance importance
        +str question
        +str reference_answer
    }
    class MeaningUnitImportance {
        <<enumeration>>
        critical
        important
    }
    note for MeaningUnitSchema "aspect: str (min_length=1)\nreplaces MeaningUnitAspect enum"
    note for QualityQAItemSchema "aspect: str (min_length=1)\nalso hardened in this PR"
    MeaningUnitSchema --> MeaningUnitImportance
    QualityQAItemSchema --> MeaningUnitImportance
Loading

Reviews (4): Last reviewed commit: "fix: add min_length=1 to MeaningUnitSche..." | Re-trigger Greptile

Comment thread src/anonymizer/engine/schemas/rewrite.py Outdated
asteier2026 and others added 2 commits August 6, 2026 19:24
The enum was causing record loss when the model returned values like
"goal" or "symptom" that weren't enumerated. Aspect is now an
unvalidated str to accommodate open-ended LLM output.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
…hema.aspect

Empty strings are structurally valid but semantically meaningless; guard
against them consistently with the other non-id string fields in these schemas.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
@lipikaramaswamy
lipikaramaswamy force-pushed the asteier2026/feature/meaning-unit-aspect-str branch from 212a478 to 373f2b6 Compare August 6, 2026 18:25
MeaningUnitSchema(
id=1,
aspect=MeaningUnitAspect.ROLE,
aspect="role",

@lipikaramaswamy lipikaramaswamy Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: could we add a regression test using an aspect outside the former enum, such as "diagnosis" or "goal"? The current "role" and "environment" fixtures were valid under the old enum, so they don’t directly verify the behavior this PR fixes.

@lipikaramaswamy lipikaramaswamy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM on the code and tests (left a small nit), plus internal benchmark results support merging.

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