fix(dml): fail closed when cross-fit learners cannot be isolated

[shawcharles]

Fixes #814.

Summary

• A one-copy validation preflight rejects custom learner templates whose deepcopy fails, returns the original object, or loses the required learner protocol. A preflight failure raises before any group-time cell is fitted.
• Per-fold cloning stays lazy. Later clone failures propagate as sanitised TypeError, never reuse the template and never become a NaN-cell skip. Custom __deepcopy__ implementations remain responsible for isolating nested mutable state.
• Preserve feat(dml): Caetano, Callaway, Payne & Sant'Anna (2026) bad-control lane on DMLDiD (PR-B) #829's iter_fold_fits architecture and expanded bad-control validation. Cover ordinary panel/RCS fits and nested bad-control stages; failed re-fits clear prior fitted results.
• Add scikit-learn to development and both test-workflow dependency lists, retaining optional test guards. A separate follow-up commit preserves the stateless omitted-parameter sentinel's identity under shallow/deep copying, fixing the newly enabled StackedDiD clone test without changing alias resolution or deprecation warnings.
• Update API/methodology documentation, tutorial prose and changelog fragments. No estimator formulas or variance calculations change.

Verification

Rebased onto abcd785a. Current head: ac1ef63c.

• Python 3.13 focused cross-fit, DML, estimator/sentinel, survey, methodology, bad-control parity, documentation and changelog tests: 1137 passed, 13 skipped, 5 deselected.
• Python 3.9 estimator/sentinel, survey and methodology checks: 790 passed, 13 skipped, 5 deselected.
• Pre-merge scanner: zero pattern findings; its resolved test set passed (807 passed, 13 skipped, 5 deselected, overlapping the checks above).
• Package/test Ruff and Black, whole-package mypy, changelog validation, TOML/YAML/notebook parsing and git diff --check passed.
• Tutorial 32's executable cells and stored outputs are unchanged; its changes are markdown-only.
• Final correction of two stale fallback-warning descriptions: 367 documentation/changelog tests passed; runtime code is unchanged from the tested rebase.
• The prior broad pure-Python run at dfe900ea, before that final documentation-only correction, recorded 14542 passed, 256 skipped, one failed. The failure is test_allowlists_are_reachable, reporting stale DMLDiDResults.groups and DMLDiDResults.overall_att allowlist entries. It was rerun on both the rebased branch and unchanged main and fails identically; left outside this PR's scope.

Not run in this verification pass: the full suite, Rust backend matrix, Sphinx build or notebook execution. The repository's default slow-test exclusion remains in effect. Mirror #817 needs refreshing to evaluate the rebased head in hosted CI.

[shawcharles]

The agreed copy-isolation boundary, preflight placement, error sanitisation, documentation surfaces, changelog fragment, and regression cases are now in place. Local validation covers the DML/cross-fit and documentation suites, targeted Python 3.9 execution, static checks, and native built-in output parity. Could you apply ready-for-ci when you are content to start the repository workflows?

[shawcharles]

Pushed follow-up commit bb187a3 to complete the fail-closed contract.

• Corrected tutorial 32 and the documentation dependency registry, and made the changelog explicit about the warning-to-TypeError behavioural change.
• Preflight now requires a distinct clone that still implements the required learner protocol; declared survey fits also require the clone to accept sample_weight. Direct cross_fit_predict validates each fold clone as a backstop.
• Delayed clone failures now name propensity_learner or outcome_learner, propagate as hard errors on both panel and repeated-cross-section paths, and clear any prior fitted result.
• Added regression coverage for traceback sanitisation, clone protocol loss, weighted clone loss, panel/RCS propagation, re-fit state, and survey preflight.

Verification: Python 3.9 scoped suite passed (665 passed, 6 skipped), along with Black, Ruff, scoped mypy, changelog validation, documentation integrity checks, and notebook JSON validation. The full repository suite was not run.

[igerber]

Thanks - this lands the boundary exactly as agreed, and the test coverage is thorough.

Fork PRs cannot run the label-gated CI or the CI AI reviewer here, so I opened a mirror of your branch as #817 to get that feedback. The CI review came back clean (no P0/P1) with one P3, and while preparing the mirror I hit one P2-level wording issue. I would like you to make these changes on this PR so the work stays yours; I will refresh the mirror from your branch afterwards. Three items:

1. Narrow the "before any cell" claim to the one-copy preflight (P2). The changelog fragment, docs/api/dml_did.rst (both the Learner configuration paragraph and the "Degenerate cells skip loudly" bullet), the REGISTRY cross_fit_degenerate skip-reason entry, and tutorial 32 all say an uncloneable template raises before any group-time cell is fitted. That is only true of the preflight copy. A per-fold deepcopy can still fail later in the fit and propagate as a hard TypeError, which your own test_fold_copy_failure_propagates_as_a_hard_error exercises. Each surface should distinguish the two: the preflight raises before any cell; a later per-fold clone failure propagates as the same sanitized TypeError, never a NaN-cell skip. I already made this edit on the mirror as commit 50109fe on mirror/816-dml-crossfit-isolation if you want to cherry-pick it or use it as a reference. Either is fine.

2. Stale fallback-era phrase (P3 from the CI review). The DMLDiD seed docstring at diff_diff/dml_did.py:297 and the matching REGISTRY line at docs/methodology/REGISTRY.md:2937 still say the estimator "deep-copies the learner template where copyable". Now that an uncopyable template always raises, replace it with wording along the lines of "requires a distinct deep copy of the template and raises TypeError otherwise", and say explicitly that the supplied template must be unfitted.

3. Make the scikit-learn tests actually run in CI. The two new importorskip("sklearn") tests skip everywhere except your machine, because scikit-learn is not installed by CI and is not in the dev extra. Since sklearn estimators are the dominant real-world custom learner, I would like that test to run for real. Please add scikit-learn (unpinned) to the dev extra in pyproject.toml and to the two pip install lines in .github/workflows/rust-test.yml (the "Install test dependencies" step and the pure-Python "Install dependencies" step). Unpinned is safe across the CI matrix: pip resolves 1.6.1 on the 3.9 leg and 1.9.0 on 3.11 through 3.14, and prebuilt wheels exist for every leg. Keep the importorskip guards so the tests stay optional for users who install without the dev extra.

Once those are pushed I will refresh #817, let CI run, and then move this PR forward.

[shawcharles]

Thanks, pushed 47e98ad with all three requested changes. I used 50109fe as the reference for the preflight/later-fold distinction, replaced both "where copyable" descriptions with the unfitted-template requirement, and added unpinned scikit-learn to the dev extra and both workflow install steps. The importorskip guards remain.

Validation: 582 focused tests passed on Python 3.12, and both DML/cross-fit sklearn tests ran without skips on Python 3.9. Black, Ruff, changelog and configuration-format checks passed.

One additional CI consequence surfaced during review: installing sklearn enables the existing tests/test_base_estimator.py::test_sklearn_clone_round_trip_if_available[StackedDiD], which fails because cloned clean_control and control_group hold distinct <not supplied> sentinels and their parameter dictionaries compare unequal. I reproduced it on Python 3.9/scikit-learn 1.6.1 in both this branch and the unchanged base e982340:

python -m pytest -q tests/test_base_estimator.py -k 'sklearn_clone and StackedDiD'

The requested dependency additions are in place, but this newly enabled failure needs resolving before CI can pass. I left the shared deprecation-sentinel behaviour unchanged to keep this revision within the agreed scope. The full repository suite and remote CI matrix were not run locally.

[shawcharles]

I have a tested local fix for the newly enabled StackedDiD clone test. The production change is limited to the stateless _NotSupplied sentinel in diff_diff/_deprecation.py:

def __copy__(self) -> "_NotSupplied":
    return self

def __deepcopy__(self, memo: Any) -> "_NotSupplied":
    return self

This preserves the omitted-parameter marker during sklearn cloning without changing type-based alias resolution. Tests cover direct copy identity, nested mutable values remaining independently copied, and default/canonical/deprecated StackedDiD configurations with their existing warning behaviour. It does not relax the distinct-object requirement for DML learner templates.

The base-estimator, mechanical/semantic rename, cross-fit and DML suites pass: Python 3.9/sklearn 1.6.1: 590 passed, 19 skipped; Python 3.12/sklearn 1.7.2: 596 passed, 13 skipped. The previously failing clone test runs and passes on both. Black, Ruff, scoped mypy and changelog checks also pass; the full suite and remote CI were not run.

I recommend adding this as one small follow-up commit here, with regression tests and a changelog fragment, to unblock the dependency change. Would you prefer that or a separate PR? The patch is prepared locally; #816 remains at 47e98ad pending your choice.

[igerber]

Thanks - all three items landed as asked, and I confirmed the scikit-learn consequence you found: with sklearn installed, test_sklearn_clone_round_trip_if_available[StackedDiD] fails on unchanged main too, and it is the only test the dependency addition newly enables across the suite (I ran every sklearn-referencing test file on main). Your _NotSupplied.__copy__ / __deepcopy__ returning self is the right fix: the sentinel is stateless and identity-keyed. Please add it as one follow-up commit on this PR, with the regression tests and a changelog fragment as you proposed, so the PR carries the change that makes its own dependency addition CI-clean.

One more thing before I can refresh the mirror: this branch is now conflicting with main. Two PRs merged since your base:

• feat(dml): Caetano, Callaway, Payne & Sant'Anna (2026) bad-control lane on DMLDiD (PR-B) #829 restructured diff_diff/_crossfit.py around an iter_fold_fits generator and, in the process, kept the old _fresh_learner warn-and-reuse fallback (with a warn_stacklevel parameter threaded through). It also changed DMLDiD._validate_and_prepare to carry the bad-control parameters (bad_control, bad_control_covariates, has_survey_design) and return the materialized bad-control covariate list.
• feat: TWFE weight diagnostics (twfeweights R port, re-scoped from #753) #812 touched diff_diff/dml_did.py near the aggregation-kit build; that overlap is textual only.

When you rebase onto main, the invariant to preserve is the one this PR establishes: every clone failure is a sanitized TypeError, never the reinstated fallback. Concretely, _fresh_learner inside iter_fold_fits should become your _clone_learner_template (and the now-dead warn_stacklevel plumbing can go), the per-fold backstop should cover every consumer of iter_fold_fits including the bad-control lane, and require_sample_weight should merge into the new _validate_and_prepare signature rather than replace it. Your existing tests for fold-copy propagation on the panel and RCS paths are the right check that nothing regressed; a sibling for the bad-control lane would close the gap #829 opened.

Once the rebased branch is up, I will refresh #817 from it so the CI reviewer runs on the final shape.

[shawcharles]

Rebased onto abcd785a and pushed ac1ef63c. The separate sentinel commit includes the copy/clone regression tests and changelog fragment.

The rebase preserves iter_fold_fits and the expanded bad-control validation signature. Clone failures remain sanitised TypeError throughout, with the obsolete warning plumbing removed and regression coverage for panel, RCS and nested bad-control stages.

Fresh checks passed on Python 3.13 (1,137 tests) and 3.9 (790 tests), plus Ruff, Black, mypy, documentation/changelog checks and the pre-merge scan. The final documentation-only correction passed another 367 checks. The prior broad run, before that prose correction, recorded 14,542 passes and one failure: test_allowlists_are_reachable reports stale DMLDiDResults.groups / overall_att allowlist entries. I reran that test on unchanged main and the rebased branch; both fail identically, so I left it outside this PR. Full counts and verification limits are in the updated description.

Could you refresh #817 from this head when convenient?

[igerber]

Looks good, thank you for the repeated contributions!