[Mirror] Dataset loader provenance - CI-review mirror of PR #723 (do not merge) - #731
Closed
igerber wants to merge 17 commits into
Closed
[Mirror] Dataset loader provenance - CI-review mirror of PR #723 (do not merge)#731igerber wants to merge 17 commits into
igerber wants to merge 17 commits into
Conversation
…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.
Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
Methodology
Code QualityNo findings. Validation failures, empty sources, non-finite values, malformed panels, and provenance are handled consistently. PerformanceNo findings. Downloads and cache reads are bounded at 50 MiB. MaintainabilityNo findings. Tech Debt
SecurityNo findings. Commit-pinned HTTPS sources, SHA-256 verification, bounded reads, and atomic cache replacement improve the integrity boundary. Documentation/Tests
|
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 #723 mirror (#731).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
Methodology
Code QualityNo findings. PerformanceNo findings. Cache and response reads are bounded at 50 MiB. MaintainabilityNo findings. Tech Debt
SecurityNo findings. SHA-256 verification, bounded downloads, atomic replacement, and verified-cache recovery strengthen the integrity boundary. Documentation/Tests
|
Owner
Author
|
closing PR, only used for workflow |
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
3 participants
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.
Summary
main). Fork PRs cannot receive CI AI review (fork-skip security gate inai_pr_review.yml), so this same-repo mirror exists solely to run review rounds against the identical tree.ready-for-ci- fix: make legacy dataset provenance explicit #723 is the merge vehicle and preserves the contributor's authorship. This PR closes without merging once fix: make legacy dataset provenance explicit #723 lands; the endgame check is tree-hash equality between the two heads.treatment_exposure) + 13 maintainer commits on top. Head isd126b6de, identical to fix: make legacy dataset provenance explicit #723's head.Maintainer commits address, in order of severity:
HTTPExceptionfallback boundary. The handler namedIncompleteRead;BadStatusLine/LineTooLongare siblings and none derive fromOSError, so they escaped the warn-and-fall-back contract and propagated out of the loaders.SYNTHETIC, since no synthetic data is involved).emp_pre/ 14emp_postreadings while the synthetic frame was complete, so the docstring anddatasets.rstexamples estimated fine until the source resolved._write_cache_atomicallystranded its temp file when the write itself failed, andclear_cache()could not remove those hidden files afterwards.test_clear_cache_creates_directoryran against~/.cache/diff_diff/datasetsunpinned.datasets.rst, both LLM guides, tutorial 09,doc-deps.yaml, and two backlog rows.Methodology references (required if estimator / math changes)
load_castle_doctrine). Cheng & Hoekstra (2013), JHR 48(3), p. 830 defineCDL_itas the proportion of yeartin which the law was effective; that regressor is preserved verbatim astreatment_exposure. The binarytreatedcolumn follows the staggered-DiD conventionyear >= first_treat, which matches neither the paper's main specification nor its drop-the-adoption-year robustness check, and differs from the source file's ownpostindicator on 21 of 550 rows. Documented with**Note:**labels; replication requireslog(homicide_rate)ontreatment_exposure.Validation
ruff,black --check,mypy diff_diff(zero errors); source parses under 3.9 grammar (library floor).test_datasets,test_doc_snippets,test_docs_ia,test_guides,test_doc_deps_integrity,test_methodology_callaway,test_methodology_wooldridge,test_etwfe_cs_stata_parity,test_v4_matrix,test_wooldridge,test_methodology_lwdid.docs/api/datasets.rstcode blocks executed verbatim against both canonical and synthetic-fallback data: 28 executions, 0 failures.CallawaySantAnnaon mpdta -0.03995 (SE 0.01203) vs the Rdidvignette.public.datis byte-identical to the copy innjmin.zipfrom David Card's data archive; the pinnedmpdta.csvmatchesdid::mpdta(R 2.5.1) to CSV round-trip precision.tests/test_wooldridge.pycallsload_mpdta()at 26 sites and was silently running on synthetic data before this change. Verified passing both on canonical data and with an empty cache plus blocked network (251 each).09_real_world_examples.ipynbexecutes.Security / privacy
HOMEno longer crashes the loaders.