fix(annotation): topology-vs-manifest guard + bounded fraction at all scopes#230
Closed
henrycgbaker wants to merge 1 commit into
Conversation
1 task
11fc689 to
0b91eb0
Compare
6582de7 to
56b4439
Compare
fbe4222 to
9ebbf36
Compare
56b4439 to
4a4f5fd
Compare
9ebbf36 to
f8f4b3f
Compare
8161e15 to
be8fa2e
Compare
be8fa2e to
7b1572b
Compare
… 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.
f8f4b3f to
db6c94a
Compare
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). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_recordsnow raisesRuntimeErrorwhen 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-scopedcalibration_fractionnow constrained to[0.0, 1.0]via a sharedCalibrationFraction = 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 ofworkspaces.x.calibration_fraction: 1.5is 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 intest_cap_under_split_imports_is_order_dependent_by_design).test_records_for_task_absent_from_topology_raisesreplaces the prior warn-and-skip test. Locale-mismatch test gets a complete 3-task topology so the topology guard isn't tripped incidentally.test_per_chunk_retrieval_routing_to_argillaverifies 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
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_recordsquietly skipped them with alogger.warning. Promoting toRuntimeErrorforces the operator to address the inconsistency (either re-add the task or clear the affected partition manifest).The deployment-level
calibration_fractionfield hadField(ge=0.0, le=1.0), but the workspace and task variants were typedfloat | Inheritwith no range check. A YAML setting ofworkspaces.x.calibration_fraction: 1.5validated, 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 inTestPerWorkspaceMinSubmitted::test_workspace_carve_out_creates_correct_dataset_min_submittedexists onmainalready)