Skip to content

feat: add opt-in ECO annotations to validation reports - #13

Closed
PandaHUN777 wants to merge 1 commit into
NingyuSUN:mainfrom
PandaHUN777:feat/eco-report-annotations-9
Closed

PandaHUN777 wants to merge 1 commit into
NingyuSUN:mainfrom
PandaHUN777:feat/eco-report-annotations-9

Conversation

@PandaHUN777

Copy link
Copy Markdown
Contributor

What and why

Closes #9

Adds opt-in ECO annotations to validation reports without changing the default report contract.

  • validate_record(..., annotate_eco=True) and RecordValidator.validate(..., annotate_eco=True) add evidence_eco_annotations.
  • bioevidence validate --annotate-eco exposes the same behavior in the CLI.
  • Each annotation contains the evidence item id, its extraction method, and the ECO CURIE.
  • ECO mappings are read from the packaged LinkML ExtractionMethod enum with SchemaView; they are not hard-coded in the implementation.
  • Tests cover all extraction methods, the Python opt-in/default behavior, and the CLI flag.
  • Standards/engineering docs and the Unreleased changelog are updated.

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.

  • No change to admission decisions
  • Changes admission decisions (explained above; tests on both sides of the boundary)

Checklist

  • uv run --frozen ruff check ., uv run --frozen mypy and uv run --frozen pytest --cov pass
  • Committed benchmark results regenerated if report contents changed — not applicable: benchmark/default report output is unchanged
  • CHANGELOG.md updated if users will notice
  • New third-party data has its license, attribution and exact version recorded — not applicable: no third-party data added

Full repository CI is left to the upstream pull-request checks; the fork did not start a runner automatically.

@NingyuSUN NingyuSUN left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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

  1. Don't rebuild the mapping on every record. _evidence_eco_annotations constructs a SchemaView on every validate() 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). RecordValidator is 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.

  2. Rebase on main. There is a conflict in CHANGELOG.md: 0.7.0 was released since you opened this. Please put your entry under the new ## Unreleased heading at the top (#16) and leave the 0.7.0 section unchanged.

Optional

  • engine.py now imports linkml_runtime directly; consider declaring linkml-runtime in [project.dependencies] instead of relying on it arriving transitively through linkml.
  • For a schema-invalid record with an unknown extraction_method, eco_curie is null. That seems right; a sentence in docs/STANDARDS.md would make it explicit.
  • CONTRIBUTING asks to match the surrounding style; the codebase favours compact signatures and calls (e.g. the one-line validate_record signature). Not blocking.

Thanks again — looking forward to the update.

@PandaHUN777
PandaHUN777 force-pushed the feat/eco-report-annotations-9 branch from 74cf82d to be4dd1f Compare September 28, 2026 07:42
@PandaHUN777

PandaHUN777 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Updated the pull request to address the review feedback:

  1. Cached _eco_meanings via @functools.cache so SchemaView is constructed once rather than per record validation.
  2. Rebased on current main and resolved the CHANGELOG.md conflict by placing the entry under ## Unreleased while keeping the 0.7.0 release section intact.
  3. Added linkml-runtime to dependencies in pyproject.toml and clarified in docs/STANDARDS.md that unknown extraction methods map eco_curie to null.
  4. Compacted the validate_record signature to match codebase conventions.

Validation: 234 tests passing with 97.56% coverage; ruff check ., mypy src, and git diff --check all clean.

@NingyuSUN NingyuSUN left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 takes grounders.
    • validate() adds report["grounding"] when grounders are configured.
    • validate_record gained grounders: Sequence[Any] = ().

    Please keep both features. Add annotate_eco to validate_record next to grounders, and pass it through to validate(). Append evidence_eco_annotations after the grounding field, still only on request, so reports without either option keep their exact previous shape.

  • cli.py: validate has a new --snapshot-dir option. Add --annotate-eco alongside it.

  • CHANGELOG.md: put your entry under ## Unreleased together 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 in docs/STANDARDS.md stating 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
@PandaHUN777
PandaHUN777 force-pushed the feat/eco-report-annotations-9 branch from be4dd1f to 2ec6a10 Compare September 30, 2026 13:59
@NingyuSUN

Copy link
Copy Markdown
Owner

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 engine.py, cli.py and the validation reports, and opt-in ECO annotations are no longer something we plan to add. Rather than ask you to rebase onto a target that has moved this much, I'm closing this PR.

Your other two contributions, Python 3.14 support (#12) and the dataset-label draft examples (#14), are part of the 0.8.0 release. Thank you for both. If you'd like to contribute again, issues labelled good first issue are a good place to start.

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.

Opt-in ECO codes for evidence items in validation reports

2 participants