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

[shawcharles]

Fixes #814

Summary

• Reject custom DML learner templates whose deepcopy fails or returns the original object before any group-time cell is fitted.
• Keep lazy per-fold cloning as a fail-closed backstop for direct cross_fit_predict callers.
• Document the top-level clone boundary and add regression coverage for copy failures, identity copies, state isolation, scikit-learn learners, and configuration-time propagation.

Verification

• pytest -q tests/test_crossfit.py tests/test_dml_did.py tests/test_changelog_fragments.py tests/test_docs_ia.py tests/test_doc_deps_integrity.py (576 passed)
• Python 3.9 targeted regression tests including scikit-learn preflight (4 passed)
• Ruff, Black, scoped mypy, changelog compiler, and a native cross-fit output comparison against current upstream/main
• Not run: the full default suite was stopped after an interrupted long-running attempt.

[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.