Skip to content

fix(annotation): topology-vs-manifest guard + bounded fraction at all scopes#230

Closed
henrycgbaker wants to merge 1 commit into
feat/annotation-per-item-calibration/05-capfrom
feat/annotation-per-item-calibration/06-audit-fixes
Closed

fix(annotation): topology-vs-manifest guard + bounded fraction at all scopes#230
henrycgbaker wants to merge 1 commit into
feat/annotation-per-item-calibration/05-capfrom
feat/annotation-per-item-calibration/06-audit-fixes

Conversation

@henrycgbaker

@henrycgbaker henrycgbaker commented May 27, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to stack: #226#227#228#229 (companion to standalone IAA fix #225)

This PR: Two small hardening fixes uncovered by audit on the per-item partition stack, plus per-chunk integration coverage.


Scope

  • core/annotation/record_builder.py: fan_out_records now raises RuntimeError when the partition manifest assigns records to a task absent from the current workspaces topology. Previously it warned and continued, silently dropping data on disk if an operator reshaped their workspaces between imports.
  • core/settings/annotation_settings.py: workspace- and task-scoped calibration_fraction now constrained to [0.0, 1.0] via a shared CalibrationFraction = Annotated[float, Field(ge=0.0, le=1.0)] alias. The deployment-scope guard already had this range check; the new alias closes the gap so a YAML setting of workspaces.x.calibration_fraction: 1.5 is rejected at load.
  • api/annotation_import.py: clarify the manifest-write-ordering comment - a retry under a binding cap is order-dependent (the cap-vs-existing logic is documented in test_cap_under_split_imports_is_order_dependent_by_design).
  • Tests: test_records_for_task_absent_from_topology_raises replaces the prior warn-and-skip test. Locale-mismatch test gets a complete 3-task topology so the topology guard isn't tripped incidentally.
  • Integration: test_per_chunk_retrieval_routing_to_argilla verifies per-chunk independence end-to-end against a live Argilla dataset (assignments in the manifest match the chunk_ids actually present in each retrieval dataset).

Why

  1. The topology-vs-manifest path was silent data loss. If a user removed a task from their workspace topology while old manifest entries for that task existed, fan_out_records quietly skipped them with a logger.warning. Promoting to RuntimeError forces the operator to address the inconsistency (either re-add the task or clear the affected partition manifest).

  2. The deployment-level calibration_fraction field had Field(ge=0.0, le=1.0), but the workspace and task variants were typed float | Inherit with no range check. A YAML setting of workspaces.x.calibration_fraction: 1.5 validated, then resolved to a value >= 1.0, then routed 100% to calibration. The bounded alias closes that gap consistently across scopes.

Test plan

  • uv run python -m pytest tests/unit (770 passing)
  • uv run python -m pytest tests/integration/test_annotation_calibration.py -m "integration and annotation" (8/9 passing; 1 pre-existing failure in TestPerWorkspaceMinSubmitted::test_workspace_carve_out_creates_correct_dataset_min_submitted exists on main already)

@github-actions github-actions Bot added feature Adds or expands user-facing functionality size: S 50-199 LOC labels May 27, 2026
@henrycgbaker henrycgbaker added annotation Changes affecting the annotation tool api Changes affecting the public Python API or user-facing function signatures config Changes user-facing settings, defaults, configuration resolution, or runtime behavior integration Changes affecting integration tests or cross-component test coverage labels May 27, 2026
@henrycgbaker henrycgbaker marked this pull request as draft May 28, 2026 07:31
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/06-audit-fixes branch from 11fc689 to 0b91eb0 Compare June 4, 2026 17:06
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/05-cap branch from 6582de7 to 56b4439 Compare June 4, 2026 19:35
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/06-audit-fixes branch from fbe4222 to 9ebbf36 Compare June 4, 2026 19:35
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/05-cap branch from 56b4439 to 4a4f5fd Compare June 17, 2026 09:33
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/06-audit-fixes branch from 9ebbf36 to f8f4b3f Compare June 17, 2026 09:35
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/05-cap branch 2 times, most recently from 8161e15 to be8fa2e Compare June 19, 2026 21:13
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/05-cap branch from be8fa2e to 7b1572b Compare July 3, 2026 12:52
… scopes

Two small hardening fixes uncovered by audit on the per-item partition
feature stack:

- core/annotation/record_builder: ``fan_out_records`` now raises
  ``RuntimeError`` when the partition manifest assigns records to a task
  absent from the current workspaces topology. Previously it warned and
  continued, silently dropping data on disk if an operator reshaped their
  workspaces between imports.
- core/settings/annotation_settings: workspace- and task-scoped
  ``calibration_fraction`` now constrained to [0.0, 1.0] via a shared
  ``CalibrationFraction = Annotated[float, Field(ge=0.0, le=1.0)]`` alias.
  The deployment-scope guard already had this range check; the new alias
  closes the gap so a YAML setting of `workspaces.x.calibration_fraction:
  1.5` is rejected at load.
- api/annotation_import: clarify the manifest-write-ordering comment - a
  retry under a binding cap is order-dependent (the cap-vs-existing logic
  is documented in ``test_cap_under_split_imports_is_order_dependent_by_design``).
- tests: ``test_records_for_task_absent_from_topology_raises`` replaces
  the prior warn-and-skip test. Locale-mismatch test gets a complete
  3-task topology so the topology guard isn't tripped incidentally.
- Integration: ``test_per_chunk_retrieval_routing_to_argilla`` verifies
  per-chunk independence end-to-end against a live Argilla dataset.
@henrycgbaker henrycgbaker force-pushed the feat/annotation-per-item-calibration/06-audit-fixes branch from f8f4b3f to db6c94a Compare July 3, 2026 13:02
@henrycgbaker

Copy link
Copy Markdown
Collaborator Author

Folded into #229 — the audit-fix commit is now part of that PR's history (cherry-picked cleanly onto the same tip, verified via diff audit + full test suite).

@henrycgbaker henrycgbaker deleted the feat/annotation-per-item-calibration/06-audit-fixes branch July 3, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

annotation Changes affecting the annotation tool api Changes affecting the public Python API or user-facing function signatures config Changes user-facing settings, defaults, configuration resolution, or runtime behavior feature Adds or expands user-facing functionality integration Changes affecting integration tests or cross-component test coverage size: S 50-199 LOC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant