Skip to content

ci: per-cell LKG on an orphan branch, plus dependency profiles as uv config files - #1055

Draft
kbattocchi wants to merge 10 commits into
kebatt/doctest-drop-ellipsesfrom
kebatt/ci-profiles
Draft

ci: per-cell LKG on an orphan branch, plus dependency profiles as uv config files#1055
kbattocchi wants to merge 10 commits into
kebatt/doctest-drop-ellipsesfrom
kebatt/ci-profiles

Conversation

@kbattocchi

@kbattocchi kbattocchi commented Aug 10, 2026

Copy link
Copy Markdown
Member

Supersedes #1041, whose commits are included here rebased onto current main.

Replaces the merged lkg.txt / lkg-notebook.txt files with per-cell freeze files on an orphan lkg branch, and introduces named dependency profiles.

Dependency profiles

A profile is a uv configuration file under profiles/, passed via uv --config-file. uv parses its own format, so ci.yml needs no parser and knows only the profile name.

profile pins cells
current newest known-good everything the existing matrix
all-floor resolution = "lowest-direct" ubuntu / 3.12, three kinds

all-floor resolves every direct dependency to its declared lower bound, making it the only thing in CI that exercises the bounds in pyproject.toml. Test cells go from 94 to 97.

Dependency coverage was previously a side effect of which interpreter pinned what: the 18 (os, python) cells collapse to five distinct dependency sets, and the scikit-learn 1.6 / numpy 2.0 floors were reachable only through the Python 3.9 row.

Freeze invalidation

Each freeze records the hash of the profile recipe it was generated from:

# recipe-sha: 4ed249e49727
scikit-learn==1.9.0

A cell uses its freeze only while that hash matches; otherwise it resolves from the recipe. Two consequences:

  • A fallback preserves the profile's intent. An unconstrained install resolves to newest, which would silently turn all-floor into a duplicate of current — floating selects numpy 2.5.2 where lowest-direct selects 2.0.2.
  • A dependency change and the code it forces can land together. Editing a recipe invalidates that profile's stale freezes, so the cells re-resolve and the PR is green on its own, with nothing to clean up afterwards. MAINTAINING.md documents this under "Changing dependencies and code together".

profiles/all-floor.toml records no freeze, since re-resolving from the floors is its whole purpose.

Also included

  • README's developer sections split into CONTRIBUTING.md and MAINTAINING.md, sequenced as the first commit so later commits document the new system in MAINTAINING.md directly.
  • verify now depends on eval. It is the only required check on main and a skipped job counts as a pass, so an eval failure previously made every downstream job skip while the required check reported green.
  • NonParamDML's doctest updated for scikit-learn 1.9, which changed how RandomForest draws its weighted bootstrap. Without lkg.txt the docs job resolves 1.9, so the recorded value has to match it.
  • .github/workflows/tests/test_ci_matrix.py expands the matrix the way Actions does and asserts that every cell names a profile with a recipe on disk, that include-created combinations carry the fields the job bodies dereference (they inherit nothing), and that recipes parse with no unrecognised keys.

@kbattocchi

Copy link
Copy Markdown
Member Author

First CI run: what it tells us

Two things, one expected and one a real defect I've since fixed.

1. The docs job failure is pre-existing, not from this change. It is the scikit-learn 1.9 ellipsis drift:

Failed example: est.const_marginal_effect(X[:2])
Expected: array([[0.516888..., 0.995747...], ...])
Got:      array([[0.51691875, 0.99584224], ...])

That is exactly what #1051 fixes (the ellipsis defeats scipy-doctest's float-tolerant checker, so a 6e-5 drift is fatal) and #1052 completes.

It surfaced here because this run exercised the bootstrap path for real: the lkg branch still holds files under the old naming convention, so no cell found a matching freeze and every one resolved fresh — which pulled scikit-learn 1.9. So the run doubles as an unplanned end-to-end test of the fallback, and it behaved correctly. The ::notice:: lines confirm the intended branch was taken.

Practical consequence: land #1051 and #1052 before this, otherwise this PR's docs job stays red for reasons unrelated to its diff.

2. Job names were leaking matrix internals. The all-floor cells rendered as:

Run tests (ubuntu-latest, 3.12, other, all-floor, false, -m "cate_api and not ray" -n auto, [plt])

A combination created by include: exposes every key in its auto-generated name — including record_freeze and the raw pytest opts. Unreadable, and unstable in a way that matters: adding any matrix property renames the check and would silently break a branch protection rule naming it.

Fixed in c12dd2c with explicit names (Run tests (current, ubuntu-latest, 3.12, dml)), plus a test asserting the job name pins the displayed identity so a future property cannot leak back in.

Still waiting on the all-floor cells, which are the genuinely unknown part — nothing here has ever run scikit-learn 1.6 on a modern interpreter.

@kbattocchi
kbattocchi changed the base branch from main to kebatt/doctest-drop-ellipses August 11, 2026 01:35
kbattocchi and others added 7 commits August 10, 2026 21:57
README.md had grown to roughly a thousand lines, of which the developer-facing
material was about 70 and mixed two audiences: everything a first-time
contributor needs (environment setup, finding an issue, running tests, building
docs) sat alongside release steps only an admin can perform.

Move it into two files:

* CONTRIBUTING.md -- the contributor workflow. GitHub gives this filename special
  treatment, surfacing a "Contributing guidelines" link on the repository landing
  page and in the new-issue and new-pull-request flows, so a newcomer is far more
  likely to find it there than partway down a long README.
* MAINTAINING.md -- tasks requiring maintainer permissions. Currently just the
  release process; the dependency-pinning infrastructure lands here in a later
  commit, which is the immediate reason for making the split now rather than
  growing the README further.

README.md keeps a short "For Developers" section pointing at both, and the
"Contributing and Feedback" section now links to CONTRIBUTING.md as well.

Two things fixed in passing:

* The release checklist said to add a news entry "in the news section of this
  file". That file is now MAINTAINING.md while the news section stayed in
  README.md, so the reference is updated to name README.md explicitly.
* Dropped the placeholder note in the sklearn-compatibility section promising
  that "the broader contributor guidance will be split out into a dedicated
  CONTRIBUTING.md in a future change" -- this is that change.

Content is otherwise moved verbatim. No links elsewhere in the repository pointed
into the relocated sections, so nothing else needed updating.

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
Adds a new push-lkg job that runs after the tests, notebooks, build, and
docs jobs on the nightly schedule (or workflow_dispatch with
use_lkg=false dispatched from main without an inputs.ref override). It
downloads the per-cell requirements-* artifacts those jobs upload,
checks out the long-lived lkg branch into a worktree, copies the freezes
in as lkg-tests-<os>-<py>-<kind>.txt / lkg-notebooks-<kind>-<py>.txt
(and, after the install-side rewrite lands, lkg-build-<os>-<py>.txt and
lkg-docs-<os>-<py>.txt), and commits + pushes with a small fetch/reset
retry loop so concurrent runs don't race.

The trigger condition is intentionally restrictive on the dispatch path:
github.ref must be refs/heads/main and inputs.ref must be empty (or
main), so manual runs that test against a future/breaking dep version on
a feature branch with use_lkg=false do NOT enshrine that version into
LKG before the breaking change has been reviewed and merged. The
schedule path is unrestricted because nightly only runs on the default
branch.

The job is serialized via an lkg-push concurrency group with
cancel-in-progress: false, gated by !cancelled() so a single failing
test cell doesn't block updating the other cells' freezes (the freeze
artifact is uploaded before pytest runs, so it captures install state
not test state), and runs with contents: write permission so it can push
to the protected lkg branch.

build_wheels is intentionally not included in the needs list because it
does not use LKG today: cibuildwheel manages its own per-Python build
envs and the use_lkg input only affects which Python versions get built.

This is the first half of the LKG redesign. The install-side rewrite
that consumes these per-cell files (and adds the matching upload steps
to build_sdist / create_docs) lands in a follow-up commit, and the old
merged-lkg.txt machinery (store-reqs-per-env / merge-artifacts /
generate-lkg / merge-lkg / generate_lkg.py) is removed after that. The
lkg branch itself must be created and seeded manually by the user before
this job will succeed; until then it will fail loudly on the checkout
step, which is the intended bootstrap signal.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Rewrites the install step in the tests, notebooks, build_sdist, and
create_docs jobs to check out the long-lived lkg branch into a
lkg-cache/ subdir and read its own per-cell freeze, falling back to a
floating install when the file is missing. The per-cell file naming
matches what push-lkg writes:

  tests cells       -> lkg-tests-<os>-<py>-<kind>.txt
  notebooks cells   -> lkg-notebooks-<kind>-<py>.txt
  build_sdist       -> lkg-build-ubuntu-latest-3.12.txt
  create_docs       -> lkg-docs-ubuntu-latest-3.12.txt

The lkg checkout uses continue-on-error: true so the bootstrap case
(branch not yet created, or this matrix cell newly added with no entry
on the branch yet) silently falls through to a floating install. A new
cell will install floating once, the per-cell freeze it uploads will be
picked up by push-lkg on the next nightly, and subsequent PR runs will
then install from that freeze.

For build_sdist and create_docs (which were previously using the merged
lkg.txt directly) this also adds the pip freeze + upload-artifact steps
so their per-cell freezes flow into push-lkg the same way tests' and
notebooks' do. build_wheels is intentionally left untouched because
cibuildwheel manages its own per-Python build envs and never read
lkg.txt.

For the notebooks job specifically, the Install notebook requirements
step (jupyter, jupyter-client, nbconvert, nbformat, seaborn, xgboost,
tqdm) is folded into the same shell block: when the per-cell freeze
exists it already pins those packages, and when it doesn't we install
them as before. This collapses two steps into one and removes the
awkward !env.use_lkg conditional.

The old root-level lkg.txt and lkg-notebook.txt are removed in this same
commit; the install fallback handles the brief window between this PR
merging and the next nightly populating the lkg branch. The push-lkg
job will fail to find files until the lkg branch is created, which is
the intended bootstrap signal.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
The push-lkg job + per-cell install rewrite in the preceding commits make
the merged-lkg.txt machinery obsolete. Removed:

  * store-reqs-per-env  (jobs/ci.yml) - replaced by the freeze upload
    steps already present in tests, notebooks, build_sdist, create_docs.
  * generate-lkg        (jobs/ci.yml) - no merged lkg.txt to generate.
  * merge-lkg           (jobs/ci.yml) - no merged lkg artifact to bundle.
  * generate_lkg.py     (.github/workflows/) - the marker-merging Python
    script that backed generate-lkg.

merge-artifacts survives but is trimmed: it now bundles only the
non-LKG categories (requirements, tests, coverage, notebooks) for
PR-browsing UX, drops store-reqs-per-env from its needs list, and adds
build + docs so the merged requirements bundle picks up the per-cell
freezes from build_sdist and create_docs.

This removes both prior bugs by construction:

  Bug 1 (partial re-run breaks merge): there is no longer a merge step
  in the LKG path; push-lkg sees only the per-cell artifacts uploaded
  during this attempt's run.

  Bug 2 (marker-based merge over-shares versions across kinds): each
  matrix cell now reads its own freeze; no version is ever pinned in a
  cell that didn't produce that version.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
This adds a manual seed/recovery tool that walks `gh run download` output
from a green nightly and copies each cell's requirements file onto the
`lkg` branch worktree under its renamed name.

Why a script:
- The normal flow is the `push-lkg` job auto-committing freezes after
  every nightly. The script exists for bootstrap (the very first seed of
  the `lkg` branch) and for manual recovery (e.g., if the user wants
  to roll the branch back to a specific run).
- The manual recipe is in the module docstring so it's discoverable from
  the file itself.

Tests:
- New `.github/workflows/tests/test_seed_lkg_branch.py` covers
  `rename_artifact_file` mapping, `collect_freezes` directory walking,
  and `main` success/failure paths.
- New `lint` job step runs them. `-o addopts=""` neutralizes the
  package-wide pytest addopts in pyproject.toml (which need pytest-xdist,
  pytest-cov, and the econml package itself — none of which are
  installed in the lint job).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Describes the per-cell freeze layout on the orphan `lkg` branch introduced by the
preceding commits: how the nightly populates it, how PR runs consume it, the
filename convention, why the push job is gated the way it is, the one-time setup,
and the recovery paths for seeding the branch or unwedging a single cell.

Lands in MAINTAINING.md rather than README.md -- operating the dependency-pinning
infrastructure needs maintainer permissions, so it belongs with the release
process rather than in the contributor-facing docs.

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
The orphan `lkg` branch has no `.pre-commit-config.yaml`. If a
contributor with `pre-commit install` already run on the repo
follows the seed recipe from the README or seed_lkg_branch.py
docstring, `git commit` will abort with a confusing "No
.pre-commit-config.yaml found" error inside the lkg worktree.

`--no-verify` skips that hook for these specific commits, which is
appropriate since:

- The lkg branch contains only pip-freeze text files, not source code.
- The repo's pre-commit hook only runs ruff on python/pyi/jupyter
  files anyway, so it has nothing meaningful to check here.

Also drops the DCO sign-off (`-s`) from the seed commit example —
the lkg branch's commits represent pip freeze output, not human
authorship, so the DCO attestation doesn't conceptually apply.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
kbattocchi and others added 3 commits August 11, 2026 12:27
Builds on the per-cell LKG redesign in the preceding commits by naming the
dependency configurations CI is expected to support, instead of inheriting them
by accident.

Today's coverage is accidental. Evaluating lkg.txt's markers across all 18
(os, python) cells yields only five distinct dependency sets, and two pairs of
those are near-duplicates -- 3.11 and 3.12 are identical, and 3.13 differs only
in scipy. Which scikit-learn versions get tested is a side effect of which
interpreter pins what, so dropping Python 3.9 (EOL) would silently delete the
only coverage of the scikit-learn 1.6 / numpy 2.0 floors that pyproject.toml
declares.

A profile is a uv configuration file under profiles/, passed to uv with
--config-file. uv parses its own format, so the workflow needs no parser and no
per-profile table: ci.yml knows only the profile name. Any uv setting is
available without inventing syntax, which keeps the door open for profiles that
do not fit a "constraints plus flags" shape.

Two profiles to start:

* current -- newest known-good everything; the whole existing matrix.
* all-floor -- resolution = "lowest-direct", so every direct dependency lands on
  its declared lower bound. Verified to resolve to exactly the floors in
  pyproject.toml (numpy 2.0.2, scipy 1.13.1, scikit-learn 1.6.0, pandas 2.2.3,
  shap 0.46.0, statsmodels 0.14.4, numba 0.60.0), which nothing in CI currently
  exercises. Three ubuntu/3.12 cells, so 97 test cells rather than 94.

Freezes now record the hash of the recipe they came from, and a cell only uses
its freeze when that hash still matches:

    # recipe-sha: 4ed249e49727
    scikit-learn==1.9.0

This is what makes a floating fallback safe. Falling back to an unconstrained
install would resolve to newest, which for a variant profile silently converts it
into a duplicate of current -- and for all-floor inverts it outright, since
floating selects numpy 2.5.2 where lowest-direct selects 2.0.2. Applying the
recipe on both paths keeps a profile's intent whether or not a freeze exists.

Hashing also closes the atomicity gap the per-cell redesign otherwise opens. With
pins on a separate branch a pull request cannot change its own pins, so a change
needing both new pins and new code deadlocks -- scikit-learn 1.9 moved a
NonParamDML doctest by 37%, and the new value only passes on 1.9. Editing a
recipe invalidates that profile's freezes by hash, so the cells re-resolve and
both halves land in one green pull request. Invalidation is derived rather than
declared, so no follow-up commit is needed to undo an override.

all-floor deliberately records no freeze; re-resolving from the floors is the
point, and lowest-direct is deterministic, so nothing is lost.

Also make `verify` depend on `eval`. It is the only required check on main and a
skipped job counts as a pass, so an eval failure would skip every downstream job
and report green with nothing having run. That is independent of this change but
becomes load-bearing as eval gains logic.

Adds .github/workflows/tests/test_ci_matrix.py, which expands the matrix the way
Actions does and asserts that every cell names a profile with a recipe on disk,
that include-created combinations carry the fields the job bodies dereference
(they inherit nothing), that recipes parse as TOML with no unrecognised keys, and
that verify depends on eval.

Both recipes were resolved against live PyPI; the hash invalidation was exercised
end to end.

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
A combination created by an `include:` entry exposes every one of its keys in the
auto-generated job name, so the all-floor cells rendered as

    Run tests (ubuntu-latest, 3.12, other, all-floor, false, -m "cate_api and not ray" -n auto, [plt])

which is unreadable, and unstable in a way that matters: adding a matrix property
would rename the check and silently break any branch protection rule that names
it. Pin the displayed identity to the dimensions we actually care about, and
assert it in test_ci_matrix.py so a future property cannot leak back in.

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
… doctest

Removing lkg.txt in the preceding commits means the docs job no longer installs a
pinned scikit-learn 1.8. It resolves from profiles/current.toml, which carries no
constraint on scikit-learn, so it gets the newest release the project permits --
1.9 today, since pyproject caps below 1.10.

scikit-learn 1.9 changed RandomForest and ExtraTrees to use sample_weight when
drawing the bootstrap sample, rather than forwarding the weights alongside a
uniform mask. NonParamDML is maximally exposed to that, because its final-stage
weights are literally T_res**2 and so are highly skewed. Its doctest moves:

    array([0.35318, 1.28760, 0.83506])  ->  array([0.22215931, 1.6484125 , 0.9278963 ])

Recorded rather than worked around. Benchmarked on a known DGP over 6 seeds at
n=2000, mean MSE improves from 0.1795 to 0.1571 and is better on every seed, with
bias essentially unchanged, so 1.9 is the better estimator here. econml's own
forests (CausalForestDML, econml.grf, econml.policy) are Cython and unaffected.

This lands in the same pull request as the change that causes it, which is the
point: a dependency change and the code change it forces belong together. That is
the workflow MAINTAINING.md now documents under "Changing dependencies and code
together", and it is why the freeze files record the hash of the profile recipe
they were generated from.

Originally prepared as part of a separate LKG-refresh change; the rest of that
work refreshed lkg.txt, which these commits delete outright, so only this part
survives.

Release note: estimators whose final stage is a scikit-learn forest -- most
notably NonParamDML -- produce different results under scikit-learn 1.9 and
later. Effects are statistically equivalent or slightly better and no action is
required.

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11774e2-02be-42fa-a2b9-429871322083
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant