From 174d174713d69e32ad6a0ff849fd84bd9acf5ef0 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 05:13:04 +0200 Subject: [PATCH 1/6] feat(ci,#19101): reveiller la branche credited-examples de exercises-advisory (balayage post-mortem) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit La branche « exemples credites » posee par #18761 etait **dormante**. Le diff exige `--base` ET `--pr-body-file` ; sous `schedule` -- seul declencheur qui subsiste apres la tranche 1 de #12817 -- il n'y a pas de contexte PR, donc `PR_NUMBER` est vide, donc `PR_BODY_FILE` reste vide, donc le test de `exercises-advisory.yml` prend la branche sans `--base` et **saute le diff**. Le label `credited-examples-lost` ne pouvait structurellement pas etre pose. ## Ce que livre la PR `scripts/notebook_tools/credited_examples_sweep.py` : balayage **post-mortem** des PRs mergees de la fenetre, rejouant le diff pour chacune avec SA base et SON body (option 1 de #19101). Cable comme etape du workflow ; l'etape existante n'est pas touchee. **Aucun `pull_request` n'est reintroduit** sur ce workflow : c'est la contrainte explicite de l'issue (le cout du clone par PR est la motivation d'origine de #12817). Le compromis -- detecter les pertes passees, pas proteger le merge -- est ecrit dans le module, pas seulement ici. ## Le piege trouve en mesurant Premiere mesure sur 24 h : **6 « erreurs de diff »** sur 38 PRs. Instruites, elles venaient toutes du meme cas : `git show :` sort en **128** parce que le carnet n'est pas a ce chemin dans la base. Ce n'etait pas cosmetique. `credited_diff_errors > 0` **bloque** la pose du label (#18761) : une erreur structurelle sur un carnet empechait la mesure reelle des carnets modifies de la meme PR. Un faux positif d'erreur produisait un faux zero de pertes. Trois cas separes, qui ne disent pas la meme chose : - `ADDED` : neuf, **rien a perdre** par construction (6 des 6 erreurs) ; - `RENAMED` : la base est a un **autre chemin**, que `gh pr view --json files` n'expose pas (`previousFilename` absent). Non comparable -- et contrairement a `ADDED`, un renommage **peut** perdre des exemples : declare NON MESURE, jamais « sans perte » ; - `DELETED` : exclu, comme le `--diff-filter=d` du workflow. Les carnets non mesurables sont **nommes** dans le rapport au lieu de disparaitre : un carnet tu se lirait comme un carnet conforme. ## Mesure Fenetre de 24 h sur `main`, `--json`, sans `--apply`. Un diff en erreur n'est PAS un zero mesure (#18761) : le rapport separe « pertes non exemptees = 0 » de « carnets non mesurables », et n'affirme jamais une couverture que la mesure ne porte pas. ## Tests `test_credited_examples_sweep.py` : tout hors ligne (reseau et git injectes). Le lot de l'API de recherche **au plafond** leve au lieu de passer pour un compte ; le label ne se pose que si **tous** les diffs ont reussi ; un `merge-base` indisponible est nomme ; un body illisible est nomme et la PR ecartee ; un `ADDED` n'est pas mesure ; un `RENAMED` est declare non mesure ET ne bloque plus les carnets modifies de la meme PR. Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/exercises-advisory.yml | 23 ++ .../notebook_tools/credited_examples_sweep.py | 300 ++++++++++++++++++ scripts/tests/test_credited_examples_sweep.py | 238 ++++++++++++++ 3 files changed, 561 insertions(+) create mode 100644 scripts/notebook_tools/credited_examples_sweep.py create mode 100644 scripts/tests/test_credited_examples_sweep.py diff --git a/.github/workflows/exercises-advisory.yml b/.github/workflows/exercises-advisory.yml index fbb9adc9f3..517a956658 100644 --- a/.github/workflows/exercises-advisory.yml +++ b/.github/workflows/exercises-advisory.yml @@ -252,3 +252,26 @@ jobs: echo "RESULT: all in-corpus modified notebooks meet their threshold (verified)." fi exit 0 + + - name: Credited-examples sweep over the day's merged PRs (nocturne) + # #19101 -- l'etape precedente porte la branche « exemples credites » + # de #18761, mais elle est DORMANTE sous `schedule` : le diff exige + # `--base` ET `--pr-body-file`, et le mode nocturne n'a pas de contexte + # PR, donc pas de body. Le test de l'etape (l.136) echouait, la branche + # `else` sans `--base` etait prise, et le diff n'etait **jamais** + # mesure : le label `credited-examples-lost` ne pouvait pas etre pose. + # + # Ce balayage le reveille SANS reintroduire `pull_request` sur ce + # workflow (interdit : le clone par PR est la motivation de #12817 + # tranche 1). Il rejoue le diff apres coup, PR par PR, avec SA base et + # SON body -- option 1 de #19101, assumee post-mortem. + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + run: | + set -uo pipefail + # Advisory : un balayage qui echoue NOMME sa panne et ne rougit pas + # le run (le contrat de cet organe porte sur le verdict). Le repli et + # les erreurs de diff sont imprimes, jamais tus (#18761). + python scripts/notebook_tools/credited_examples_sweep.py \ + --hours 24 --apply || echo "::warning::credited sweep failed -- nothing labelled" diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py new file mode 100644 index 0000000000..d2aba25408 --- /dev/null +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -0,0 +1,300 @@ +#!/usr/bin/env python3 +r"""credited_examples_sweep.py -- pose POST-MORTEM du label `credited-examples-lost`. + +## Pourquoi (#19101) + +`exercises-advisory.yml` porte une branche « exemples credites » posee par +#18761, mais **dormante** : elle exige `--base` ET `--pr-body-file`. Sous +`schedule` -- le seul declencheur qui subsiste apres la tranche 1 de #12817 -- +il n'y a pas de contexte PR, donc pas de body, donc pas de `--base` : le diff +des exemples credites n'etait **jamais** mesure et le label ne pouvait pas +etre pose. + +Ce balayage le reveille **sans reintroduire `pull_request`** sur ce workflow +(interdit : le clone par PR etait la motivation de #12817). Il tourne apres +coup : il liste les PRs MERGEES de la fenetre et rejoue, pour chacune, le diff +avec SA base et SON body -- c'est l'option 1 de #19101. + +## Portee honnete + +Post-mortem : il detecte les pertes **passees**, il ne protege pas le merge. +C'est le compromis assume de l'option 1 ; l'option 2 (un declencheur PR leger) +reste ouverte et n'est pas traitee ici. + +Deux limites de la mesure, nommees plutot que tues : + + - la version HEAD est lue dans l'**arbre de travail** (defaut de + `check_notebooks`), donc une PR dont un carnet a ete re-touche par une PR + ulterieure est mesuree contre l'etat courant, pas contre son propre head. + Le chiffre reste une borne basse exploitable ; il n'est pas presente comme + « exactement l'apport de cette PR ». + - quand `headRefOid` n'est plus atteignable (ref supprimee au merge), le + merge-base est indisponible : la PR est alors mesuree **contre sa base + declaree** et le repli est imprime en `[WARN]`, jamais silencieux. + +## Usage + + python scripts/notebook_tools/credited_examples_sweep.py --hours 24 + python scripts/notebook_tools/credited_examples_sweep.py --hours 24 --apply + +Sans `--apply`, rien n'est pose : le rapport dit ce qui **serait** labellise. +""" +from __future__ import annotations + +import argparse +import datetime as dt +import json +import subprocess +import sys +from pathlib import Path + +_TOOLS_DIR = Path(__file__).resolve().parent +if str(_TOOLS_DIR) not in sys.path: + sys.path.insert(0, str(_TOOLS_DIR)) + +from check_pr_exercises import LABEL_NAME, check_notebooks # noqa: E402 + +REPO_DEFAULT = "jsboige/CoursIA" +LABEL_CREDITED_LOST = "credited-examples-lost" + +# Le plafond de l'API de recherche. Un lot qui l'atteint n'est pas un compte : +# c'est le plafond, et il cache les plus anciens (#19209, meme classe). Une +# fenetre qui sature est une erreur, jamais un corpus tronque qui a l'air +# complet. +SEARCH_RESULT_CAP = 1000 + + +def _run(argv: list[str], **kw) -> subprocess.CompletedProcess: + return subprocess.run( + argv, capture_output=True, text=True, encoding="utf-8", + errors="replace", check=False, **kw, + ) + + +def _gh_json(argv: list[str]) -> object: + proc = _run(["gh", *argv]) + if proc.returncode != 0: + raise RuntimeError( + f"gh failed ({proc.returncode}): " + f"{proc.stderr.strip() or proc.stdout.strip()}" + ) + if not proc.stdout.strip(): + return None + return json.loads(proc.stdout) + + +def merged_prs(repo: str, since: dt.datetime, run=_gh_json) -> list[dict]: + """PRs mergees depuis `since`, avec leurs fichiers et leurs deux refs. + + Leve si le lot atteint le plafond de l'API de recherche : un corpus + tronque se lirait comme un corpus complet, et les PRs perdues sont les + plus anciennes de la fenetre. + """ + stamp = since.strftime("%Y-%m-%dT%H:%M:%SZ") + rows = run([ + "pr", "list", "--repo", repo, "--state", "merged", + "--limit", str(SEARCH_RESULT_CAP), + "--search", f"merged:>={stamp}", + "--json", "number,baseRefOid,headRefOid,files,mergedAt", + ]) or [] + if len(rows) >= SEARCH_RESULT_CAP: + raise RuntimeError( + f"la fenetre depuis {stamp} rend {len(rows)} PRs, au plafond de " + f"l'API de recherche ({SEARCH_RESULT_CAP}) : le corpus serait " + "tronque par les plus anciennes. Reduire --hours." + ) + return rows + + +def _ipynb_by_change(pr: dict) -> tuple[list[str], list[str], list[str]]: + """`(modifies, ajoutes, renommes)` parmi les `.ipynb` de la PR. + + Deux exclusions et une separation, toutes structurelles. Toutes viennent + d'un `git show :` qui sort en **128** parce que le carnet + n'est pas a ce chemin dans la base -- ce qui n'est PAS une erreur de + mesure, mais ne veut pas dire la meme chose selon le cas : + + - `DELETED` : supprime, plus rien a compter (meme regle que le + `--diff-filter=d` du workflow) ; + - `ADDED` : neuf, donc **rien a perdre** par construction. Mesure du + 2026-10-05 : 6 des 6 « erreurs de diff » du corpus de 24 h venaient de + la (#18968, #19037, #19020, #19007, #18986) ; + - `RENAMED` : la version de base existe **sous un autre chemin**, que + `gh pr view --json files` n'expose pas ici (`previousFilename` absent). + On ne peut donc pas la comparer -- et contrairement a `ADDED`, un + renommage **peut** perdre des exemples. On le declare NON MESURE. + + Pourquoi separer plutot que compter en erreur : `credited_diff_errors > 0` + **bloque** la pose du label (#18761). Une erreur structurelle sur un carnet + empechait donc la mesure reelle des carnets modifies de la meme PR -- un + faux positif d'erreur produisait un faux zero de pertes. + """ + modified, added, renamed = [], [], [] + for f in (pr.get("files") or []): + path = f.get("path") or "" + if not path.endswith(".ipynb"): + continue + change = f.get("changeType") or "MODIFIED" + if change == "DELETED": + continue + if change == "ADDED": + added.append(path) + elif change == "RENAMED": + renamed.append(path) + else: + modified.append(path) + return modified, added, renamed + + +def ipynb_paths(pr: dict) -> list[str]: + """Chemins `.ipynb` **modifies** par la PR (cf. `_ipynb_by_change`).""" + return _ipynb_by_change(pr)[0] + + +def merge_base(repo_dir: Path, base_ref: str, head_ref: str) -> str | None: + """`git merge-base` des deux refs, ou None si l'une n'est pas atteignable. + + Pour une PR deja mergee et squash-mergee, `headRefOid` peut avoir disparu + du depot (ref supprimee). On rend None et l'appelant NOMME le repli -- + jamais un diff silencieusement mesure contre la mauvaise base. + """ + proc = _run(["git", "-C", str(repo_dir), "merge-base", base_ref, head_ref]) + if proc.returncode != 0: + return None + return proc.stdout.strip() or None + + +def pr_body(repo: str, number: int, run=_gh_json) -> str: + payload = run(["pr", "view", str(number), "--repo", repo, "--json", "body"]) + return ((payload or {}).get("body") or "") if isinstance(payload, dict) else "" + + +def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, + *, fetch_merged=merged_prs, fetch_body=pr_body, + base_of=merge_base, check=check_notebooks) -> tuple[list[dict], list[str]]: + """Rejoue le diff des exemples credites pour chaque PR mergee de la fenetre. + + Rend `(lignes, erreurs)`. Une ligne porte, par PR : le nombre de pertes non + exemptees et si le label serait pose. Un diff en erreur n'est JAMAIS + presente comme un zero mesure (#18761) : il est compte a part. + """ + since = now - dt.timedelta(hours=hours) + rows: list[dict] = [] + errors: list[str] = [] + for pr in fetch_merged(repo, since): + number = pr.get("number") + paths, added, renamed = _ipynb_by_change(pr) + if not paths and not added and not renamed: + continue + if not paths: + # PR sans carnet mesurable : rien a perdre (ajouts) ou rien de + # comparable (renommages). On la NOMME plutot que de la faire + # disparaitre du rapport. + rows.append({ + "number": number, + "notebooks": 0, + "added_notebooks": len(added), + "renamed_notebooks": len(renamed), + "credited_lost_unexempted": 0, + "credited_diff_errors": 0, + "would_label": False, + "paths": [], + }) + continue + base_ref = pr.get("baseRefOid") or "" + head_ref = pr.get("headRefOid") or "" + base = base_of(repo_dir, base_ref, head_ref) if base_ref and head_ref else None + if base is None: + # Repli nomme : on mesure contre la ref de base declaree, sans + # merge-base. Le chiffre reste exploitable, sa provenance est dite. + errors.append( + f"#{number}: merge-base indisponible pour " + f"{base_ref[:8]}...{head_ref[:8]} -- diff mesure contre la base declaree" + ) + base = base_ref + try: + body = fetch_body(repo, number) + except RuntimeError as exc: + errors.append(f"#{number}: body illisible ({exc})") + continue + result = check([Path(p) for p in paths], base_ref=base, pr_body=body) + summary = result.as_payload().get("summary", {}) + lost = int(summary.get("credited_lost_unexempted", 0) or 0) + diff_errors = int(summary.get("credited_diff_errors", 0) or 0) + rows.append({ + "number": number, + "notebooks": len(paths), + "added_notebooks": len(added), + "renamed_notebooks": len(renamed), + "credited_lost_unexempted": lost, + "credited_diff_errors": diff_errors, + # #18761 : le label ne se pose QUE si tous les diffs ont reussi. + "would_label": lost > 0 and diff_errors == 0, + "paths": paths, + }) + return rows, errors + + +def apply_label(repo: str, number: int, run=_run) -> bool: + """Pose le label sur la PR (idempotent cote GitHub). Rend True si pose.""" + proc = run([ + "gh", "pr", "edit", str(number), "--repo", repo, + "--add-label", LABEL_CREDITED_LOST, + ]) + return proc.returncode == 0 + + +def main(argv: list[str] | None = None) -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("--repo", default=REPO_DEFAULT) + ap.add_argument("--repo-dir", default=".") + ap.add_argument("--hours", type=int, default=24) + ap.add_argument("--apply", action="store_true", + help="poser reellement le label (defaut: rapport seul)") + ap.add_argument("--json", action="store_true") + args = ap.parse_args(argv) + + now = dt.datetime.now(dt.timezone.utc) + try: + rows, errors = sweep(args.repo, Path(args.repo_dir), args.hours, now) + except RuntimeError as exc: + print(f"credited_examples_sweep: {exc}", file=sys.stderr) + return 2 + + labelled: list[int] = [] + if args.apply: + for row in rows: + if row["would_label"] and apply_label(args.repo, row["number"]): + labelled.append(row["number"]) + + if args.json: + json.dump({"prs": rows, "errors": errors, "labelled": labelled}, + sys.stdout, ensure_ascii=False) + print() + return 0 + + print(f"Fenetre : {args.hours} h -- {len(rows)} PR(s) mergee(s) touchant un .ipynb") + for row in rows: + mark = "LABEL" if row["would_label"] else " - " + added = row.get("added_notebooks", 0) + renamed = row.get("renamed_notebooks", 0) + suffix = "" + if added: + suffix += f" (+{added} neuf(s), rien a perdre)" + if renamed: + suffix += f" ({renamed} renomme(s), NON MESURE(S) : base a un autre chemin)" + print(f" [{mark}] #{row['number']}: {row['notebooks']} carnet(s) modifie(s), " + f"pertes non exemptees={row['credited_lost_unexempted']}, " + f"erreurs de diff={row['credited_diff_errors']}{suffix}") + for err in errors: + print(f" [WARN] {err}", file=sys.stderr) + total_lost = sum(r["credited_lost_unexempted"] for r in rows if r["would_label"]) + total_err = sum(r["credited_diff_errors"] for r in rows) + print(f"RESULT: {len(labelled) if args.apply else sum(1 for r in rows if r['would_label'])} " + f"PR(s) a labelliser, {total_lost} perte(s) non exemptee(s), " + f"{total_err} erreur(s) de diff, {len(errors)} repli(s) nomme(s)") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py new file mode 100644 index 0000000000..189f2e48f7 --- /dev/null +++ b/scripts/tests/test_credited_examples_sweep.py @@ -0,0 +1,238 @@ +#!/usr/bin/env python3 +"""Tests for `scripts/notebook_tools/credited_examples_sweep.py` (#19101). + +Le balayage est concu pour etre injectable : `sweep()` prend ses appels +reseau et git en parametres, donc tout se teste hors ligne. Les controles +epinglent les trois pieges qui rendraient le balayage trompeur : + + - un lot qui ATTEINT le plafond de l'API de recherche n'est pas un compte ; + - le label ne se pose que si TOUS les diffs ont reussi (#18761) ; + - un merge-base indisponible est NOMME, jamais un diff silencieux contre la + mauvaise base. +""" +from __future__ import annotations + +import datetime as dt +import importlib.util +import sys +from pathlib import Path + +_SCRIPT = Path(__file__).resolve().parent.parent / "notebook_tools" / "credited_examples_sweep.py" +_spec = importlib.util.spec_from_file_location("credited_examples_sweep", _SCRIPT) +assert _spec and _spec.loader, f"could not load {_SCRIPT}" +_mod = importlib.util.module_from_spec(_spec) +sys.modules["credited_examples_sweep"] = _mod +_spec.loader.exec_module(_mod) + +NOW = dt.datetime(2026, 10, 5, 12, 0, tzinfo=dt.timezone.utc) + + +class _FakeResult: + def __init__(self, lost=0, diff_errors=0): + self._s = { + "credited_lost_unexempted": lost, + "credited_diff_errors": diff_errors, + } + + def as_payload(self): + return {"summary": self._s} + + +def _pr(number, files=None, base="a" * 40, head="b" * 40): + return { + "number": number, + "baseRefOid": base, + "headRefOid": head, + "files": files or [], + "mergedAt": NOW.isoformat(), + } + + +def _nb(path, change="MODIFIED"): + return {"path": path, "changeType": change} + + +IPY = "MyIA.AI.Notebooks/Search/Part1/x.ipynb" +MD = "MyIA.AI.Notebooks/Search/Part1/README.md" + + +def _run_sweep(prs): + """Drive `sweep` with the network and git fully injected (offline).""" + return _mod.sweep( + "o/r", Path("."), 24, NOW, + fetch_merged=lambda repo, since: prs, + fetch_body=lambda repo, number: "", + base_of=lambda repo_dir, b, h: "deadbeef", + check=lambda paths, base_ref="", head_ref="", pr_body="": _FakeResult(), + ) + + +class TestFiltering: + def test_only_ipynb_paths_are_kept(self): + pr = _pr(1, [_nb(IPY), _nb(MD)]) + assert _mod.ipynb_paths(pr) == [IPY] + + def test_deleted_notebooks_are_excluded(self): + """Un carnet supprime n'a plus rien a compter (--diff-filter=d).""" + pr = _pr(1, [_nb(IPY, "DELETED")]) + assert _mod.ipynb_paths(pr) == [] + + def test_added_notebooks_are_not_measured(self): + """Un carnet neuf n'existe pas dans la base : `git show` sort en 128. + + Ce n'est PAS une erreur de mesure (#18761) -- et la compter comme telle + bloquait le label pour les carnets MODIFIES de la meme PR. + """ + pr = _pr(1, [_nb(IPY, "ADDED")]) + assert _mod.ipynb_paths(pr) == [] + modified, added, renamed = _mod._ipynb_by_change(pr) + assert modified == [] and added == [IPY] and renamed == [] + + def test_added_and_modified_split_in_the_same_pr(self): + other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + pr = _pr(1, [_nb(other, "ADDED"), _nb(IPY, "MODIFIED")]) + modified, added, renamed = _mod._ipynb_by_change(pr) + assert modified == [IPY] and added == [other] and renamed == [] + + def test_a_purely_additive_pr_is_named_not_dropped(self): + rows, errs = _run_sweep([_pr(1, [_nb(IPY, "ADDED")])]) + assert len(rows) == 1 and rows[0]["notebooks"] == 0 + assert rows[0]["added_notebooks"] == 1 + assert rows[0]["would_label"] is False + assert errs == [] + + def test_a_renamed_notebook_is_declared_unmeasured_not_lost_free(self): + """Un renommage PEUT perdre des exemples : on ne le dit pas « sans perte ». + + La base est a un autre chemin (`previousFilename` non expose par + `gh pr view --json files`), donc la comparaison est impossible. Le + carnet ne doit ni etre mesure contre le mauvais chemin, ni etre tu. + """ + pr = _pr(1, [_nb(IPY, "RENAMED")]) + assert _mod.ipynb_paths(pr) == [] + modified, added, renamed = _mod._ipynb_by_change(pr) + assert modified == [] and added == [] and renamed == [IPY] + rows, errs = _run_sweep([pr]) + assert len(rows) == 1 and rows[0]["renamed_notebooks"] == 1 + assert rows[0]["notebooks"] == 0 + assert rows[0]["would_label"] is False + + def test_a_renamed_notebook_does_not_block_the_other_notebooks(self): + """Le faux positif d'erreur produisait un faux zero de pertes (#18761). + + Un carnet renomme dans la meme PR ne doit plus empecher la mesure des + carnets modifies -- c'est le defaut que la separation corrige. + """ + other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + pr = _pr(1, [_nb(other, "RENAMED"), _nb(IPY, "MODIFIED")]) + + def check(paths, base_ref="", head_ref="", pr_body=""): + assert [str(p).endswith("x.ipynb") for p in paths] == [True], paths + return _FakeResult(lost=2, diff_errors=0) + + rows, errs = _mod.sweep( + "o/r", Path("."), 24, NOW, + fetch_merged=lambda repo, since: [pr], + fetch_body=lambda repo, number: "", + base_of=lambda *a: "deadbeef", + check=check, + ) + assert rows[0]["would_label"] is True, rows + assert rows[0]["renamed_notebooks"] == 1 + assert errs == [] + + def test_a_pr_without_notebook_is_skipped_by_the_sweep(self): + rows, errs = _run_sweep([_pr(1, [_nb(MD)])]) + assert rows == [] and errs == [] + + def test_modified_notebook_is_kept(self): + pr = _pr(1, [_nb(IPY, "MODIFIED")]) + assert _mod.ipynb_paths(pr) == [IPY] + + +class TestSearchCapIsNotACount: + def test_a_batch_at_the_cap_raises(self): + def run(argv): + return [{} for _ in range(_mod.SEARCH_RESULT_CAP)] + + try: + _mod.merged_prs("o/r", NOW - dt.timedelta(hours=24), run=run) + except RuntimeError as exc: + assert "plafond" in str(exc) + else: + raise AssertionError("un lot au plafond doit lever, pas passer") + + def test_a_batch_under_the_cap_passes(self): + rows = _mod.merged_prs("o/r", NOW - dt.timedelta(hours=24), + run=lambda argv: [{"number": 1}]) + assert rows == [{"number": 1}] + + def test_the_window_is_asked_on_merge_time(self): + seen = {} + + def run(argv): + seen["argv"] = argv + return [] + + _mod.merged_prs("o/r", NOW - dt.timedelta(hours=24), run=run) + argv = seen["argv"] + assert "--search" in argv + assert any(a.startswith("merged:>=") for a in argv), argv + assert argv[argv.index("--limit") + 1] == str(_mod.SEARCH_RESULT_CAP) + + +class TestSweepVerdicts: + def _one(self, lost, diff_errors, *, bases="ok"): + pr = _pr(1, [_nb(IPY)]) + + def fetch_merged(repo, since): + return [pr] + + def fetch_body(repo, number): + return "" + + def base_of(repo_dir, b, h): + return "deadbeef" if bases == "ok" else None + + def check(paths, base_ref="", head_ref="", pr_body=""): + return _FakeResult(lost=lost, diff_errors=diff_errors) + + return _mod.sweep("o/r", Path("."), 24, NOW, + fetch_merged=fetch_merged, fetch_body=fetch_body, + base_of=base_of, check=check) + + def test_a_loss_with_clean_diffs_is_labelled(self): + rows, errs = self._one(2, 0) + assert rows[0]["would_label"] is True + assert errs == [] + + def test_a_broken_diff_never_labels_even_with_a_loss(self): + """#18761 : un diff casse n'est pas un zero mesure, ni un label.""" + rows, errs = self._one(3, 1) + assert rows[0]["would_label"] is False + assert rows[0]["credited_lost_unexempted"] == 3 + + def test_zero_loss_does_not_label(self): + rows, _ = self._one(0, 0) + assert rows[0]["would_label"] is False + + def test_a_missing_merge_base_is_named_and_the_pr_is_still_measured(self): + rows, errs = self._one(1, 0, bases="missing") + assert len(errs) == 1 and "merge-base" in errs[0] + assert rows[0]["would_label"] is True + + def test_an_unreadable_body_is_named_and_the_pr_is_skipped(self): + pr = _pr(1, [_nb(IPY)]) + + def boom(repo, number): + raise RuntimeError("gh failed (1): not found") + + rows, errs = _mod.sweep( + "o/r", Path("."), 24, NOW, + fetch_merged=lambda repo, since: [pr], + fetch_body=boom, + base_of=lambda *a: "deadbeef", + check=lambda *a, **k: _FakeResult(1, 0), + ) + assert rows == [] + assert len(errs) == 1 and "body illisible" in errs[0] From 5cdd314bcee8d89a2959da2599ba308a378dc2e6 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 08:58:21 +0200 Subject: [PATCH 2/6] =?UTF-8?q?fix(ci,#19101):=20sweep=20credited=20?= =?UTF-8?q?=E2=80=94=20changeType=20GraphQL,=20tete=20de=20PR,=20echec=20n?= =?UTF-8?q?on=20masque?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reponse a la review CHANGES_REQUESTED du 05/10 (5 conditions) : 1. changeType n'existe pas dans gh pr list --json files (mesure gh 2.83.2 sur #19040 : {additions, deletions, path} seulement). Les fichiers et leur nature viennent maintenant de GraphQL (pullRequest.files.nodes {path changeType}), pagines au curseur -- le repli « tout MODIFIED » classait GameTheory-18d (supprime par #19040) en modifie et levait FileNotFoundError, hors de tout garde. 2. check_notebooks recoit head_ref=headRefOid : le cote « apres » est la tete de la PR, pas l'arbre du moment du balayage. Le commit est amene par fetch-by-SHA s'il manque (PR squash-mergee) ; inatteignable = erreur nommee, PR ecartee -- jamais mesuree contre l'arbre du jour. 3. Tests : 17 -> 27. La forme reelle du lot (sans files/changeType) est desormais un fixture ; carnet supprime sans crash + carnets modifies de la meme PR toujours mesures ; pagination >100 fichiers ; tete inatteignable ; propagation de head_ref au check. Falsification : 3 mutants (branche DELETED retiree, head_ref non passe, pagination coupee) -> chacun cuche par au moins un test ; restore vert 27/27. 4. Le masque || echo "::warning::..." du workflow est retire : un plantage permanent laissait le run vert chaque nuit (classe de defaut #19214). set -euo pipefail ; sans effet merge (schedule/dispatch seulement). 5. Rejou de la mesure 24 h sur le corpus reel : dans le corps de la PR. Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/exercises-advisory.yml | 16 +- .../notebook_tools/credited_examples_sweep.py | 156 +++++++--- scripts/tests/test_credited_examples_sweep.py | 279 +++++++++++++----- 3 files changed, 337 insertions(+), 114 deletions(-) diff --git a/.github/workflows/exercises-advisory.yml b/.github/workflows/exercises-advisory.yml index 517a956658..bdf12d4ae3 100644 --- a/.github/workflows/exercises-advisory.yml +++ b/.github/workflows/exercises-advisory.yml @@ -269,9 +269,15 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} GH_REPO: ${{ github.repository }} run: | - set -uo pipefail - # Advisory : un balayage qui echoue NOMME sa panne et ne rougit pas - # le run (le contrat de cet organe porte sur le verdict). Le repli et - # les erreurs de diff sont imprimes, jamais tus (#18761). + set -euo pipefail + # Le masque `|| echo "::warning::..."` est RETIRE (review #19215, + # point 3) : un plantage permanent laissait le run vert chaque nuit + # -- exactement la classe de defaut de #19214 (35 jours verts sans + # rapport). Le contrat de l'organe reste : ses pannes ATTENDUES + # (refs absentes, body illisible, plafond atteint) sont nommees en + # [WARN] et le script sort 0 ; ce qui ROUGIT ici est l'imprevu + # (traceback, crash) -- jamais masque. Sans effet merge : la mesure + # du body de #19214 vaut aussi pour ce workflow (schedule/dispatch + # seulement, hors du perimetre `merge_dwell`). python scripts/notebook_tools/credited_examples_sweep.py \ - --hours 24 --apply || echo "::warning::credited sweep failed -- nothing labelled" + --hours 24 --apply diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index d2aba25408..2eee6a01f2 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -7,30 +7,38 @@ #18761, mais **dormante** : elle exige `--base` ET `--pr-body-file`. Sous `schedule` -- le seul declencheur qui subsiste apres la tranche 1 de #12817 -- il n'y a pas de contexte PR, donc pas de body, donc pas de `--base` : le diff -des exemples credites n'etait **jamais** mesure et le label ne pouvait pas -etre pose. +des exemples credites n'etait **jamais** mesure et le label ne pouvait pas etre +pose. Ce balayage le reveille **sans reintroduire `pull_request`** sur ce workflow (interdit : le clone par PR etait la motivation de #12817). Il tourne apres coup : il liste les PRs MERGEES de la fenetre et rejoue, pour chacune, le diff avec SA base et SON body -- c'est l'option 1 de #19101. +## Deux sources de verite, chacune du bon cote (review 04:53Z) + +- **`changeType` n'existe pas dans `gh pr list --json files`** (mesure gh + 2.83.2 : cette forme ne rend que `{additions, deletions, path}`). Le + classifieur ADDED/DELETED/RENAMED se nourrit donc de **GraphQL** + (`pullRequest.files.nodes { path changeType }`), pagine au curseur. Un + carnet supprime (#19040 a supprime GameTheory-18d) fait sinon planter le + balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la premiere + version classait tout en modifie. +- **le cote « apres » est la tete de la PR, pas l'arbre de travail** : + `check_notebooks` recoit `head_ref=headRefOid`, et le commit est amene + localement s'il manque (PR squash-mergee : l'objet n'est pas dans `main`). + Un commit de tete inatteignable est **nomme**, jamais remplace par l'etat + du jour. + ## Portee honnete Post-mortem : il detecte les pertes **passees**, il ne protege pas le merge. C'est le compromis assume de l'option 1 ; l'option 2 (un declencheur PR leger) reste ouverte et n'est pas traitee ici. -Deux limites de la mesure, nommees plutot que tues : - - - la version HEAD est lue dans l'**arbre de travail** (defaut de - `check_notebooks`), donc une PR dont un carnet a ete re-touche par une PR - ulterieure est mesuree contre l'etat courant, pas contre son propre head. - Le chiffre reste une borne basse exploitable ; il n'est pas presente comme - « exactement l'apport de cette PR ». - - quand `headRefOid` n'est plus atteignable (ref supprimee au merge), le - merge-base est indisponible : la PR est alors mesuree **contre sa base - declaree** et le repli est imprime en `[WARN]`, jamais silencieux. +Quand `headRefOid` n'est plus atteignable (ref supprimee au merge), le +merge-base est indisponible : la PR est alors mesuree **contre sa base +declaree** et le repli est imprime en `[WARN]`, jamais silencieux. ## Usage @@ -63,6 +71,20 @@ # complet. SEARCH_RESULT_CAP = 1000 +# La liste des fichiers d'une PR AVEC leur nature de changement. Cette +# information n'existe pas dans `gh pr list --json files` (review 04:53Z, +# mesure gh 2.83.2) : elle est lue par GraphQL, page par 100. +GRAPHQL_FILES = """query($owner: String!, $name: String!, $num: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $num) { + files(first: 100, after: $cursor) { + nodes { path changeType } + pageInfo { hasNextPage endCursor } + } + } + } +}""" + def _run(argv: list[str], **kw) -> subprocess.CompletedProcess: return subprocess.run( @@ -84,18 +106,19 @@ def _gh_json(argv: list[str]) -> object: def merged_prs(repo: str, since: dt.datetime, run=_gh_json) -> list[dict]: - """PRs mergees depuis `since`, avec leurs fichiers et leurs deux refs. + """PRs mergees depuis `since`, avec leurs deux refs -- PAS leurs fichiers. - Leve si le lot atteint le plafond de l'API de recherche : un corpus - tronque se lirait comme un corpus complet, et les PRs perdues sont les - plus anciennes de la fenetre. + La forme `gh pr list --json files` ne rend pas `changeType` : les fichiers + (et leur nature) sont lus par `pr_files`, par PR, en GraphQL. Leve si le + lot atteint le plafond de l'API de recherche : un corpus tronque se + lirait comme un corpus complet. """ stamp = since.strftime("%Y-%m-%dT%H:%M:%SZ") rows = run([ "pr", "list", "--repo", repo, "--state", "merged", "--limit", str(SEARCH_RESULT_CAP), "--search", f"merged:>={stamp}", - "--json", "number,baseRefOid,headRefOid,files,mergedAt", + "--json", "number,baseRefOid,headRefOid,mergedAt", ]) or [] if len(rows) >= SEARCH_RESULT_CAP: raise RuntimeError( @@ -106,23 +129,48 @@ def merged_prs(repo: str, since: dt.datetime, run=_gh_json) -> list[dict]: return rows -def _ipynb_by_change(pr: dict) -> tuple[list[str], list[str], list[str]]: +def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: + """Fichiers de la PR avec `changeType`, par GraphQL, pagine au curseur. + + `gh pr view --json files` (REST/CLI) ne rend que des decomptes : la nature + du changement n'existe que cote GraphQL. Pages de 100, boucle sur + `pageInfo.endCursor` -- une PR de carnet peut deplacer plus de 100 fichiers + (mesure : #19040 en deplace 2876+214 lignes sur plusieurs carnets). + """ + owner, name = repo.split("/", 1) + nodes: list[dict] = [] + cursor = None + while True: + payload = run([ + "api", "graphql", "-f", f"query={GRAPHQL_FILES}", + "-F", f"owner={owner}", "-F", f"name={name}", + "-F", f"num={number}", + ] + (["-f", f"cursor={cursor}"] if cursor else [])) + conn = (payload or {}).get("data", {}).get("repository", {}) \ + .get("pullRequest", {}).get("files", {}) + nodes.extend(conn.get("nodes") or []) + page = conn.get("pageInfo") or {} + if not page.get("hasNextPage"): + return nodes + cursor = page.get("endCursor") + + +def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str]]: """`(modifies, ajoutes, renommes)` parmi les `.ipynb` de la PR. - Deux exclusions et une separation, toutes structurelles. Toutes viennent - d'un `git show :` qui sort en **128** parce que le carnet - n'est pas a ce chemin dans la base -- ce qui n'est PAS une erreur de - mesure, mais ne veut pas dire la meme chose selon le cas : + Deux exclusions et une separation, toutes structurelles, toutes lues de + `changeType` (GraphQL) : - `DELETED` : supprime, plus rien a compter (meme regle que le - `--diff-filter=d` du workflow) ; - - `ADDED` : neuf, donc **rien a perdre** par construction. Mesure du - 2026-10-05 : 6 des 6 « erreurs de diff » du corpus de 24 h venaient de - la (#18968, #19037, #19020, #19007, #18986) ; + `--diff-filter=d` du workflow). C'est le cas qui FAIT PLANTER le + balayage quand on le croit modifie : `git show :` puis + l'ouverture du carnet levait `FileNotFoundError` sur GameTheory-18d + (supprime par #19040) ; + - `ADDED` : neuf, donc **rien a perdre** par construction ; - `RENAMED` : la version de base existe **sous un autre chemin**, que - `gh pr view --json files` n'expose pas ici (`previousFilename` absent). - On ne peut donc pas la comparer -- et contrairement a `ADDED`, un - renommage **peut** perdre des exemples. On le declare NON MESURE. + cette source n'expose pas (`previousFilename` absent du jeu GraphQL + demande ici). On ne peut donc pas la comparer -- et contrairement a + `ADDED`, un renommage **peut** perdre des exemples. NON MESURE. Pourquoi separer plutot que compter en erreur : `credited_diff_errors > 0` **bloque** la pose du label (#18761). Une erreur structurelle sur un carnet @@ -130,7 +178,7 @@ def _ipynb_by_change(pr: dict) -> tuple[list[str], list[str], list[str]]: faux positif d'erreur produisait un faux zero de pertes. """ modified, added, renamed = [], [], [] - for f in (pr.get("files") or []): + for f in (files or []): path = f.get("path") or "" if not path.endswith(".ipynb"): continue @@ -146,15 +194,15 @@ def _ipynb_by_change(pr: dict) -> tuple[list[str], list[str], list[str]]: return modified, added, renamed -def ipynb_paths(pr: dict) -> list[str]: +def ipynb_paths(files: list[dict]) -> list[str]: """Chemins `.ipynb` **modifies** par la PR (cf. `_ipynb_by_change`).""" - return _ipynb_by_change(pr)[0] + return _ipynb_by_change(files)[0] def merge_base(repo_dir: Path, base_ref: str, head_ref: str) -> str | None: """`git merge-base` des deux refs, ou None si l'une n'est pas atteignable. - Pour une PR deja mergee et squash-mergee, `headRefOid` peut avoir disparu + Pour une PR deja mergeee et squash-mergee, `headRefOid` peut avoir disparu du depot (ref supprimee). On rend None et l'appelant NOMME le repli -- jamais un diff silencieusement mesure contre la mauvaise base. """ @@ -164,14 +212,31 @@ def merge_base(repo_dir: Path, base_ref: str, head_ref: str) -> str | None: return proc.stdout.strip() or None +def ensure_commit(repo_dir: Path, sha: str, run=_run) -> bool: + """Ameine localement le commit de tete s'il manque, puis confirme. + + Une PR squash-mergee n'est pas un ancetre de `main` : son `headRefOid` + n'est pas dans un clone frais. GitHub autorise le fetch d'un SHA atteignable + depuis une ref de PR (`git fetch origin `). Rend False si le commit + reste inatteignable -- l'appelant NOMME l'echec, il ne mesure pas contre + un autre arbre. + """ + probe = ["git", "-C", str(repo_dir), "cat-file", "-e", f"{sha}^{{commit}}"] + if run(probe).returncode == 0: + return True + run(["git", "-C", str(repo_dir), "fetch", "--quiet", "origin", sha]) + return run(probe).returncode == 0 + + def pr_body(repo: str, number: int, run=_gh_json) -> str: payload = run(["pr", "view", str(number), "--repo", repo, "--json", "body"]) return ((payload or {}).get("body") or "") if isinstance(payload, dict) else "" def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, - *, fetch_merged=merged_prs, fetch_body=pr_body, - base_of=merge_base, check=check_notebooks) -> tuple[list[dict], list[str]]: + *, fetch_merged=merged_prs, fetch_files=pr_files, + fetch_body=pr_body, base_of=merge_base, ensure=ensure_commit, + check=check_notebooks) -> tuple[list[dict], list[str]]: """Rejoue le diff des exemples credites pour chaque PR mergee de la fenetre. Rend `(lignes, erreurs)`. Une ligne porte, par PR : le nombre de pertes non @@ -183,7 +248,12 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, errors: list[str] = [] for pr in fetch_merged(repo, since): number = pr.get("number") - paths, added, renamed = _ipynb_by_change(pr) + try: + files = fetch_files(repo, number) + except RuntimeError as exc: + errors.append(f"#{number}: fichiers illisibles ({exc})") + continue + paths, added, renamed = _ipynb_by_change(files) if not paths and not added and not renamed: continue if not paths: @@ -203,7 +273,16 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, continue base_ref = pr.get("baseRefOid") or "" head_ref = pr.get("headRefOid") or "" - base = base_of(repo_dir, base_ref, head_ref) if base_ref and head_ref else None + if not base_ref or not head_ref: + errors.append(f"#{number}: refs absentes (base={base_ref!r}, head={head_ref!r})") + continue + if not ensure(repo_dir, head_ref): + errors.append( + f"#{number}: commit de tete {head_ref[:8]} inatteignable -- " + "PR ecartee, pas mesuree contre l'arbre du jour" + ) + continue + base = base_of(repo_dir, base_ref, head_ref) if base is None: # Repli nomme : on mesure contre la ref de base declaree, sans # merge-base. Le chiffre reste exploitable, sa provenance est dite. @@ -217,7 +296,8 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, except RuntimeError as exc: errors.append(f"#{number}: body illisible ({exc})") continue - result = check([Path(p) for p in paths], base_ref=base, pr_body=body) + result = check([Path(p) for p in paths], base_ref=base, + head_ref=head_ref, pr_body=body) summary = result.as_payload().get("summary", {}) lost = int(summary.get("credited_lost_unexempted", 0) or 0) diff_errors = int(summary.get("credited_diff_errors", 0) or 0) diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py index 189f2e48f7..bcac7db0fe 100644 --- a/scripts/tests/test_credited_examples_sweep.py +++ b/scripts/tests/test_credited_examples_sweep.py @@ -3,12 +3,20 @@ Le balayage est concu pour etre injectable : `sweep()` prend ses appels reseau et git en parametres, donc tout se teste hors ligne. Les controles -epinglent les trois pieges qui rendraient le balayage trompeur : - +epinglent les pieges qui rendraient le balayage trompeur, dont les trois +mesures de la review #19215 : + + - la forme REELLE de `gh pr list --json` ne porte NI `files` NI + `changeType` (mesure gh 2.83.2) : la nature des fichiers vient de + `pr_files`, par GraphQL pagine -- jamais du lot ; + - un carnet supprime (#19040 a supprime GameTheory-18d) est ecarte sans + planter, et n'empeche pas la mesure des carnets modifies de la PR ; + - le cote « apres » est la tete de la PR (`headRefOid`), pas l'arbre du + moment ; un commit de tete inatteignable est NOMME, la PR ecartee ; - un lot qui ATTEINT le plafond de l'API de recherche n'est pas un compte ; - le label ne se pose que si TOUS les diffs ont reussi (#18761) ; - - un merge-base indisponible est NOMME, jamais un diff silencieux contre la - mauvaise base. + - un merge-base indisponible est NOMME, jamais un diff silencieux contre + la mauvaise base. """ from __future__ import annotations @@ -26,6 +34,9 @@ NOW = dt.datetime(2026, 10, 5, 12, 0, tzinfo=dt.timezone.utc) +IPY = "MyIA.AI.Notebooks/Search/Part1/x.ipynb" +MD = "MyIA.AI.Notebooks/Search/Part1/README.md" + class _FakeResult: def __init__(self, lost=0, diff_errors=0): @@ -38,44 +49,132 @@ def as_payload(self): return {"summary": self._s} -def _pr(number, files=None, base="a" * 40, head="b" * 40): +class _Proc: + def __init__(self, rc): + self.returncode = rc + + +def _pr(number, base="a" * 40, head="b" * 40): + """La forme EXACTE rendue par `gh pr list --json number,baseRefOid,headRefOid,mergedAt` : + pas de cle `files` -- les fichiers (et leur `changeType`) viennent de + `pr_files`, en GraphQL. C'est l'ecart que masquaient les tests d'avant, + qui injectaient des lots deja classes.""" return { "number": number, "baseRefOid": base, "headRefOid": head, - "files": files or [], "mergedAt": NOW.isoformat(), } def _nb(path, change="MODIFIED"): + """Un noeud GraphQL `pullRequest.files.nodes { path changeType }`.""" return {"path": path, "changeType": change} -IPY = "MyIA.AI.Notebooks/Search/Part1/x.ipynb" -MD = "MyIA.AI.Notebooks/Search/Part1/README.md" +def _files_of(mapping): + def fetch_files(repo, number): + return mapping[number] + return fetch_files -def _run_sweep(prs): +def _sweep(prs, *, files=None, body=None, base_of=None, ensure=None, check=None): """Drive `sweep` with the network and git fully injected (offline).""" + if files is None: + files = {} + fetch_files = _files_of(files) if isinstance(files, dict) else files return _mod.sweep( "o/r", Path("."), 24, NOW, fetch_merged=lambda repo, since: prs, - fetch_body=lambda repo, number: "", - base_of=lambda repo_dir, b, h: "deadbeef", - check=lambda paths, base_ref="", head_ref="", pr_body="": _FakeResult(), + fetch_files=fetch_files, + fetch_body=body or (lambda repo, number: ""), + base_of=base_of or (lambda repo_dir, b, h: "deadbeef"), + ensure=ensure or (lambda repo_dir, sha: True), + check=check or (lambda paths, base_ref="", head_ref="", pr_body="": + _FakeResult()), ) +class TestGhFormIsNotTheFilesSource: + """Review #19215, point 1 : `gh pr list --json files` ne rend pas + `changeType` -- tout le classement ADDED/DELETED/RENAMED etait mort en + production (repli « tout MODIFIED »), et les tests d'avant l'ignoraient + en injectant des dicts deja classes.""" + + def test_the_real_gh_pr_list_form_has_no_files_and_still_measures(self): + pr = _pr(1) + assert "files" not in pr, "la forme reelle de gh pr list ne porte pas files" + rows, errs = _sweep([pr], files={1: [_nb(IPY)]}) + assert len(rows) == 1 and rows[0]["notebooks"] == 1 and errs == [] + + def test_merged_prs_asks_exactly_the_four_fields(self): + seen = {} + + def run(argv): + seen["argv"] = argv + return [] + + _mod.merged_prs("o/r", NOW - dt.timedelta(hours=24), run=run) + argv = seen["argv"] + assert argv[argv.index("--json") + 1] == "number,baseRefOid,headRefOid,mergedAt" + assert "files" not in argv[argv.index("--json") + 1] + + def test_an_unreadable_files_query_names_the_pr_and_continues(self): + def fetch_files(repo, number): + if number == 1: + raise RuntimeError("gh failed (1): graphql timeout") + return [_nb(IPY)] + + rows, errs = _sweep([_pr(1), _pr(2)], files=fetch_files) + assert [r["number"] for r in rows] == [2] + assert len(errs) == 1 and "fichiers illisibles" in errs[0] + + def test_pages_are_followed_to_the_cursor(self): + """Une PR de carnet peut deplacer plus de 100 fichiers : la requete + GraphQL pagine au curseur, et le lot est la concatenation des pages.""" + pages = [ + {"data": {"repository": {"pullRequest": {"files": { + "nodes": [{"path": f"n{i}.ipynb", "changeType": "MODIFIED"} + for i in range(100)], + "pageInfo": {"hasNextPage": True, "endCursor": "CUR1"}}}}}}, + {"data": {"repository": {"pullRequest": {"files": { + "nodes": [{"path": "last.ipynb", "changeType": "DELETED"}], + "pageInfo": {"hasNextPage": False}}}}}}, + ] + calls = [] + + def run(argv): + calls.append(argv) + return pages[len(calls) - 1] + + nodes = _mod.pr_files("o/r", 7, run=run) + assert len(nodes) == 101 + assert nodes[-1]["changeType"] == "DELETED" + assert "cursor=CUR1" in calls[1], f"le curseur de la 1re page doit servir : {calls[1]}" + assert "cursor=CUR1" not in calls[0] + + class TestFiltering: def test_only_ipynb_paths_are_kept(self): - pr = _pr(1, [_nb(IPY), _nb(MD)]) - assert _mod.ipynb_paths(pr) == [IPY] + assert _mod.ipynb_paths([_nb(IPY), _nb(MD)]) == [IPY] def test_deleted_notebooks_are_excluded(self): """Un carnet supprime n'a plus rien a compter (--diff-filter=d).""" - pr = _pr(1, [_nb(IPY, "DELETED")]) - assert _mod.ipynb_paths(pr) == [] + assert _mod.ipynb_paths([_nb(IPY, "DELETED")]) == [] + + def test_a_deleted_notebook_no_longer_crashes_the_sweep(self): + """Regression #19215 (cause 1 de la review) : GameTheory-18d, + supprime par #19040, etait classe MODIFIED (changeType absent du lot) + puis ouvert -> FileNotFoundError, hors de tout garde. Avec la source + GraphQL il est ecarte ET les carnets modifies de la meme PR restent + mesures.""" + gone = "MyIA.AI.Notebooks/GameTheory/GameTheory-18d-Humour-Banc-Dur-Python.ipynb" + rows, errs = _sweep( + [_pr(19040)], + files={19040: [_nb(gone, "DELETED"), _nb(IPY, "MODIFIED")]}, + ) + assert errs == [] + assert rows[0]["notebooks"] == 1 and rows[0]["paths"] == [IPY] def test_added_notebooks_are_not_measured(self): """Un carnet neuf n'existe pas dans la base : `git show` sort en 128. @@ -83,19 +182,17 @@ def test_added_notebooks_are_not_measured(self): Ce n'est PAS une erreur de mesure (#18761) -- et la compter comme telle bloquait le label pour les carnets MODIFIES de la meme PR. """ - pr = _pr(1, [_nb(IPY, "ADDED")]) - assert _mod.ipynb_paths(pr) == [] - modified, added, renamed = _mod._ipynb_by_change(pr) + modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "ADDED")]) assert modified == [] and added == [IPY] and renamed == [] def test_added_and_modified_split_in_the_same_pr(self): other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" - pr = _pr(1, [_nb(other, "ADDED"), _nb(IPY, "MODIFIED")]) - modified, added, renamed = _mod._ipynb_by_change(pr) + modified, added, renamed = _mod._ipynb_by_change( + [_nb(other, "ADDED"), _nb(IPY, "MODIFIED")]) assert modified == [IPY] and added == [other] and renamed == [] def test_a_purely_additive_pr_is_named_not_dropped(self): - rows, errs = _run_sweep([_pr(1, [_nb(IPY, "ADDED")])]) + rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "ADDED")]}) assert len(rows) == 1 and rows[0]["notebooks"] == 0 assert rows[0]["added_notebooks"] == 1 assert rows[0]["would_label"] is False @@ -104,15 +201,13 @@ def test_a_purely_additive_pr_is_named_not_dropped(self): def test_a_renamed_notebook_is_declared_unmeasured_not_lost_free(self): """Un renommage PEUT perdre des exemples : on ne le dit pas « sans perte ». - La base est a un autre chemin (`previousFilename` non expose par - `gh pr view --json files`), donc la comparaison est impossible. Le - carnet ne doit ni etre mesure contre le mauvais chemin, ni etre tu. + La base est a un autre chemin (`previousFilename` hors du jeu GraphQL + demande ici), donc la comparaison est impossible. Le carnet ne doit ni + etre mesure contre le mauvais chemin, ni etre tu. """ - pr = _pr(1, [_nb(IPY, "RENAMED")]) - assert _mod.ipynb_paths(pr) == [] - modified, added, renamed = _mod._ipynb_by_change(pr) + modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "RENAMED")]) assert modified == [] and added == [] and renamed == [IPY] - rows, errs = _run_sweep([pr]) + rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "RENAMED")]}) assert len(rows) == 1 and rows[0]["renamed_notebooks"] == 1 assert rows[0]["notebooks"] == 0 assert rows[0]["would_label"] is False @@ -124,30 +219,98 @@ def test_a_renamed_notebook_does_not_block_the_other_notebooks(self): carnets modifies -- c'est le defaut que la separation corrige. """ other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" - pr = _pr(1, [_nb(other, "RENAMED"), _nb(IPY, "MODIFIED")]) + seen = [] def check(paths, base_ref="", head_ref="", pr_body=""): - assert [str(p).endswith("x.ipynb") for p in paths] == [True], paths + seen.append([str(p) for p in paths]) return _FakeResult(lost=2, diff_errors=0) - rows, errs = _mod.sweep( - "o/r", Path("."), 24, NOW, - fetch_merged=lambda repo, since: [pr], - fetch_body=lambda repo, number: "", - base_of=lambda *a: "deadbeef", + rows, errs = _sweep( + [_pr(1)], + files={1: [_nb(other, "RENAMED"), _nb(IPY, "MODIFIED")]}, check=check, ) assert rows[0]["would_label"] is True, rows assert rows[0]["renamed_notebooks"] == 1 + assert seen == [[str(Path(IPY))]] assert errs == [] def test_a_pr_without_notebook_is_skipped_by_the_sweep(self): - rows, errs = _run_sweep([_pr(1, [_nb(MD)])]) + rows, errs = _sweep([_pr(1)], files={1: [_nb(MD)]}) assert rows == [] and errs == [] def test_modified_notebook_is_kept(self): - pr = _pr(1, [_nb(IPY, "MODIFIED")]) - assert _mod.ipynb_paths(pr) == [IPY] + assert _mod.ipynb_paths([_nb(IPY, "MODIFIED")]) == [IPY] + + +class TestHeadIsThePrHead: + """Review #19215, point 2 : le cote « apres » est la tete de la PR, pas + l'arbre de travail du moment du balayage.""" + + def test_the_check_receives_the_head_of_the_pr(self): + captured = {} + + def check(paths, base_ref="", head_ref="", pr_body=""): + captured["base_ref"] = base_ref + captured["head_ref"] = head_ref + return _FakeResult(1, 0) + + _sweep([_pr(1, base="a" * 40, head="c" * 40)], + files={1: [_nb(IPY)]}, check=check) + # cote « apres » : la TETE de la PR, pas l'arbre du moment + assert captured["head_ref"] == "c" * 40 + # cote « avant » : le merge-base rendu par base_of, pas la base brute + assert captured["base_ref"] == "deadbeef" + + def test_a_local_commit_short_circuits_without_fetch(self): + calls = [] + + def run(argv): + calls.append(argv) + return _Proc(0) + + assert _mod.ensure_commit(Path("."), "b" * 40, run=run) is True + assert len(calls) == 1, "le probe suffit, pas de fetch" + assert "cat-file" in calls[0] + + def test_a_missing_commit_is_fetched_then_reconfirmed(self): + seq = [_Proc(1), _Proc(0), _Proc(0)] # probe manque, fetch, probe ok + + def run(argv): + return seq.pop(0) + + assert _mod.ensure_commit(Path("."), "b" * 40, run=run) is True + assert not seq + + def test_a_still_missing_commit_returns_false(self): + seq = [_Proc(1), _Proc(0), _Proc(1)] # meme apres fetch, introuvable + + def run(argv): + return seq.pop(0) + + assert _mod.ensure_commit(Path("."), "b" * 40, run=run) is False + assert not seq + + def test_an_unreachable_head_names_the_pr_and_skips_the_measurement(self): + """Une PR squash-mergee dont le headRefOid a disparu n'est JAMAIS + mesuree contre l'arbre du jour -- elle est ecartee, en erreur nommee.""" + seen = [] + + def check(*a, **k): + seen.append(a) + return _FakeResult(9, 0) + + rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY)]}, + ensure=lambda repo_dir, sha: False, check=check) + assert rows == [] and seen == [] + assert len(errs) == 1 and "inatteignable" in errs[0] + + def test_a_missing_merge_base_is_named_and_the_pr_is_still_measured(self): + rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY)]}, + base_of=lambda repo_dir, b, h: None, + check=lambda *a, **k: _FakeResult(1, 0)) + assert len(errs) == 1 and "merge-base" in errs[0] + assert rows[0]["would_label"] is True class TestSearchCapIsNotACount: @@ -182,24 +345,11 @@ def run(argv): class TestSweepVerdicts: - def _one(self, lost, diff_errors, *, bases="ok"): - pr = _pr(1, [_nb(IPY)]) - - def fetch_merged(repo, since): - return [pr] - - def fetch_body(repo, number): - return "" - - def base_of(repo_dir, b, h): - return "deadbeef" if bases == "ok" else None - - def check(paths, base_ref="", head_ref="", pr_body=""): - return _FakeResult(lost=lost, diff_errors=diff_errors) - - return _mod.sweep("o/r", Path("."), 24, NOW, - fetch_merged=fetch_merged, fetch_body=fetch_body, - base_of=base_of, check=check) + def _one(self, lost, diff_errors): + return _sweep( + [_pr(1)], files={1: [_nb(IPY)]}, + check=lambda *a, **k: _FakeResult(lost, diff_errors), + ) def test_a_loss_with_clean_diffs_is_labelled(self): rows, errs = self._one(2, 0) @@ -216,23 +366,10 @@ def test_zero_loss_does_not_label(self): rows, _ = self._one(0, 0) assert rows[0]["would_label"] is False - def test_a_missing_merge_base_is_named_and_the_pr_is_still_measured(self): - rows, errs = self._one(1, 0, bases="missing") - assert len(errs) == 1 and "merge-base" in errs[0] - assert rows[0]["would_label"] is True - def test_an_unreadable_body_is_named_and_the_pr_is_skipped(self): - pr = _pr(1, [_nb(IPY)]) - def boom(repo, number): raise RuntimeError("gh failed (1): not found") - rows, errs = _mod.sweep( - "o/r", Path("."), 24, NOW, - fetch_merged=lambda repo, since: [pr], - fetch_body=boom, - base_of=lambda *a: "deadbeef", - check=lambda *a, **k: _FakeResult(1, 0), - ) + rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY)]}, body=boom) assert rows == [] assert len(errs) == 1 and "body illisible" in errs[0] From 90b0435f1e942fe7dd96b99c6c201daf737b76c7 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 09:40:12 +0200 Subject: [PATCH 3/6] fix(ci,#19215): le comptage lit la revision de la PR, et un carnet illisible n'emporte plus le balayage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 5411248369, deux voies demandees, les deux faites. 1. `check_notebooks` comptait les exercices sur l'ARBRE DU JOUR (`count_exercises_in_notebook(path)`) alors que le diff credite lit deja le blob de `head_ref`. Consequence mesuree : #18788 MODIFIE ICT-45-InoculationBifurcation-9B, #19153 le RENOMME ensuite -> le chemin est MODIFIED mais absent de l'arbre, et le comptage levait un FileNotFoundError qui emportait TOUT le balayage (`--hours 72`, rc=1 : les autres PR de la fenetre n'etaient pas mesurees). Le comptage porte desormais sur le blob de tete, qui existe par construction pour un chemin MODIFIED. La classification reste sur le chemin d'origine : `classify_notebook` lit les regles de REPERTOIRE, que le fichier temporaire du blob ne porte pas. Effet de bord voulu, sur le meme chemin : meme quand le carnet existe dans l'arbre, c'est la revision de la PR qui est comptee -- l'ancien code pouvait mesurer un arbre different de celui qu'il comparait. 2. Le repli par PR : un echec de `check` est NOMME (« carnet illisible depuis l'arbre du jour (FileNotFoundError: ...) -- renomme ou supprime apres merge, PR ecartee ») et la suite de la fenetre est mesuree. La portee du `except` est etroite (`OSError`, `ValueError`) : un autre type d'echec remonte, pour qu'un bug du compteur ne se derobe pas en « carnet renomme ». Tests : 27 -> 32. Les nouveaux portent sur un depot git REEL (deux commits, renommage effectif), pas sur un dict injecte : carnet MODIFIED absent de l'arbre compte depuis le blob (3 exercices) ; l'arbre porte une autre version (1 exercice) et la tete gagne ; sans `head_ref` l'arbre sert encore (retro-compatibilite) ; un `check` qui leve est nomme et les autres PR sont mesurees ; un `RuntimeError` remonte au lieu d'etre absorbe. Falsification : 3 mutants, 3 rouges -- comptage remis sur l'arbre (2 tests), repli par PR retire (1), `except` elargi a `Exception` (1). Source restauree, 32 passed. Suivi RENAMED : issue #19251 ouverte avant merge (mesurer les renommages, `previousFilename` absent du jeu GraphQL). Co-Authored-By: Claude Sonnet 5.5 --- scripts/notebook_tools/check_pr_exercises.py | 23 ++- .../notebook_tools/credited_examples_sweep.py | 16 +- scripts/tests/test_credited_examples_sweep.py | 140 ++++++++++++++++++ 3 files changed, 176 insertions(+), 3 deletions(-) diff --git a/scripts/notebook_tools/check_pr_exercises.py b/scripts/notebook_tools/check_pr_exercises.py index 47486909fd..37a4e7d1b7 100644 --- a/scripts/notebook_tools/check_pr_exercises.py +++ b/scripts/notebook_tools/check_pr_exercises.py @@ -53,6 +53,7 @@ import argparse import json +import subprocess import sys from dataclasses import asdict, dataclass, field from pathlib import Path @@ -238,7 +239,27 @@ def check_notebooks( ) continue - cnt = count_exercises_in_notebook(path) + # #19215 (review 5411248369) : le comptage doit porter sur la MEME + # revision que le diff credite, pas sur l'arbre du jour. Un carnet + # MODIFIE par une PR puis RENOMME (ou supprime) par une PR suivante + # est absent de l'arbre : `count_exercises_in_notebook` levait un + # FileNotFoundError qui emportait tout le balayage -- les autres PR + # de la fenetre n'etaient pas mesurees. Le blob de tete, lui, existe + # par construction pour un chemin MODIFIED. + # + # La classification, elle, reste sur `path` : `classify_notebook` lit + # les regles de REPERTOIRE (`IIT/`, `groupe-`, `_`) et le fichier + # temporaire du blob ne les porte pas -- classify sur le blob + # reclasserait le carnet en `archive`/`tooling` sur son seul prefixe. + count_path = path + if head_ref and _HAS_CREDITED: + try: + count_path = _read_git_blob(head_ref, str(path).replace("\\", "/")) + except (subprocess.CalledProcessError, OSError): + # Blob absent de la tete : on garde l'arbre du jour. Si lui + # aussi manque, l'echec est nomme par l'appelant (sweep). + count_path = path + cnt = count_exercises_in_notebook(count_path) if cnt.parse_error is not None: result.parse_errors.append( NotebookVerdict( diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index 2eee6a01f2..ee723722f1 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -296,8 +296,20 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, except RuntimeError as exc: errors.append(f"#{number}: body illisible ({exc})") continue - result = check([Path(p) for p in paths], base_ref=base, - head_ref=head_ref, pr_body=body) + try: + result = check([Path(p) for p in paths], base_ref=base, + head_ref=head_ref, pr_body=body) + except (OSError, ValueError) as exc: + # #19215 (review 5411248369, voie 2) : un carnet illisible depuis + # l'arbre du jour ne doit pas emporter les AUTRES PR de la + # fenetre. Nommee, la PR est ecartee et la suite est mesuree -- + # l'inverse d'un masque : le repli est compte et imprime. + errors.append( + f"#{number}: carnet illisible depuis l'arbre du jour " + f"({exc.__class__.__name__}: {exc}) -- renomme ou supprime " + "apres merge, PR ecartee" + ) + continue summary = result.as_payload().get("summary", {}) lost = int(summary.get("credited_lost_unexempted", 0) or 0) diff_errors = int(summary.get("credited_diff_errors", 0) or 0) diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py index bcac7db0fe..b5e3c668e5 100644 --- a/scripts/tests/test_credited_examples_sweep.py +++ b/scripts/tests/test_credited_examples_sweep.py @@ -22,9 +22,17 @@ import datetime as dt import importlib.util +import json +import subprocess import sys + +import pytest from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "notebook_tools")) + +import check_pr_exercises as _cpe # noqa: E402 + _SCRIPT = Path(__file__).resolve().parent.parent / "notebook_tools" / "credited_examples_sweep.py" _spec = importlib.util.spec_from_file_location("credited_examples_sweep", _SCRIPT) assert _spec and _spec.loader, f"could not load {_SCRIPT}" @@ -373,3 +381,135 @@ def boom(repo, number): rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY)]}, body=boom) assert rows == [] assert len(errs) == 1 and "body illisible" in errs[0] + + +class TestARenamedNotebookNoLongerTakesDownTheSweep: + """Review #19215 (5411248369), la mesure qui a fait tomber `--hours 72`. + + #18788 a MODIFIE ICT-45-InoculationBifurcation-9B ; #19153 l'a ensuite + RENOMME en ICT-42b. Pour #18788 le chemin est donc MODIFIED, mais il + n'existe plus dans l'arbre du jour : `check_notebooks` comptait les + exercices sur cet arbre et levait un FileNotFoundError, ce qui emportait + TOUT le balayage -- les autres PR de la fenetre n'etaient pas mesurees. + """ + + def test_a_failing_check_names_the_pr_and_the_others_are_still_measured(self): + def check(paths, base_ref="", head_ref="", pr_body=""): + if paths and "ICT-45" in str(paths[0]): + raise FileNotFoundError( + "MyIA.AI.Notebooks/IIT/ICT-Series/ICT-45-...ipynb") + return _FakeResult() + + rows, errs = _sweep( + [_pr(18788), _pr(18789)], + files={18788: [_nb("MyIA.AI.Notebooks/IIT/ICT-Series/ICT-45-x.ipynb")], + 18789: [_nb(IPY)]}, + check=check, + ) + assert [r["number"] for r in rows] == [18789], \ + "un carnet illisible ne doit pas emporter les autres PR" + assert len(errs) == 1 + assert errs[0].startswith("#18788:") + assert "carnet illisible" in errs[0] + assert "renomme ou supprime apres merge" in errs[0] + assert "FileNotFoundError" in errs[0], \ + "le nom de l'exception doit rester lisible dans le repli" + + def test_only_a_missing_notebook_is_named_other_errors_still_propagate(self): + """La portee du `except` est etroite : un bug du compteur ne doit pas + se derober en « carnet renomme ». Un repli trop large remplacerait une + panne par un chiffre manquant, ce que la fenetre ne distinguerait pas + d'une PR sans perte.""" + def check(paths, base_ref="", head_ref="", pr_body=""): + raise RuntimeError("bug du compteur") + + with pytest.raises(RuntimeError): + _sweep([_pr(7)], files={7: [_nb(IPY)]}, check=check) + + +def _notebook_json(n_exercises): + """Un carnet minimal : N paires (en-tete `### Exercice i` + stub TODO).""" + cells = [] + for i in range(1, n_exercises + 1): + cells.append({"cell_type": "markdown", "metadata": {}, + "source": [f"### Exercice {i}\n", "A completer.\n"]}) + cells.append({"cell_type": "code", "metadata": {}, "execution_count": None, + "outputs": [], "source": ["# TODO etudiant\n", + "result = None\n"]}) + return {"cells": cells, "metadata": {}, "nbformat": 4, "nbformat_minor": 5} + + +def _git(repo, *args): + subprocess.run( + ["git", "-C", str(repo), "-c", "user.name=t", "-c", "user.email=t@t", + "-c", "core.autocrlf=false", *args], + check=True, capture_output=True, text=True, + ) + + +class TestTheCountReadsTheRevisionNotTheTree: + """Review #19215 : `check_notebooks` comptait les exercices sur l'ARBRE DU + JOUR (`count_exercises_in_notebook(path)`), alors que le diff credite lit + deja le blob de `head_ref`. Deux consequences, une panne et un chiffre + faux -- les deux sont epinglees ici sur un depot git reel.""" + + REL = "MyIA.AI.Notebooks/Search/Part1/x.ipynb" + + def _repo_with_head(self, tmp_path, *, tree): + """Depot a deux commits ; la tete porte 3 exercices. + + ``tree`` : ce que l'arbre de travail contient a la fin -- ``None`` + (carnet renomme/supprime depuis), ou un carnet d'un autre nombre + d'exercices (l'arbre a bouge depuis le merge).""" + repo = tmp_path / "repo" + (repo / Path(self.REL).parent).mkdir(parents=True) + target = repo / self.REL + _git(repo, "init", "-q") + target.write_text(json.dumps(_notebook_json(1)), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "base") + base = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True).stdout.strip() + target.write_text(json.dumps(_notebook_json(3)), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "head") + head = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True).stdout.strip() + if tree is None: + target.unlink() + else: + target.write_text(json.dumps(_notebook_json(tree)), encoding="utf-8") + return repo, base, head + + def _count(self, result): + """Le compte du carnet mesure, quel que soit son seau.""" + for bucket in (result.ok, result.sub_threshold, result.parse_errors, + result.out_of_corpus): + for verdict in bucket: + if verdict.path.endswith("x.ipynb"): + return verdict.count + raise AssertionError("le carnet n'apparait dans aucun seau du resultat") + + def test_a_modified_notebook_absent_from_the_tree_is_counted_from_the_blob( + self, tmp_path, monkeypatch): + """Le cas #18788 puis #19153 : ICT-45 modifie, puis renomme.""" + repo, base, head = self._repo_with_head(tmp_path, tree=None) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks([Path(self.REL)], base_ref=base, head_ref=head) + assert self._count(result) == 3, \ + "le compte doit venir du blob de tete, pas de l'arbre (absent)" + + def test_the_tree_version_never_wins_over_the_pr_head(self, tmp_path, monkeypatch): + """Meme quand le chemin existe, c'est la revision de la PR qui compte.""" + repo, base, head = self._repo_with_head(tmp_path, tree=1) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks([Path(self.REL)], base_ref=base, head_ref=head) + assert self._count(result) == 3, \ + "1 exercice dans l'arbre, 3 dans la tete : la tete est la mesure" + + def test_without_a_head_ref_the_tree_still_serves(self, tmp_path, monkeypatch): + """Retro-compatibilite : l'appel sans tete (CLI locale) lit l'arbre.""" + repo, _base, _head = self._repo_with_head(tmp_path, tree=2) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks([Path(self.REL)]) + assert self._count(result) == 2 From 1fd57432dcad87ebcec3f6033fe84602d71bddee Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 10:11:30 +0200 Subject: [PATCH 4/6] feat(ci,#19251): mesurer les carnets RENAMED dans le balayage credited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le balayage post-mortem des exemples credites (#19101) declarait tout carnet RENAMED « NON MESURE » faute de connaitre son chemin de base : `previousFilename` n'etait pas demande au jeu GraphQL. Un `git mv` suivi d'une edition pouvait donc perdre un exemple credite sans que rien ne le voie. Le champ est desormais demande (`nodes { path changeType previousFilename }`). Quand il est present, le renommage est MESURE comme un MODIFIED : le diff credite lit `base:previousFilename` contre `head:path`. La correspondance passe par `base_path_of`, consultee en POSIX -- GraphQL rend des `/` et `str(Path)` des `\` sous Windows : sans la normalisation, la correspondance raterait en silence et le renommage redeviendrait non mesure (mutant M3). Quand `previousFilename` manque (renommage sous un seuil de similarite, ou reponse d'API degradee), le carnet reste nomme NON MESURE -- et surtout, sans correspondance la base est absente a ce chemin : le diff est en ERREUR, que #18761 refuse de convertir en label. Jamais un zero silencieux. Tests 32 -> 37 : le renommage mesure (un exemple credite perdu au `git mv` est vu), le renommage sans perte (pas de faux positif), et l'absence de zero silencieux sont epingles sur un depot git REEL ; le contrat de la carte en plus. Falsification : 4 mutants, 4 rouges -- mapping ignore (M1), `previousFilename` ignore (M2), cle non normalisee (doublure du bug Windows, M3), diff en erreur converti en zero silencieux (M4) ; source restauree, 37 passed. Au passage, les appels `subprocess` des tests touchés portent `encoding="utf-8", errors="replace"` (garde #13140/#12811 : un hôte cp1252 leve sur un payload UTF-8). Closes #19251 Co-Authored-By: Claude Sonnet 5.5 --- scripts/notebook_tools/check_pr_exercises.py | 18 +- .../notebook_tools/credited_examples_sweep.py | 80 ++++-- scripts/tests/test_credited_examples_sweep.py | 231 ++++++++++++++++-- 3 files changed, 280 insertions(+), 49 deletions(-) diff --git a/scripts/notebook_tools/check_pr_exercises.py b/scripts/notebook_tools/check_pr_exercises.py index 37a4e7d1b7..7fcb8b6271 100644 --- a/scripts/notebook_tools/check_pr_exercises.py +++ b/scripts/notebook_tools/check_pr_exercises.py @@ -205,6 +205,7 @@ def check_notebooks( base_ref: str = "", head_ref: str = "", pr_body: str = "", + base_path_of: dict[str, str] | None = None, ) -> CheckResult: """Classify + count each path, bucketing by advisory status. @@ -219,6 +220,12 @@ def check_notebooks( exemption markers from ``pr_body``. The default (``base_ref=""``) skips this check (preserves backward compatibility for callers that don't pass a base ref). + + #19251 -- ``base_path_of`` maps a ``path`` to the location it had at the + base, for a notebook the PR **renamed**: the credited diff then reads + ``base:previousFilename`` against ``head:path`` instead of the same path on + both sides (which would find nothing at the base and read as a full loss). + Absent from the map, ``path`` is used for both sides -- the pre-rename case. """ result = CheckResult() # Pré-calcul des exemptions une seule fois (cf. #18740). @@ -283,7 +290,16 @@ def check_notebooks( head_examples = count_credited_examples(_read_nb(head_blob)) else: head_examples = count_credited_examples(_read_nb(path)) - base_path = _read_git_blob(base_ref, str(path).replace("\\", "/")) + # #19251 -- un carnet RENOMME par la PR n'existe pas au meme + # chemin dans la base : sans la correspondance, `base:path` + # rendrait un blob absent et le diff lirait une perte totale. + # La carte est clee en POSIX (le chemin GraphQL) : on normalise + # la cle du `Path` avant de la consulter, sinon sous Windows + # (`str(path)` rend des `\`) la correspondance raterait en + # silence et le renommage redeviendrait non mesure. + posix_path = str(path).replace("\\", "/") + base_side = (base_path_of or {}).get(posix_path, posix_path) + base_path = _read_git_blob(base_ref, base_side) base_examples = count_credited_examples(_read_nb(base_path)) diff = diff_examples(base_examples, head_examples) nb_name = path.name diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index ee723722f1..24fde00c02 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -20,10 +20,12 @@ - **`changeType` n'existe pas dans `gh pr list --json files`** (mesure gh 2.83.2 : cette forme ne rend que `{additions, deletions, path}`). Le classifieur ADDED/DELETED/RENAMED se nourrit donc de **GraphQL** - (`pullRequest.files.nodes { path changeType }`), pagine au curseur. Un - carnet supprime (#19040 a supprime GameTheory-18d) fait sinon planter le - balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la premiere - version classait tout en modifie. + (`pullRequest.files.nodes { path changeType previousFilename }`), pagine au + curseur. Un carnet supprime (#19040 a supprime GameTheory-18d) fait sinon + planter le balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la + premiere version classait tout en modifie. `previousFilename` (#19251) donne + le chemin de base d'un RENAMED : quand il est present, le renommage est + **mesure** comme un MODIFIED ; quand il manque, il reste nomme NON MESURE. - **le cote « apres » est la tete de la PR, pas l'arbre de travail** : `check_notebooks` recoit `head_ref=headRefOid`, et le commit est amene localement s'il manque (PR squash-mergee : l'objet n'est pas dans `main`). @@ -78,7 +80,7 @@ repository(owner: $owner, name: $name) { pullRequest(number: $num) { files(first: 100, after: $cursor) { - nodes { path changeType } + nodes { path changeType previousFilename } pageInfo { hasNextPage endCursor } } } @@ -155,10 +157,12 @@ def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: cursor = page.get("endCursor") -def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str]]: - """`(modifies, ajoutes, renommes)` parmi les `.ipynb` de la PR. +def _ipynb_by_change( + files: list[dict], +) -> tuple[list[str], list[str], list[tuple[str, str]], list[str]]: + """`(modifies, ajoutes, renommes_mesurables, renommes_non_mesurables)`. - Deux exclusions et une separation, toutes structurelles, toutes lues de + Trois exclusions et une separation, toutes structurelles, toutes lues de `changeType` (GraphQL) : - `DELETED` : supprime, plus rien a compter (meme regle que le @@ -167,17 +171,27 @@ def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str] l'ouverture du carnet levait `FileNotFoundError` sur GameTheory-18d (supprime par #19040) ; - `ADDED` : neuf, donc **rien a perdre** par construction ; - - `RENAMED` : la version de base existe **sous un autre chemin**, que - cette source n'expose pas (`previousFilename` absent du jeu GraphQL - demande ici). On ne peut donc pas la comparer -- et contrairement a - `ADDED`, un renommage **peut** perdre des exemples. NON MESURE. + - `RENAMED` : la version de base existe **sous un autre chemin**. Depuis + #19251 ce chemin est demande au jeu GraphQL (`previousFilename`) : + quand il est present, le renommage est **mesurable** et rendu comme la + paire `(chemin_de_tete, chemin_de_base)`, pour que le diff credite soit + calcule entre `base:previous` et `head:path` -- un `git mv` suivi d'une + edition peut perdre des exemples comme un `MODIFIED`. + + `previousFilename` peut manquer (renommage detecte par git sous un seuil de + similarite, ou reponse d'API degradee) : ces renommages-la restent + **non mesurables** et sont rendus a part, pour etre **nommes** NON MESURES + plutot que comptes comme zero perte. Pourquoi separer plutot que compter en erreur : `credited_diff_errors > 0` **bloque** la pose du label (#18761). Une erreur structurelle sur un carnet empechait donc la mesure reelle des carnets modifies de la meme PR -- un faux positif d'erreur produisait un faux zero de pertes. """ - modified, added, renamed = [], [], [] + modified: list[str] = [] + added: list[str] = [] + renamed_measured: list[tuple[str, str]] = [] + renamed_unmeasured: list[str] = [] for f in (files or []): path = f.get("path") or "" if not path.endswith(".ipynb"): @@ -188,10 +202,14 @@ def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str] if change == "ADDED": added.append(path) elif change == "RENAMED": - renamed.append(path) + previous = f.get("previousFilename") or "" + if previous: + renamed_measured.append((path, previous)) + else: + renamed_unmeasured.append(path) else: modified.append(path) - return modified, added, renamed + return modified, added, renamed_measured, renamed_unmeasured def ipynb_paths(files: list[dict]) -> list[str]: @@ -253,18 +271,27 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, except RuntimeError as exc: errors.append(f"#{number}: fichiers illisibles ({exc})") continue - paths, added, renamed = _ipynb_by_change(files) + modified, added, renamed_measured, renamed_unmeasured = \ + _ipynb_by_change(files) + # #19251 -- un renommage dont `previousFilename` est connu est mesure + # comme un MODIFIED : sa tete se lit au nouveau chemin, sa base a + # l'ancien. `base_path_of` porte cette correspondance ; un renommage + # sans `previousFilename` reste NON MESURE et seulement nomme. + base_path_of = {head: prev for head, prev in renamed_measured} + paths = modified + [head for head, _ in renamed_measured] + renamed = len(renamed_measured) + len(renamed_unmeasured) if not paths and not added and not renamed: continue if not paths: # PR sans carnet mesurable : rien a perdre (ajouts) ou rien de - # comparable (renommages). On la NOMME plutot que de la faire - # disparaitre du rapport. + # comparable (renommages sans chemin de base). On la NOMME plutot + # que de la faire disparaitre du rapport. rows.append({ "number": number, "notebooks": 0, "added_notebooks": len(added), - "renamed_notebooks": len(renamed), + "renamed_notebooks": renamed, + "renamed_measured": len(renamed_measured), "credited_lost_unexempted": 0, "credited_diff_errors": 0, "would_label": False, @@ -298,7 +325,8 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, continue try: result = check([Path(p) for p in paths], base_ref=base, - head_ref=head_ref, pr_body=body) + head_ref=head_ref, pr_body=body, + base_path_of=base_path_of) except (OSError, ValueError) as exc: # #19215 (review 5411248369, voie 2) : un carnet illisible depuis # l'arbre du jour ne doit pas emporter les AUTRES PR de la @@ -317,7 +345,8 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, "number": number, "notebooks": len(paths), "added_notebooks": len(added), - "renamed_notebooks": len(renamed), + "renamed_notebooks": renamed, + "renamed_measured": len(renamed_measured), "credited_lost_unexempted": lost, "credited_diff_errors": diff_errors, # #18761 : le label ne se pose QUE si tous les diffs ont reussi. @@ -370,11 +399,16 @@ def main(argv: list[str] | None = None) -> int: mark = "LABEL" if row["would_label"] else " - " added = row.get("added_notebooks", 0) renamed = row.get("renamed_notebooks", 0) + renamed_measured = row.get("renamed_measured", 0) + renamed_unmeasured = renamed - renamed_measured suffix = "" if added: suffix += f" (+{added} neuf(s), rien a perdre)" - if renamed: - suffix += f" ({renamed} renomme(s), NON MESURE(S) : base a un autre chemin)" + if renamed_measured: + suffix += f" ({renamed_measured} renomme(s) mesure(s) via previousFilename)" + if renamed_unmeasured: + suffix += (f" ({renamed_unmeasured} renomme(s) NON MESURE(S) : " + "previousFilename absent)") print(f" [{mark}] #{row['number']}: {row['notebooks']} carnet(s) modifie(s), " f"pertes non exemptees={row['credited_lost_unexempted']}, " f"erreurs de diff={row['credited_diff_errors']}{suffix}") diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py index b5e3c668e5..3904543f57 100644 --- a/scripts/tests/test_credited_examples_sweep.py +++ b/scripts/tests/test_credited_examples_sweep.py @@ -75,9 +75,17 @@ def _pr(number, base="a" * 40, head="b" * 40): } -def _nb(path, change="MODIFIED"): - """Un noeud GraphQL `pullRequest.files.nodes { path changeType }`.""" - return {"path": path, "changeType": change} +def _nb(path, change="MODIFIED", previous=None): + """Un noeud GraphQL `pullRequest.files.nodes { path changeType previousFilename }`. + + `previous` n'est pose que quand il est fourni : un noeud sans la cle simule + la reponse degradee (renommage sous seuil de similarite) que #19251 doit + continuer de nommer NON MESURE, pas de mesurer contre le mauvais chemin. + """ + node = {"path": path, "changeType": change} + if previous is not None: + node["previousFilename"] = previous + return node def _files_of(mapping): @@ -98,8 +106,8 @@ def _sweep(prs, *, files=None, body=None, base_of=None, ensure=None, check=None) fetch_body=body or (lambda repo, number: ""), base_of=base_of or (lambda repo_dir, b, h: "deadbeef"), ensure=ensure or (lambda repo_dir, sha: True), - check=check or (lambda paths, base_ref="", head_ref="", pr_body="": - _FakeResult()), + check=check or (lambda paths, base_ref="", head_ref="", pr_body="", + base_path_of=None: _FakeResult()), ) @@ -190,14 +198,17 @@ def test_added_notebooks_are_not_measured(self): Ce n'est PAS une erreur de mesure (#18761) -- et la compter comme telle bloquait le label pour les carnets MODIFIES de la meme PR. """ - modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "ADDED")]) - assert modified == [] and added == [IPY] and renamed == [] + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(IPY, "ADDED")]) + assert modified == [] and added == [IPY] + assert renamed_m == [] and renamed_u == [] def test_added_and_modified_split_in_the_same_pr(self): other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" - modified, added, renamed = _mod._ipynb_by_change( + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( [_nb(other, "ADDED"), _nb(IPY, "MODIFIED")]) - assert modified == [IPY] and added == [other] and renamed == [] + assert modified == [IPY] and added == [other] + assert renamed_m == [] and renamed_u == [] def test_a_purely_additive_pr_is_named_not_dropped(self): rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "ADDED")]}) @@ -206,30 +217,82 @@ def test_a_purely_additive_pr_is_named_not_dropped(self): assert rows[0]["would_label"] is False assert errs == [] - def test_a_renamed_notebook_is_declared_unmeasured_not_lost_free(self): - """Un renommage PEUT perdre des exemples : on ne le dit pas « sans perte ». + def test_a_renamed_notebook_without_previous_is_unmeasured_not_lost_free(self): + """Un renommage sans `previousFilename` PEUT perdre des exemples : on ne + le dit pas « sans perte ». - La base est a un autre chemin (`previousFilename` hors du jeu GraphQL - demande ici), donc la comparaison est impossible. Le carnet ne doit ni - etre mesure contre le mauvais chemin, ni etre tu. + `previousFilename` manque (renommage sous seuil de similarite, ou + reponse d'API degradee) : la base est a un autre chemin inconnu, donc la + comparaison est impossible. Le carnet ne doit ni etre mesure contre le + mauvais chemin, ni etre tu (#19251 preserve ce repli). """ - modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "RENAMED")]) - assert modified == [] and added == [] and renamed == [IPY] + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(IPY, "RENAMED")]) + assert modified == [] and added == [] + assert renamed_m == [] and renamed_u == [IPY] rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "RENAMED")]}) assert len(rows) == 1 and rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 0 assert rows[0]["notebooks"] == 0 assert rows[0]["would_label"] is False + def test_a_renamed_notebook_with_previous_is_measured_as_modified(self): + """#19251 : quand `previousFilename` est connu, un renommage est mesure. + + Sa tete se lit au nouveau chemin, sa base a l'ancien -- un `git mv` + suivi d'une edition peut perdre des exemples comme un MODIFIED. + """ + old = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + new = "MyIA.AI.Notebooks/Search/Part2/y.ipynb" + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(new, "RENAMED", previous=old)]) + assert modified == [] and added == [] + assert renamed_m == [(new, old)] and renamed_u == [] + rows, errs = _sweep( + [_pr(1)], files={1: [_nb(new, "RENAMED", previous=old)]}, + check=lambda paths, base_ref="", head_ref="", pr_body="", + base_path_of=None: _FakeResult(lost=3, diff_errors=0), + ) + assert errs == [] + assert rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 1 + assert rows[0]["notebooks"] == 1 + assert rows[0]["credited_lost_unexempted"] == 3 + assert rows[0]["would_label"] is True + + def test_the_base_path_of_a_rename_reaches_the_check(self): + """Le contrat de la carte : `base_path_of` porte la correspondance + nouveau chemin -> ancien chemin, pour le cote base du diff credite.""" + old = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + new = "MyIA.AI.Notebooks/Search/Part2/y.ipynb" + seen = {} + + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): + seen["paths"] = [str(p) for p in paths] + seen["map"] = dict(base_path_of or {}) + return _FakeResult() + + _sweep( + [_pr(1)], files={1: [_nb(new, "RENAMED", previous=old)]}, + check=check, + ) + assert seen["paths"] == [str(Path(new))] + # la carte est clee en POSIX (le chemin tel que rendu par GraphQL), pas + # dans la forme native du systeme : c'est ce que `check_notebooks` + # normalise avant de la consulter (cf. le test de lookup ci-dessous). + assert seen["map"] == {new: old} + def test_a_renamed_notebook_does_not_block_the_other_notebooks(self): """Le faux positif d'erreur produisait un faux zero de pertes (#18761). - Un carnet renomme dans la meme PR ne doit plus empecher la mesure des - carnets modifies -- c'est le defaut que la separation corrige. + Un carnet renomme SANS chemin de base dans la meme PR ne doit pas + empecher la mesure des carnets modifies -- c'est le defaut que la + separation corrige. """ other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" seen = [] - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): seen.append([str(p) for p in paths]) return _FakeResult(lost=2, diff_errors=0) @@ -240,6 +303,7 @@ def check(paths, base_ref="", head_ref="", pr_body=""): ) assert rows[0]["would_label"] is True, rows assert rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 0 assert seen == [[str(Path(IPY))]] assert errs == [] @@ -258,7 +322,7 @@ class TestHeadIsThePrHead: def test_the_check_receives_the_head_of_the_pr(self): captured = {} - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): captured["base_ref"] = base_ref captured["head_ref"] = head_ref return _FakeResult(1, 0) @@ -394,7 +458,7 @@ class TestARenamedNotebookNoLongerTakesDownTheSweep: """ def test_a_failing_check_names_the_pr_and_the_others_are_still_measured(self): - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): if paths and "ICT-45" in str(paths[0]): raise FileNotFoundError( "MyIA.AI.Notebooks/IIT/ICT-Series/ICT-45-...ipynb") @@ -420,7 +484,7 @@ def test_only_a_missing_notebook_is_named_other_errors_still_propagate(self): se derober en « carnet renomme ». Un repli trop large remplacerait une panne par un chiffre manquant, ce que la fenetre ne distinguerait pas d'une PR sans perte.""" - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): raise RuntimeError("bug du compteur") with pytest.raises(RuntimeError): @@ -443,7 +507,8 @@ def _git(repo, *args): subprocess.run( ["git", "-C", str(repo), "-c", "user.name=t", "-c", "user.email=t@t", "-c", "core.autocrlf=false", *args], - check=True, capture_output=True, text=True, + check=True, capture_output=True, text=True, encoding="utf-8", + errors="replace", ) @@ -469,12 +534,14 @@ def _repo_with_head(self, tmp_path, *, tree): _git(repo, "add", "-A") _git(repo, "commit", "-q", "-m", "base") base = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], - capture_output=True, text=True, check=True).stdout.strip() + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() target.write_text(json.dumps(_notebook_json(3)), encoding="utf-8") _git(repo, "add", "-A") _git(repo, "commit", "-q", "-m", "head") head = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], - capture_output=True, text=True, check=True).stdout.strip() + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() if tree is None: target.unlink() else: @@ -513,3 +580,117 @@ def test_without_a_head_ref_the_tree_still_serves(self, tmp_path, monkeypatch): monkeypatch.chdir(repo) result = _cpe.check_notebooks([Path(self.REL)]) assert self._count(result) == 2 + + +def _credited_notebook_json(n, *, tag): + """Un carnet a ``n`` exemples credits, chacun d'identite distincte. + + L'identite d'un exemple credite est ``(credit, cell_id)`` : un exemple + conserve au renommage garde le MEME ``cell_id`` des deux cotes, sinon il + se lirait comme perdu puis reajoute. ``tag`` sert a distinguer des carnets + differents d'un meme test, pas la base de la tete. + """ + cells = [] + for i in range(1, n + 1): + cells.append({ + "cell_type": "markdown", "id": f"{tag}-ex{i}", "metadata": {}, + "source": [f"### Exemple {i}\n", f"[crédité #{1000 + i}]\n"], + }) + cells.append({ + "cell_type": "code", "id": f"{tag}-c{i}", "metadata": {}, + "execution_count": 1, "outputs": [], "source": [f"print({i})\n"], + }) + return {"cells": cells, "metadata": {}, "nbformat": 4, "nbformat_minor": 5} + + +class TestARenamedNotebookIsMeasuredAtItsOldPath: + """#19251 : un RENAMED dont `previousFilename` est connu est MESURE. + + Le renommage est le cas aveugle qui a motive #19251 : la base est a un + autre chemin, donc `base:path` ne trouve rien. Sans la correspondance, la + mesure tombe -- et une mesure tombee se lirait soit comme zero perte (si + on avalait l'erreur), soit comme un diff en erreur (honnete mais inutile). + Ici on epingle la VRAIE mesure : le carnet renomme qui perd un exemple + credite est vu, sur un depot git reel. + """ + + OLD = "MyIA.AI.Notebooks/Search/Part1/moved.ipynb" + NEW = "MyIA.AI.Notebooks/Search/Part2/moved.ipynb" + + def _repo_with_rename(self, tmp_path, *, keep=2): + """Commit base : 3 exemples credites a OLD. Commit tete : `git mv` vers + NEW, ``keep`` exemples conserves (identites stables) -- ``keep=2`` perd + le 3e, ``keep=3`` ne perd rien.""" + repo = tmp_path / "repo" + (repo / Path(self.OLD).parent).mkdir(parents=True) + _git(repo, "init", "-q") + (repo / self.OLD).write_text( + json.dumps(_credited_notebook_json(3, tag="ex")), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "base") + base = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() + (repo / Path(self.NEW).parent).mkdir(parents=True) + _git(repo, "mv", self.OLD, self.NEW) + (repo / self.NEW).write_text( + json.dumps(_credited_notebook_json(keep, tag="ex")), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "head") + head = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() + return repo, base, head + + def _verdict(self, result, name): + for bucket in (result.ok, result.sub_threshold, result.parse_errors): + for v in bucket: + if v.path.endswith(name): + return v + raise AssertionError("le carnet n'apparait dans aucun seau") + + def test_the_loss_of_a_renamed_notebook_is_measured(self, tmp_path, monkeypatch): + repo, base, head = self._repo_with_rename(tmp_path) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head, + base_path_of={self.NEW: self.OLD}, + ) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status == "ok", \ + "avec previousFilename, le diff doit etre calcule, pas en erreur" + assert verdict.credited_lost_unexempted == 1, \ + "l'exemple credite perdu au renommage doit etre vu" + + def test_a_rename_without_loss_is_not_a_false_positive( + self, tmp_path, monkeypatch): + """Un renommage qui ne perd rien ne doit pas etre signale. + + Sans ce controle, la mesure du renommage pourrait crier a la perte sur + un simple `git mv` -- le faux positif que #19251 doit eviter autant que + l'angle mort qu'il ferme. + """ + repo, base, head = self._repo_with_rename(tmp_path, keep=3) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head, + base_path_of={self.NEW: self.OLD}, + ) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status == "ok" + assert verdict.credited_lost_unexempted == 0, \ + "aucun exemple perdu au renommage : rien a signaler" + + def test_without_the_mapping_the_rename_is_not_silently_a_zero( + self, tmp_path, monkeypatch): + """Sans `previousFilename`, on ne MESURE pas -- et surtout on ne rend + pas « zero perte » : le diff reste en erreur, que #18761 refuse de + convertir en label. Un zero silencieux serait le vrai mensonge.""" + repo, base, head = self._repo_with_rename(tmp_path) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status.startswith("error:"), \ + "base absente a ce chemin : le diff doit etre en erreur, pas a zero" + assert verdict.credited_lost_unexempted == 0 From 5b897be698d3ca9bde39a1e6adcc4983a9869a29 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 10:14:12 +0200 Subject: [PATCH 5/6] fix(ci,#19251): le chemin de base d'un renommage est cote REST, pas GraphQL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correction du commit precedent, dont l'hypothese etait fausse et que le rejeu `--hours 72` a demasquee : `previousFilename` N'EXISTE PAS sur le type GraphQL `PullRequestChangedFile`. Mesure, serveur : Field 'previousFilename' doesn't exist on type 'PullRequestChangedFile' Ses champs sont `additions, changeType, deletions, path, viewerViewedState` (verifie par introspection). Demander le champ faisait echouer TOUTES les lectures de fichiers : `--hours 72` rendait « 0 PR mesuree, 309 replis nommes » au lieu des 2 PR / 4 pertes de la veille -- une regression, pas un progres. Le classement reste donc en GraphQL (`changeType`), et le chemin de base est lu par une SECONDE source, REST : `pulls/{n}/files`, champ `previous_filename` (verifie sur #19153 : ICT-45 -> ICT-42b). Cette passe n'est faite que si la PR porte au moins un renommage, et son echec laisse le renommage NON MESURE (`previousFilename` rendu `""`) au lieu de le mesurer contre un mauvais chemin. Tests 37 -> 40 : la passe REST enrichit un noeud RENAMED, elle est sautee quand rien n'est renomme, et son echec ne casse rien. Falsification : 5 mutants, tous rouges (dont M5 : passe REST supprimee). Closes #19251 Co-Authored-By: Claude Sonnet 5.5 --- .../notebook_tools/credited_examples_sweep.py | 90 +++++++++++++++---- scripts/tests/test_credited_examples_sweep.py | 56 ++++++++++++ 2 files changed, 128 insertions(+), 18 deletions(-) diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index 24fde00c02..78ac56b519 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -20,12 +20,18 @@ - **`changeType` n'existe pas dans `gh pr list --json files`** (mesure gh 2.83.2 : cette forme ne rend que `{additions, deletions, path}`). Le classifieur ADDED/DELETED/RENAMED se nourrit donc de **GraphQL** - (`pullRequest.files.nodes { path changeType previousFilename }`), pagine au - curseur. Un carnet supprime (#19040 a supprime GameTheory-18d) fait sinon - planter le balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la - premiere version classait tout en modifie. `previousFilename` (#19251) donne - le chemin de base d'un RENAMED : quand il est present, le renommage est - **mesure** comme un MODIFIED ; quand il manque, il reste nomme NON MESURE. + (`pullRequest.files.nodes { path changeType }`), pagine au curseur. Un + carnet supprime (#19040 a supprime GameTheory-18d) fait sinon planter le + balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la premiere + version classait tout en modifie. +- **le chemin de base d'un RENAMED n'existe PAS cote GraphQL** (#19251, + mesure : le serveur repond `Field 'previousFilename' doesn't exist on type + 'PullRequestChangedFile'` ; ses champs sont `additions, changeType, + deletions, path, viewerViewedState`). Il vit cote **REST**, champ + `previous_filename` de `pulls/{n}/files` -- lu par `_previous_filenames`, + et **seulement** quand la PR porte un renommage. Avec ce chemin, le + renommage est **mesure** comme un MODIFIED ; sans lui, il reste nomme NON + MESURE. - **le cote « apres » est la tete de la PR, pas l'arbre de travail** : `check_notebooks` recoit `head_ref=headRefOid`, et le commit est amene localement s'il manque (PR squash-mergee : l'objet n'est pas dans `main`). @@ -80,7 +86,7 @@ repository(owner: $owner, name: $name) { pullRequest(number: $num) { files(first: 100, after: $cursor) { - nodes { path changeType previousFilename } + nodes { path changeType } pageInfo { hasNextPage endCursor } } } @@ -131,6 +137,38 @@ def merged_prs(repo: str, since: dt.datetime, run=_gh_json) -> list[dict]: return rows +def _previous_filenames(repo: str, number: int, run=_gh_json) -> dict[str, str]: + """`{chemin_de_tete: chemin_de_base}` des renommages, par REST. + + `previousFilename` n'existe **pas** sur le type GraphQL + `PullRequestChangedFile` (#19251, mesure : le serveur repond `Field + 'previousFilename' doesn't exist on type 'PullRequestChangedFile'` ; ses + champs sont `additions, changeType, deletions, path, viewerViewedState`). + Le chemin de base d'un renommage vit cote REST, champ `previous_filename` + de `pulls/{n}/files` -- d'ou cette seconde source, appelee **seulement** + quand la PR porte un renommage. Pagement explicite (`per_page`/`page`) : + `per_page` en `-f` ferait basculer `gh api` en POST. + """ + owner, name = repo.split("/", 1) + out: dict[str, str] = {} + page = 1 + while True: + payload = run([ + "api", (f"repos/{owner}/{name}/pulls/{number}/files" + f"?per_page=100&page={page}"), + ]) + if not isinstance(payload, list) or not payload: + return out + for f in payload: + head = f.get("filename") or "" + prev = f.get("previous_filename") or "" + if head and prev: + out[head] = prev + if len(payload) < 100: + return out + page += 1 + + def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: """Fichiers de la PR avec `changeType`, par GraphQL, pagine au curseur. @@ -138,6 +176,13 @@ def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: du changement n'existe que cote GraphQL. Pages de 100, boucle sur `pageInfo.endCursor` -- une PR de carnet peut deplacer plus de 100 fichiers (mesure : #19040 en deplace 2876+214 lignes sur plusieurs carnets). + + #19251 -- le chemin de base d'un `RENAMED` n'est pas dans GraphQL : quand la + PR porte au moins un renommage, une seconde passe REST (`_previous_filenames`) + remplit `previousFilename` sur ces seuls noeuds, pour que le renommage soit + mesurable. Si cette passe echoue, les renommages restent sans chemin de base + (`""`) et seront nommes NON MESURES par `sweep` -- jamais mesures contre le + mauvais chemin. """ owner, name = repo.split("/", 1) nodes: list[dict] = [] @@ -153,8 +198,17 @@ def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: nodes.extend(conn.get("nodes") or []) page = conn.get("pageInfo") or {} if not page.get("hasNextPage"): - return nodes + break cursor = page.get("endCursor") + if any(n.get("changeType") == "RENAMED" for n in nodes): + try: + previous = _previous_filenames(repo, number, run) + except RuntimeError: + previous = {} + for node in nodes: + if node.get("changeType") == "RENAMED": + node["previousFilename"] = previous.get(node.get("path") or "", "") + return nodes def _ipynb_by_change( @@ -172,16 +226,16 @@ def _ipynb_by_change( (supprime par #19040) ; - `ADDED` : neuf, donc **rien a perdre** par construction ; - `RENAMED` : la version de base existe **sous un autre chemin**. Depuis - #19251 ce chemin est demande au jeu GraphQL (`previousFilename`) : - quand il est present, le renommage est **mesurable** et rendu comme la - paire `(chemin_de_tete, chemin_de_base)`, pour que le diff credite soit - calcule entre `base:previous` et `head:path` -- un `git mv` suivi d'une - edition peut perdre des exemples comme un `MODIFIED`. - - `previousFilename` peut manquer (renommage detecte par git sous un seuil de - similarite, ou reponse d'API degradee) : ces renommages-la restent - **non mesurables** et sont rendus a part, pour etre **nommes** NON MESURES - plutot que comptes comme zero perte. + #19251 ce chemin est rempli par `pr_files` (passe REST, + `previous_filename`) : quand il est present, le renommage est + **mesurable** et rendu comme la paire `(chemin_de_tete, chemin_de_base)`, + pour que le diff credite soit calcule entre `base:previous` et + `head:path` -- un `git mv` suivi d'une edition peut perdre des exemples + comme un `MODIFIED`. + + `previousFilename` peut manquer (passe REST en echec, ou renommage hors du + champ REST) : ces renommages-la restent **non mesurables** et sont rendus a + part, pour etre **nommes** NON MESURES plutot que comptes comme zero perte. Pourquoi separer plutot que compter en erreur : `credited_diff_errors > 0` **bloque** la pose du label (#18761). Une erreur structurelle sur un carnet diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py index 3904543f57..49fce44609 100644 --- a/scripts/tests/test_credited_examples_sweep.py +++ b/scripts/tests/test_credited_examples_sweep.py @@ -170,6 +170,62 @@ def run(argv): assert "cursor=CUR1" not in calls[0] +class TestTheRenameBasePathComesFromRest: + """#19251 : `previousFilename` n'existe PAS cote GraphQL. + + Mesure : le serveur repond `Field 'previousFilename' doesn't exist on type + 'PullRequestChangedFile'` (ses champs : additions, changeType, deletions, + path, viewerViewedState). Le chemin de base d'un renommage est lu par une + seconde passe REST (`pulls/{n}/files`, champ `previous_filename`), et + SEULEMENT quand la PR porte un renommage. + """ + + @staticmethod + def _graphql(nodes): + return {"data": {"repository": {"pullRequest": {"files": { + "nodes": nodes, "pageInfo": {"hasNextPage": False}}}}}} + + def test_a_renamed_node_gets_its_base_path_from_rest(self): + calls = [] + + def run(argv): + calls.append(list(argv)) + if "graphql" in argv: + return self._graphql( + [{"path": "new.ipynb", "changeType": "RENAMED"}]) + return [{"filename": "new.ipynb", "previous_filename": "old.ipynb", + "status": "renamed"}] + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0]["previousFilename"] == "old.ipynb" + assert any("pulls/7/files" in arg for arg in calls[-1]), \ + f"la passe REST doit viser pulls//files : {calls[-1]}" + + def test_the_rest_pass_is_skipped_when_no_file_is_renamed(self): + calls = [] + + def run(argv): + calls.append(list(argv)) + return self._graphql([{"path": "x.ipynb", "changeType": "MODIFIED"}]) + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0].get("previousFilename", "") == "" + assert len(calls) == 1, "sans renommage, une seule source est interrogee" + + def test_a_failing_rest_pass_leaves_the_rename_unmeasured_not_wrong(self): + """L'echec de la passe REST ne doit ni planter, ni faire mesurer le + renommage contre le mauvais chemin : sans chemin de base, il reste NON + MESURE (rendu `""`, que `_ipynb_by_change` range a part).""" + def run(argv): + if "graphql" in argv: + return self._graphql( + [{"path": "new.ipynb", "changeType": "RENAMED"}]) + raise RuntimeError("gh failed (1): rate limited") + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0]["previousFilename"] == "" + + class TestFiltering: def test_only_ipynb_paths_are_kept(self): assert _mod.ipynb_paths([_nb(IPY), _nb(MD)]) == [IPY] From d29f549f8a464093bd98aaf0e3035027aa122a59 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 5 Oct 2026 10:15:03 +0200 Subject: [PATCH 6/6] docs(ci,#19251): le docstring de tete annonce trois sources, pas deux Co-Authored-By: Claude Sonnet 5.5 --- scripts/notebook_tools/credited_examples_sweep.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index 78ac56b519..1eef6f327c 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -15,7 +15,7 @@ coup : il liste les PRs MERGEES de la fenetre et rejoue, pour chacune, le diff avec SA base et SON body -- c'est l'option 1 de #19101. -## Deux sources de verite, chacune du bon cote (review 04:53Z) +## Trois sources de verite, chacune du bon cote (review 04:53Z, #19251) - **`changeType` n'existe pas dans `gh pr list --json files`** (mesure gh 2.83.2 : cette forme ne rend que `{additions, deletions, path}`). Le