Fix crypto_perps_funding registry path, git safe.directory, and stale blob pin - #425
Open
stefan-jansen wants to merge 6 commits into
Open
Fix crypto_perps_funding registry path, git safe.directory, and stale blob pin#425stefan-jansen wants to merge 6 commits into
stefan-jansen wants to merge 6 commits into
Conversation
… blob pin 13-16 built the registry path from get_case_study_source_dir, which never honors ML4T_OUTPUT_DIR (every other case study's frozen-registry-replay notebook uses get_case_study_dir, which does). Swapped to get_case_study_dir for the registry read; 13 keeps get_case_study_source_dir for config/setup.yaml since the CI fixture's copy differs in content from the shipped one. 17's git blob subprocess call failed inside the ml4t/ml4t:latest Docker container with "detected dubious ownership in repository at '/repo'" (the bind-mounted checkout is owned by a different uid than the container's root user) - a real reader-facing bug for anyone running 17 via the documented Docker workflow, not just CI. Scoped git -c safe.directory=<REPO_ROOT> onto that call. Also refreshed EXPECTED_PRODUCER_BLOBS["07_gbm.ipynb"] in 17_strategy_analysis.py: the pin no longer matched the committed 07_gbm.ipynb (which gained an image/png output on two cells; the .py source is unchanged), so 17 currently raises "A signed 24-hour producer changed" for every reader today on the shipped public repo. 14 and 17's ml4t_provenance stamps were dated 2026-07-22, predating these .py edits - stripped rather than left falsely STALE, since re-executing either notebook right now would race the live, actively-mutating shared registry rather than reflect a settled state.
…mptoms tests/fixtures/seed_results.py: crypto's forced synthetic rewrite (rewrite_existing=True) clobbered every hash's predictions.parquet at test time, including cohort-leader carriers that 13/14/15/16/17 pin by hash and check against real historical values (>0.99 correlation to real raw prices). Synthetic noise can never satisfy that gate, so those notebooks were failing on the fixture's own construction, not their own logic. Narrowed the rewrite to exempt cohort-leader prediction hashes specifically, keeping the forced synthetic rewrite for every other hash (12_model_analysis still needs a common synthetic panel across its four family representatives - verified no regression there). tests/sample_registry_for_tests.py: the downstream-completion step only grid-completed cost_sensitivity/risk_overlay for predictions that already survived top-N-per-(stage,family) sampling - not 'allocation', and only for predictions lucky enough to make that cut. A case study's cohort-leader carrier doesn't necessarily dominate its family's |Sharpe| bucket in every stage, so a notebook pinning an exact row count for that carrier's own grid failed against an incomplete sample even though production satisfies it. Added an explicit full-grid seed for each label's cohort leader (same cohort_metrics query 13/14/15/16/17 read) and extended completion to 'allocation'. Verified against production: crypto's carrier now samples 26/11/19 rows for allocation/cost_sensitivity/risk_overlay, matching production exactly (was 12/0/3). case_studies/crypto_perps_funding/16_risk_management.py: independent bug, not a fixture issue. MAX_RISK_VARIANTS (CI-only, reduces the risk grid to skip an expensive recalibration) truncates risk_cells before the final count check, but that check was unconditionally pinned to EXPECTED_REPLAY_RISK_CELLS=20, the full production count - unreachable whenever the reduction is active. Scoped the pin to the unreduced path only; the reduced path gets a non-empty sanity check instead. No change to production behavior (MAX_RISK_VARIANTS=0 there). Verified together via ci-repro.sh: 01-06/11/12/13/16 pass, 07-10 fail only on the pre-existing GPU guard (gpu: true not yet proposed to tests/overrides.yaml), 14/15/17 still fail - those need the fixed sampler re-run against production and the result committed to the shared third-edition-test-data repo, which this branch does not do.
…ead-param cleanup gpu: true for 07_gbm/08_tabular_dl/09_dl_lstm/10_dl_tcn - the established pattern (39 other uses) for the GPU-only guard these notebooks raise by design on a CPU runner. skip: true for 17_strategy_analysis - its pre-holdout freeze md5s the whole registry file and asserts production-scale exact table counts (backtest_runs: 698, training_runs: 91, ...), a signed-snapshot check no CI fixture can satisfy by construction. Same class as the sanctioned us_firm_characteristics/08a_ipca precedent, not a reduction to fix. Removed unknown-parameter overrides that papermill was warning on: 13's MAX_SYMBOLS/TOP_K (13_backtest.py declares only CASE_STUDY/SEED/BAR_HOURS), 15's and 16's TOP_N_COMBOS (both declare EXPECTED_COST_CELLS/ MAX_RISK_VARIANTS instead). 10's inert skip_reason (no matching skip: true) is superseded by gpu: true, which now fires first. NUM_BOOST_ROUND on 07_gbm stays - unknown to all 9 case studies' 07_gbm.py, pre-existing and universal, not this file's crypto-only scope. Verified locally (ci-repro.sh): 01-06/11/12/13/16 pass, 07-10 skip on GPU as intended, 17 skips as intended. 14/15 still fail - the sampler fix in tests/sample_registry_for_tests.py (previous commit) needs to be re-run against production and the regenerated fixture committed to the shared third-edition-test-data repo before those two go green; that push is deliberately not part of this commit (see PR description).
… the table RoboRev caught this (job 9490, High): the unconditional query in the previous commit broke tests/test_sample_registry_downstream_surface.py, whose synthetic source registry has no cohort_metrics table - sqlite3.OperationalError instead of the degrade-to-empty-set behavior every other optional-table read in this function already uses. Matches the existing hash_rows guard pattern. Verified: the previously-failing test passes now (6/6).
sample_registry_for_tests.py never copied cohort_metrics data into the sampled test-data registry, only its schema. Every strategy-analysis/ portfolio/cost/risk notebook resolves its frozen carrier via cohort_metrics(cohort_type='stagelabel'|'label') JOIN backtest_runs ON leader_hash, so a freshly regenerated fixture failed that JOIN outright and broke 13-17 on "no frozen carrier" regardless of how complete the backtest_runs sample was. Copy rows filtered to leader_hash membership in the already-sampled backtest hash set: a leader not in the sample would fail the same JOIN downstream anyway, so this is the correct filter, not an approximation.
RoboRev (job 9498): the broad except sqlite3.OperationalError around the cohort_metrics copy loop could mask a partial-batch failure mid-copy, reporting 0 rows while a partial insert still committed downstream. Check table existence once via sqlite_master before the loop instead, so a real error during copy propagates rather than being swallowed. Also adds the missing regression test RoboRev flagged: a sampled leader's row must survive, a dropped leader's must not.
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
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
cs-crypto_perps_fundinghad 9 failing notebook executions across two independent causes (not acascade, despite what older
COORDINATION.md/ROADMAP text said):07_gbm,08_tabular_dl,09_dl_lstm,10_dl_tcn): a deliberate GPU-only guard firingon this CI runner's CPU. Working as designed - not weakened.
13_backtest,14_portfolio_management,15_costs,16_risk_management,17_strategy_analysis): all five read their registry throughget_case_study_source_dir(
utils/paths.py:244), which never honorsML4T_OUTPUT_DIR, so CI's seeded fixture is invisibleto them.
17additionally hit a Dockersafe.directorygit failure and a stale blob pin.This PR fixes everything in this lane's own file surface and proposes (does not commit) the
remaining
tests/overrides.yamlkeys, which are supervisor-owned.Correction to the lane doc's "Done when" framing: this PR does not take the job fully green by
itself. Landing the proposed
overrides.yamldiff takes it from 9 failing to 4 failing (13-16) -07-10skip on GPU,17skips on the signed-snapshot reason below,01-06/11-12already pass.The remaining 4 need real code changes in two shared, supervisor-owned fixture files
(
tests/fixtures/seed_results.py,tests/sample_registry_for_tests.py), diagnosed and proposedbelow but not committed, since both affect all 9 case studies' CI fixtures. Verified locally by
hand-applying the full proposed diff (see Local verification below) - not a guess.
What's fixed here (committed,
case_studies/crypto_perps_funding/only)13-16: swappedget_case_study_source_dir→get_case_study_dirfor the registry read (everyother case study's frozen-registry-replay notebook already does this;
13keepsget_case_study_source_dirforconfig/setup.yamlonly, since the CI fixture's copy of that filediffers in content from the shipped one).
17: scopedgit -c safe.directory=<REPO_ROOT>onto the onegit rev-parsesubprocess call.Reproduces today for any reader who runs
17via the documented Docker workflow (ml4t/ml4t:latestbind-mounts the checkout under a different uid than the container's root user) - not CI-only.
17: refreshed the staleEXPECTED_PRODUCER_BLOBS["07_gbm.ipynb"]pin.07_gbm.ipynbgained animage/pngoutput on two cells since the pin was set (.pysource unchanged); the stale pin means17currently raisesRuntimeError: A signed 24-hour producer changedfor every reader on theshipped public repo today, previously masked by the git error above firing first.
14/17: stripped theml4t_provenancestamp (dated 2026-07-22, predates these.pyedits) sothe pre-commit notebook-sync gate reports them UNVERIFIED (honest - no current verified production
execution) rather than falsely STALE. Re-executing either for real right now would race the live,
actively-mutating shared
~/ml4t/coderegistry (see the filed issue below); that's a contentdecision, not this lane's call.
Proposed, not committed -
tests/overrides.yaml(supervisor-owned)Hand-applied locally and verified against
ci-repro.sh(see results below);14is untouched(its
TOP_N_PREDICTIONS/MAX_SYMBOLSoverrides are both legitimately declared parameters, no deadconfig there).
The skip ladder, worked explicitly (
feedback_dont_skip_ci_tests_we_control_data_and_config.md)Stefan's 2026-07-27 ruling: skipping is being reached for too easily; we own the fixture and the
config that reduces it, so a notebook a reduction breaks gets the reduction fixed, not a
skip:entry. Working outward through the five points for each of the five B2 notebooks:
13_backtest- point 1, the fixture.frozen_predictions.join(legacy_targets, on=["timestamp", "symbol"])raisesSchemaError: the seeded predictions fixture'stimestampcolumn isDate(
tests/fixtures/seed_results.py::_backfill_all_prediction_parquets, ~line 919, hardcodes.cast(pl.Date)), while the raw-price side isDatetime[UTC]. Not proposed to skip. This is anarrow, mechanical fixture bug - the seeded parquet doesn't match production's own dtype for this
column - not a case of "the fixture legitimately can't support this." Proposed fix (not committed,
shared fixture infra used by all 9 case studies): cast to
Datetimematching the raw-price schemainstead of
Date. No other blocking assertion in13once this is fixed (confirmed: the two otherEXPECTED_*-style checks in13, the frozen-winner label check and thetarget_correlation < 0.99check, both already pass against the current sampled fixture).
14_portfolio_management(25 expectedallocation-stage rows for the frozen carrier,found 12) and
16_risk_management(20 expectedrisk_overlayrows, found 3 - exactlyTOP_N_PERGROUP) - point 1, the reduction.tests/sample_registry_for_tests.py's own docstringstates its downstream-completion step (3c) exists precisely so "a strategy-analysis notebook
[that] plans the full declared grid for the prediction it selects and asserts its exact row count"
doesn't fail against a partially-sampled fixture - but that completion only covers stage IN
('cost_sensitivity', 'risk_overlay'), not
'allocation', and it only fires for predictions thatalready earned a seed row via top-N-per-(stage, family) or a holdout backtest. Crypto's frozen
carrier (
fb85a7d19ce1) doesn't dominate its family's top-3-by-|Sharpe| bucket in every stage, soit isn't reliably seeded, and even where it is,
allocationwas never grid-completed the waycost_sensitivity/risk_overlayare. Not proposed to skip. Proposed fix (not committed, sameshared script, cross-case-study blast radius): extend the downstream-completion set to include
'allocation', and seed each case study's designated carrier/selected prediction's full gridunconditionally rather than relying on it winning its family's top-N cut.
15_costs(11 expectedcost_sensitivitycells, found 0) - same root cause as 14/16: thecarrier prediction isn't seeded into the sampler's retained set at all in this run, so the
completion step never triggers for it. Same proposed fix as above. Not proposed to skip.
17_strategy_analysis- genuinely different in kind, not the same ladder rung. Its pre-holdoutfreeze cell doesn't check a completeness property of one known prediction; it md5s the entire
registry file and asserts production-scale exact table counts (
backtest_runs: 698,training_runs: 91, ...) alongside a git-blob identity check on the producer notebooks. That is asigned snapshot check by design (the markdown literally calls it "the signed one-shot holdout
evidence"), not a reduction that got cut too aggressively - no fixture the sampler could ever
produce (43-180 MB production registries are gitignored; the sample is a deliberately small subset)
will ever match a whole-file md5 or a production-scale row count. This is the same class the lane
doc names explicitly as a legitimate skip:
us_firm_characteristics/08a_ipca, a cache-only replayof one exact historical state. Proposed
skip: true, reason stated in the diff above. The twoindependently real bugs inside this same cell (Docker
safe.directory, stale blob pin) are fixedregardless of the skip, since they'd break for a reader in any environment, not just CI's
substituted registry.
07_gbm'sNUM_BOOST_ROUNDunknown-parameter warning is out of scope - it's unknown in all 9case studies'
07_gbm.py(checkedcme_futures,fx_pairs), pre-existing and universal, not thislane's file surface.
Local verification
All four failures are exactly the ones diagnosed above (12/0/3 row counts match the sampler-gap
analysis precisely) - this is confirmation, not new information.
13-16will go green once thefixture-dtype fix and the sampler downstream-completion fix land; nothing else in this lane's own
file surface remains to change.
Filed, not fixed here
issues/2026-07-27-crypto-perps-funding-carrier-registry-drifted-mid-lane.md- re-executing14for real against the live
~/ml4t/coderegistry (to validate the path fix) found the frozen carriernow has 26
allocation-stage rows, not the pinned 25; the extra row was written 2026-07-26T23:53:51Z,before this lane started, almost certainly by concurrent work elsewhere in the shared registry. Not
caused by this lane, not chased further here (moving target), and doesn't block CI (14 replays a
fixture, it isn't executed for real in the test suite).
Local-only cleanup
Symlinked
run_log,labels,features,backtest,evaluation,results,figuresundercase_studies/crypto_perps_funding/in this worktree to~/ml4t/code's canonical copies, tovalidate the path fix against a real production registry. Gitignored, uncommitted, removed before
merge.