feat(orchestration): allow quarantine_max_ratio tolerance in readiness gate - #48
Conversation
…s gate (closes #36) Dataset contracts gain an optional quarantine_max_ratio knob (default 0.0, range [0.0, 1.0]). _validate_dataset_readiness now raises only when the schema is invalid OR quarantine_ratio exceeds the threshold, so real-world feeds with known low-rate quirks (e.g. NYC TLC's ~2.2% true business-key duplicates) no longer force operators to pre-clean files, widen business keys, or drop the key entirely. Schema-level violations still fail closed regardless of the threshold. Intake evidence records quarantine_max_ratio and tolerance_applied so audits can see the gate decision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 53 |
| Duplication | 7 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
| if isinstance(raw, bool) or not isinstance(raw, (int, float)): | ||
| raise ConfigError(f"contract.quarantine_max_ratio must be a number in [0.0, 1.0] ({context}); got {raw!r}") | ||
| value = float(raw) | ||
| if value != value or value in (float("inf"), float("-inf")): # NaN or inf |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
The pull request introduces the quarantine_max_ratio parameter to allow configurable tolerance for quarantine violations during readiness checks. While the core logic aligns with the requirements, the implementation is currently not up to standards according to Codacy analysis. The most significant issue is the non-idiomatic numeric validation in src/driftsentinel/config/loader.py, which is flagged by both Pylint and Semgrep. Additionally, the loader has seen a significant increase in complexity. There are also minor opportunities for code cleanup in the orchestration runner regarding redundant casting and string formatting. Addressing the static analysis findings in the config loader is required to meet quality standards.
About this PR
- Codacy analysis indicates this PR is not up to standards. This is primarily driven by the new static analysis issues in the config loader and significant complexity increases in
loader.py(+9) andrunner.py(+7).
Test suggestions
- Verify default (0.0) ratio blocks any quarantined rows (backward compatibility)
- Verify ratio below threshold passes with tolerance_applied set to True
- Verify ratio above threshold fails with measured ratio in error message
- Verify schema violation fails even if threshold is 1.0 (strict schema invariant)
- Verify loader rejects invalid types (booleans, strings) and out-of-range numbers
- Verify intake evidence artifact contains the new decision fields
- Verify quarantine_ratio rounding/fractional logic is deterministic for the gate
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if isinstance(raw, bool) or not isinstance(raw, (int, float)): | ||
| raise ConfigError(f"contract.quarantine_max_ratio must be a number in [0.0, 1.0] ({context}); got {raw!r}") | ||
| value = float(raw) | ||
| if value != value or value in (float("inf"), float("-inf")): # NaN or inf |
There was a problem hiding this comment.
🔴 HIGH RISK
Suggestion: Use math.isfinite() to check for NaN and infinity values. This replaces the manual value != value comparison and the explicit check against float('inf') with a single, clear standard library call that returns False for both NaN and infinite values.
Try running the following prompt in your IDE agent:
In
src/driftsentinel/config/loader.py, refactor the NaN and infinity check on line 113 to usemath.isfinite(value). Ensure themathmodule is imported at the top of the file.
| f"{dataset_label} does not satisfy the registered contract. " | ||
| f"Fix the {dataset_label.lower()} before running drift or benchmark. " | ||
| + "; ".join(details) | ||
| f"Fix the {dataset_label.lower()} before running drift or benchmark. " + "; ".join(details) |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Simplify string concatenation by moving the joined list into the f-string.
| details.append(f"quarantine_ratio={float(evaluation['quarantine_ratio']):.4f}") | ||
| details.append(f"quarantine_max_ratio={float(evaluation['quarantine_max_ratio']):.4f}") |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Remove redundant float() casts as these values are already guaranteed to be floats.
95b9002
into
main
…37 closed - #36 closed via PR #48 (commit 95b9002) - #37 closed via PR #49 (commit b4d5b82) - Reconciled project Status field for #35 (In Progress → Done) - No documentation drift fixed; no iteration carry-over - Validation: lint, typecheck, 491 tests pass; bundle SKIPPED (no Databricks auth) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #36.
Summary
quarantine_max_ratioknob (default0.0, range[0.0, 1.0]) — backward-compatible zero-tolerance behavior preserved when the key is absent._validate_dataset_readinessraises only when the schema is invalid orquarantine_ratio > quarantine_max_ratio. Schema violations still fail closed regardless of the threshold.quarantine_max_ratioandtolerance_appliedso audits see the gate decision;quarantine_ratioretains its current semantics.run_dataset_intake's PASS/FAIL verdict honors the same threshold so the intake artifact and the drift/benchmark gate agree._validate_quarantine_max_ratio) rejects out-of-range, non-numeric, boolean, and non-finite values at load time with aConfigErrornaming the key.Why
NYC TLC iter-1 demo evidence: 66,651 of 3 M rows duplicated on the 4-col business key (~2.2%) — a real-world floor, not a defect. The previous zero-tolerance gate forced operators to either pre-clean parquet (silently deleting evidence), widen the business key to 8 columns (distorting the contract), or drop the key entirely (disabling the check). The single-knob ratio resolves all three.
Plan
specs/DS-PATCH-036_quarantine_max_ratio_tolerance.md— full design, validation, and acceptance criteria.specs/DS-TM-001_Traceability_Matrix.md— DS-FR-005 / DS-SR-002 row links DS-PATCH-036; v1.4 changelog entry.Test plan
uv run ruff check .— All checks passeduv run mypy src/driftsentinel tests— Success: no issues found in 60 source filesuv run pytest— 487 passedtests/test_orchestration.py::TestValidateDatasetReadinesscovers all four ratio cases (0/0,0/>0,5%/3%,5%/7%) plus the schema-violation invariant and thetolerance_appliedflag-state matrix.tests/test_dataset_orchestration.py::TestIntakeToleranceEvidenceasserts the new fields land in the written intake envelope.tests/test_config_loading.pycases cover the loader-boundary rejections (out-of-range, non-numeric, boolean, negative) and the absence-of-key default.tests/test_intake.pygains a known-fraction test lockingquarantine_ratiorounding semantics relied on by the new gate.🤖 Generated with Claude Code