Repository navigation
feat: add opt-in ECO annotations to validation reports - #13
PandaHUN777 wants to merge 1 commit into
Conversation
NingyuSUN
left a comment
There was a problem hiding this comment.
Thanks for taking on #9 — the essential requirement is met: the flag defaults to off and the annotations are appended only after findings and decisions are computed, so default reports stay unchanged; the mapping is read from the schema, not hard-coded; and both paths are tested. CI passes on all jobs.
Requested changes
-
Don't rebuild the mapping on every record.
_evidence_eco_annotationsconstructs aSchemaViewon everyvalidate()call. On my machine that is ~36 ms per call, versus ~6 ms to validate a typical valid record, so annotated validation is roughly 6× slower per record (about 3 extra minutes on the 5,026-record ClinVar case).RecordValidatoris designed to load configuration once per batch (see its docstring). A cached helper keeps that design and costs nothing when the flag is off:@functools.cache def _eco_meanings(schema_path: str) -> dict[str, str]: enum = SchemaView(schema_path).get_enum("ExtractionMethod") ... return {name: str(v.meaning) for name, v in enum.permissible_values.items() if v.meaning}
and call
_eco_meanings(str(default_schema_path()))from_evidence_eco_annotations. -
Rebase on
main. There is a conflict inCHANGELOG.md: 0.7.0 was released since you opened this. Please put your entry under the new## Unreleasedheading at the top (#16) and leave the 0.7.0 section unchanged.
Optional
engine.pynow importslinkml_runtimedirectly; consider declaringlinkml-runtimein[project.dependencies]instead of relying on it arriving transitively throughlinkml.- For a schema-invalid record with an unknown
extraction_method,eco_curieisnull. That seems right; a sentence indocs/STANDARDS.mdwould make it explicit. - CONTRIBUTING asks to match the surrounding style; the codebase favours compact signatures and calls (e.g. the one-line
validate_recordsignature). Not blocking.
Thanks again — looking forward to the update.
74cf82d to
be4dd1f
Compare
|
Updated the pull request to address the review feedback:
Validation: 234 tests passing with 97.56% coverage; |
NingyuSUN
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. The cache, the linkml-runtime declaration and the STANDARDS note all look good. I checked the branch locally: ruff, mypy, pytest (97.56% coverage), the wheel build and the installed-wheel smoke check pass.
Two things before this can merge:
1. uv.lock is out of date. pyproject.toml now declares linkml-runtime, but the lock file wasn't regenerated, so uv lock --check fails. CI doesn't notice because every step runs with --frozen. Please run uv lock and commit the result. It only adds two lines: linkml-runtime under the project's dependencies and requires-dist. The package is already locked as a transitive dependency.
2. Rebase on main. The AI-validation work (#27–#34) has just landed and touches the same code. engine.py, cli.py and CHANGELOG.md will conflict.
-
engine.py:RecordValidator.__init__now takesgrounders.validate()addsreport["grounding"]when grounders are configured.validate_recordgainedgrounders: Sequence[Any] = ().
Please keep both features. Add
annotate_ecotovalidate_recordnext togrounders, and pass it through tovalidate(). Appendevidence_eco_annotationsafter thegroundingfield, still only on request, so reports without either option keep their exact previous shape. -
cli.py:validatehas a new--snapshot-diroption. Add--annotate-ecoalongside it. -
CHANGELOG.md: put your entry under## Unreleasedtogether with the new entries.
Optional:
- ECO meanings always come from the packaged schema, even with
--schema custom.yaml, whose hash the report records as the schema used. That matches what the docs say. One sentence indocs/STANDARDS.mdstating it would remove any doubt. - In
CHANGELOG.md, one bullet is enough: "Default validation behavior and report contents remain unchanged" can join the first. There is also an extra blank line before## 0.7.0. - Not blocking: the codebase keeps calls compact. For example,
validate.add_argument("--annotate-eco", action="store_true", help="...")fits on one or two lines, and the same goes for the new test calls.
With the lock file and the rebase in, this looks ready to merge.
- Add opt-in evidence_eco_annotations to validation reports via --annotate-eco and annotate_eco=True - Derive ECO CURIEs from packaged LinkML ExtractionMethod permissible values - Cache SchemaView extraction method lookups via functools.cache - Declare explicit linkml-runtime dependency in pyproject.toml and uv.lock - Preserve upstream grounding report structure and unannotated report shape
be4dd1f to
2ec6a10
Compare
|
Thanks again for this, @PandaHUN777, and for working through the earlier review so quickly. The project has moved on since then. The AI-validation work (#27–#40, now released as 0.8.0) reshaped Your other two contributions, Python 3.14 support (#12) and the |
What and why
Closes #9
Adds opt-in ECO annotations to validation reports without changing the default report contract.
validate_record(..., annotate_eco=True)andRecordValidator.validate(..., annotate_eco=True)addevidence_eco_annotations.bioevidence validate --annotate-ecoexposes the same behavior in the CLI.ExtractionMethodenum withSchemaView; they are not hard-coded in the implementation.Admission boundary
This does not change validation, admission, review, or rejection decisions. ECO annotations are appended only after the existing findings and decisions have been computed, and the option defaults to off. Existing default reports therefore keep the same shape.
Checklist
uv run --frozen ruff check .,uv run --frozen mypyanduv run --frozen pytest --covpassCHANGELOG.mdupdated if users will noticeFull repository CI is left to the upstream pull-request checks; the fork did not start a runner automatically.