fix: make legacy dataset provenance explicit - #723
Conversation
|
Thanks. I pushed a small review-response follow-up in Changes since the initial PR:
Validation:
From my side this is ready for maintainer review / CI. |
|
Thanks Charles — and more broadly, thank you for the run of work you've put into this library. Five merged PRs since June, and each one has followed the same pattern: find something genuinely wrong, write it up with a reproducer, then fix it. That's unusually disciplined, and it's exactly the kind of contribution that compounds. #722 and this PR are a good example — you didn't just report the dead URLs, you did the archaeology to find pinned, verifiable replacements. I checked the main claims rather than taking the description on faith, and they hold up: all three pinned URLs resolve with matching checksums, and with an empty cache three loaders return real data tagged with Four things and we can get CI running. 1. Two failure paths skip the warning.
2. Castle Doctrine loses the partial first year. The source has a This isn't something you broke — the old synthetic data was 0/1 as well, so nothing regressed. It only matters now because the real data carries information the fake data never had. Could you add a Please don't add anything to 3. Say plainly that 4. Please delete Here's what we learned chasing it. We ran Stata's Your instinct to move the test off live data was right, but the test can't do what its name claims either way. On the synthetic panel it passes at 0.0017 against a 5e-3 tolerance, so it's really just checking that two numbers land inside a loose bound on one fabricated dataset. Gross breakage in ETWFE is already caught more precisely by the R goldens in We're replacing it with direct Stata comparisons for both estimators, so there's nothing for you to write in its place — deleting is simplest, and it removes the Separately — we reproduced #724 and you're right about that too. The rank handling drops Everything else — the pinning, schema checks, atomic cache writes, size limit, and the |
b611147 to
0eaba4f
Compare
|
Rebased onto current
Validation:
This is ready for CI from my side. |
…refresh Records the Cheng-Hoekstra (2013) treatment-variable convention in REGISTRY: the paper's regressor is CDL_it, the proportion of the year the law was in effect (p. 830), preserved verbatim as treatment_exposure. The binary treated column follows the year >= first_treat staggered-DiD convention, which matches neither the paper's main specification nor its drop-the-adoption-year robustness check, and differs from the source file's own post indicator on 21 of 550 rows. Also refreshes surfaces the loader rewrite left stale: the datasets.rst intro still described the pre-checksum cache-fallback behaviour, the doc-snippet stub catalog still described divorce_laws as staggered-adoption data, and the backlog row this work closes was still open.
…venance claims The download handler named IncompleteRead explicitly, but BadStatusLine, LineTooLong and the rest of http.client's protocol errors are siblings under HTTPException and none derive from OSError, so they escaped the documented warn-and-fall-back boundary and propagated out of the loaders. Catch the base class instead, with a regression test covering two additional subclasses. The datasets.rst provenance paragraph also overstated the contract: it claimed commit-pinned sources and parse/validation fallback for every loader, but load_prop99/load_walmart read a mutable SSC path over plain HTTP and run their validation outside the try/except, so a validation failure raises rather than falling back. Scoped both claims to the loaders that honour them.
…et prose _write_cache_atomically bound temp_path only after the write succeeded, so a mid-write failure (ENOSPC, quota) left the delete=False temporary file stranded in the cache directory, and repeated attempts accumulated them. Bind the path at creation instead, with a regression test that fails the write and asserts an empty cache directory. Also aligns per-dataset prose left contradicting the new provenance contract: the datasets.rst divorce section presented always-synthetic data as Stevenson & Wolfers, the mpdta section still called the now-canonical R data 'simulated', list_datasets() advertised 'real-world datasets', and _construct_mpdta_data's docstring claimed to match the R package it only approximates.
…ide provenance The castle income field is documented as per-capita income, but the pinned Stata metadata labels it 'state median income'; the values were never wrong, only the description. (Pre-existing, but now materially misleading since the loader ships the real data.) The REGISTRY note claimed cdl*365 'is an exact integer' for the 17 non-half adoption-year exposures. It is not: cdl is stored as float32, so none of the 17 are bit-exact and the max deviation is 1.03e-5. Reworded to say the products recover integer day counts to within storage rounding. llms.txt still advertised the catalog as 'Real-world datasets' and listed divorce laws among them, and llms-full.txt's section heading likewise, so an agent reading the guide would treat synthetic divorce output as empirical.
…ication The divorce section ran CallawaySantAnna on load_divorce_laws() output and then told the reader the result showed 'effects near zero (validating parallel trends)' and reproduced the 'spike and fade' Wolfers (2006) documented. That data is generated, so the notebook was presenting fabricated numbers as confirmation of a published empirical finding - the exact failure mode this branch exists to remove. Reframes the section as a teaching simulation: a warning callout at the top, provenance surfaced via df.attrs['source'] in the load cell, the parallel-trends and reproduction claims removed, and the summary marking the panel synthetic. The estimator mechanics it teaches are unchanged. Card-Krueger and Castle Doctrine now say plainly that they run on checksum-verified replication data. Markdown and one print statement only; no outputs were committed and none need relocking.
…ete survey The real survey is missing 12 emp_pre and 14 emp_post readings; the synthetic fallback is complete. The load_card_krueger docstring example and the datasets.rst example both melt to long format and fit immediately, so they estimated fine while the loader served synthetic data and raise 'Column employment contains missing values' now that the canonical source resolves. The tutorial already dropped NaNs, which is why it kept passing. Adds the dropna to both examples, documents the missing-value counts in the loader Notes, and adds a regression test that asserts both halves: the raw workflow raises, and the documented one estimates finite output. Also narrows two overclaims in tutorial 09: the intro said the first two sections 'reproduce the published designs', and the Castle section presented its estimates without noting that it regresses the level rate on a binary indicator rather than Cheng-Hoekstra's log rate on fractional CDL_it.
…ce provenance in tutorial The changelog stated categorically that download failures warn and return synthetic data. They do not when a checksum-valid cache entry exists: those bytes already passed the pinned SHA-256, so the loader returns canonical data silently - including under force_download=True, which bypasses the cache on the way in but still falls back to it when the download fails. Qualifies the wording and adds a regression test covering both force_download values. The tutorial intro asserted that the Card and Castle sections run on checksum-verified data, but offline they run on the synthetic fallback and neither cell showed which it got. Both load cells now print df.attrs['source'], and the intro says 'when their canonical sources are reachable'.
…availability note doc-deps.yaml listed only the API page and tutorial for datasets.py, so /docs-impact would not flag the new 'Castle Doctrine treatment coding' registry section when the loader changes - the same drift the map exists to prevent. test_methodology_lwdid.py justified committing castle_lw_subset.csv on the grounds that 'the load_castle_doctrine upstream source is currently unavailable'. That is no longer true as of this branch. Reworded to say the subset is retained as the pinned artifact its goldens were captured against, and that consolidating onto the loader is a separate decision.
Round 5 corrected the categorical 'any download failure warns and returns synthetic data' claim in CHANGELOG.md and docs/api/datasets.rst but left it standing in the three loader docstrings and the tutorial intro - four of six surfaces still asserted a contract the code does not honour. All six now say the same thing: a download failure falls back to a checksum-valid cache entry when one exists and returns that canonical data silently; the SYNTHETIC warning and synthetic_fallback marker apply only when neither a verified cache entry nor a verified fresh download is available, or when the source fails to parse or validate.
…files _write_cache_atomically creates .<name>.<ext>.<suffix> beside the cache entry. The write-failure path unlinks it, but a hard kill between creation and os.replace strands one, and it matches neither *.csv nor *.dta - so it survived clear_cache(), the only remedy the docs offer, and accumulated across runs. Adds the two hidden patterns to the glob, guards on is_file(), and tests that an unrelated file in the cache directory is left alone. Also covers the missing-directory case, which is now reachable because the cache directory is created on write rather than on path construction.
…ilure A transport error fell back to a checksum-valid cache, but a size-limit breach or checksum mismatch raised straight through to the synthetic fallback. So with force_download=True and canonical bytes already on disk, a tampered or moved upstream silently replaced real data with generated data - the opposite of the fail-closed behaviour the pinned-mirror comment claimed, and a contradiction of the contract the previous two commits aligned six surfaces to. The cache is now read up front and retained under force_download, and all three failure modes recover from it. A checksum mismatch additionally warns that the upstream no longer matches the pin: an integrity event should not pass unnoticed, and the warning deliberately omits SYNTHETIC because no synthetic data is involved and callers key provenance checks on that word. The warning's stacklevel is computed rather than fixed. The text and binary loaders reach the download helper at different depths, so the constant that was correct for prop99/walmart pointed inside datasets.py for mpdta/card_krueger.
… raises
The semantic sweep after the cache-recovery fix searched for the phrasings I had
written, so it missed two pre-existing ones: _download_with_cache_binary's
docstring ('a checksum mismatch on freshly downloaded bytes raises') and the
module docstring ('if a source cannot be verified, the loader warns and returns
a synthetic fallback'). Both now describe cache recovery as the first resort.
…taset cache The test called clear_cache() with _CACHE_DIR unpatched, so every suite run deleted ~/.cache/diff_diff/datasets. That was survivable while the loaders served synthetic data; now that the cache holds verified canonical downloads, it forces a re-fetch of all three pinned sources after every test run. Pinned to tmp_path, and an AST sweep confirms no other test touches a loader or clear_cache without either pinning the cache directory or mocking the download. Also refreshes the CI-canary backlog row, which pointed at 'the loader-fallback repair row above' - a row this branch removed as completed.
0eaba4f to
d126b6d
Compare
|
Thanks Charles, the revisions check out - both failure paths behave as intended, and Fair warning that this grew well past the registry note I said I would add. Going through the branch in detail turned up several things small enough to fold in rather than bounce back to you, and they are all now pushed to your branch:
All of it has regression coverage. I also rebased onto current Starting the workflows now. |
Every other canonical-path test substitutes an already-normalized fabricated frame, so the real parse - column naming, first.treat renaming, dtypes - was only ever covered by a live download and never by CI. The canonical bytes are already committed at benchmarks/data/real/mpdta.csv and hash to exactly the pinned digest, so the full loader path runs offline for free. Also guards the pin: re-pinning to a revision that does not match the committed fixture now fails here rather than silently at runtime. Verified the test has teeth by perturbing the digest and confirming it fails. Raised by the CI reviewer on the igerber#723 mirror (igerber#731).
…ng indentation Two CI failures, both defects in my own additions rather than the library. Windows (4 failures, all in test_datasets.py; macOS/Ubuntu/arm all green): the cache-recovery tests seeded the cache with Path.write_text(), which translates \n to \r\n on Windows. The digest they pinned was computed from the untranslated string, so the bytes on disk never matched, the loader correctly refused the cache, and the assertions saw synthetic_fallback. Seed and hash the same bytes via write_bytes(). The library was never affected -- it writes cache entries in binary mode. Sphinx -W (4 warnings, all from load_castle_doctrine): the hanging indent I added under the treatment_exposure bullet in the Returns block reads as a block quote to docutils, producing 'Unexpected indentation' plus 'Block quote ends without a blank line'. The bullet is one line again and the canonical-vs-fallback caveat moved to Notes as prose. Verified by building the docs locally with -W (passes) and by reproducing the CRLF digest mismatch directly.
3 participants
Fixes #722.
Summary
mpdtaURLs with immutable commit-pinned sources and verify every downloaded or cached byte sequence against a pinned SHA-256load_divorce_laws()explicitly warn and return its existing synthetic fallback because no verified source reproduces its composite public schema without additional analytical choicesdf.attrs["source"]; expected source failures emit oneUserWarningcontainingSYNTHETICand usesource="synthetic_fallback"The verified artefacts are:
public.datat commit07bc929f1d6552db117bd27a7cf0d881d16e9494, SHA-25604bde0cad5540980f32ce099c6dad369e2f05494698071d8a65b3e1cbe9ca53acastle.dtaat commitca4279a87a6f0759f6b6f02841a53bdd68e27d3c, SHA-256804633c161827b6c0824f86f239046386d1a8266a866f83bf5ddb2aa762a5f29mpdta.csvat commit7ad707385354cb3924b8da94ef7e62a76bf55a4d, SHA-2562283bea1221a152420f98dfa20f633c5d054ea51d881115c8cd702a97bcd3167mpdtais now the genuine packaged example data. The Wooldridge/Callaway estimator cross-check remains on the pre-existing controlled synthetic DGP so that it tests estimators rather than source availability or a different finite-sample estimand.Validation
python -m pytest tests/test_datasets.py -q(Python 3.12): 69 passedpython3.9 -m pytest tests/test_datasets.py -qin an isolated Python 3.9.25 environment: 69 passedpython -m ruff checkon all touched Python filespython -m black --checkon all touched Python filespython -m mypy --follow-imports=skip diff_diff/datasets.pyAll tests are deterministic and required CI does not depend on live network access.