Skip to content

fix(notebook-tools,#17232): cellule ajoutee sans signature float n est pas une derive de kernel - #17309

Closed
jsboige wants to merge 1 commit into
mainfrom
fix/17232-kdrift-added-cell
Closed

jsboige wants to merge 1 commit into
mainfrom
fix/17232-kdrift-added-cell

Conversation

@jsboige

@jsboige jsboige commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Grain: MED/guard -- lane myia-po-2027:CoursIA -- prev: DEEP/notebook-python #17305

Fix faux positif : cellule ajoutée sans signature float n'est pas une dérive de kernel

Ce que la PR corrige

diff_signatures() signalait comme dérive toute cellule code présente seulement dans head, sans condition sur sa signature. Or une cellule sans sortie tabulaire float ne peut pas être une dérive de repr float — le prédicat était plus large que la propriété qu'il prétend détecter.

Instance fondatrice (issue #17232, PR #17145) : papermill remplace la cellule injected-parameters à chaque ré-exécution, et nbformat 4.5 attribue un id neuf au remplaçant. Le couple (un id retiré, un id ajouté) est l'empreinte normale d'une ré-exécution — le guard le signalait comme signature_drift_cells, forçant une section ## Diagnostic dérive (C.4) pour un écart qui n'existe pas.

Correctif

La cellule ajoutée n'est signalée que si elle porte une signature float non vide — exactement le correctif borné proposé dans l'issue, variante par prédicat de signature (générale, ne couple pas l'organe au tag d'un outil tiers) plutôt que variante par tag injected-parameters.

Cohérence retrouvée : le chemin ordinal (fallback sans ids) comparait déjà () == () sur cellule ajoutée vide → pas de signalement. La branche id-alignée devient cohérente avec lui.

Tests (18 passed — 14 existants verts + 4 nouveaux)

Test Ce qu'il prouve
test_added_cell_without_signature_is_not_drift instance fondatrice (id remplacé + cellule commune pour garder la branche id active) → 0 finding
test_added_cell_with_float_output_still_flagged contrôle positif conservé : cellule ajoutée produisant un tableau float reste signalée
test_added_empty_cell_does_not_shift_common_alignment insertion d'une cellule vide AVANT une cellule commune ne décale pas l'alignement id
test_removed_injected_parameters_not_reported_either côté retrait du remplacement papermill : invariant épinglé

Sortie d'exécution :

tests/test_check_kernel_drift.py — 18 passed in 0.09s

Pourquoi le fix ne peut pas manquer une dérive réelle

Une dérive de repr float exige une signature non vide (par construction de float_signatures) : une cellule qui produit un tableau flottant reste signalée — c'est le contrôle positif. Seule la classe « cellule ajoutée sans aucune sortie float » change de verdict, et cette classe ne peut pas contenir de dérive float.

Périmètre

2 fichiers : l'organe (check_kernel_drift.py, branche id-alignée de diff_signatures uniquement) + son module de test. Aucun autre consommateur de diff_signatures (grep : un seul appelant, _run, sémantique de retour inchangée — liste d'ids, simplement sans faux positifs).

Ne chevauche pas PR #17257 (même fichier source, hunks disjoints : #17257 = body_has_derive_exemption ligne ~198, celle-ci = diff_signatures lignes ~287-297) — les deux mergent dans n'importe quel ordre.

Closes #17232

🤖 Generated with Claude Code

…s not kernel drift

diff_signatures() flagged every head-only code cell as drift
unconditionally. Papermill replaces the injected-parameters cell
under a fresh nbformat 4.5 id on each re-execution, so any
re-executed notebook carrying one tripped the guard with zero actual
float-repr drift (founding instance PR #17145).

An added cell now reports as drift only if it carries a non-empty
float signature -- a cell that produces no tabular float output
cannot be float-repr drift. This matches the ordinal fallback's
semantics, where an added empty cell already compares () == ().
Positive control preserved: an added cell producing a float table
stays flagged (test_added_cell_with_float_output_still_flagged).

Tests: 14 existing stay green + 4 new (founding instance with common
cells to keep the id-aligned branch active, positive control, no
ordinal shift on insertion, removal-side invariant).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Trivial-diff advisory (#15740, non bloquant).
genre guard dans la famille META (docs/guard/ledger/readme/test) + diff de 100 lignes changees (<= 100) + aucune exception ecrite dans le body : le litmus de la trivialite (une douzaine d'instances scannees a la suite) est credible. Le verdict est ADVISORY -- fournir une fournée ou citer une exception de la forme #15719 l'eteint.
La demande : une fournee (le geste pourrait comprendre ~10x plus d'instances), OU une exception ecrite dans le body de la forme « exception seulement residu final mesure » (#15719). Editer le body re-deroule cet organe et retire le label.

@clusterManager-Myia clusterManager-Myia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NanoClaw] structural review — les 2 fichiers du PR téléchargés au head d7832f6b ET à la base 320c5f9d, diffés localement (organe : +9/−1 exactement dans la branche id-alignée de diff_signatures ; tests : +87, les 4 nouveaux lus en entier) ; issue #17232 lue (instance fondatrice #17145, finding bec0f46c repris tel quel dans le test) ; corps du PR confronté au code mesure par mesure. Review statique : python absent de mon conteneur (siège ai-01) — les 18 tests ne sont pas rejouables ici, la vérification porte sur le code lu intégralement autour du hunk (l.215-313) et les check-runs au head.

VERDICT: LGTM (vérifié : le prédicat borné est exact et sa cohérence avec le fallback ordinal est prouvée ligne à ligne ; contrôle positif épinglé par test — la classe « cellule ajoutée produisant des floats » reste signalée ; comptages et claims du body re-mesurés : 14→18 tests, appelant unique _run l.345, hunk disjoint de #17257)

Vérifié solide (firsthand)

  • Le correctif est la bonne variante, au bon endroit. diff_signatures() signalait TOUTE cellule code ajoutée (for cid in head−base: diffs.append(cid)) ; or une cellule sans sortie float ne peut pas être une dérive de repr float — le prédicat était plus large que la propriété gardée. Le head ne flague plus que si head_sig[h_idx] est non vide (l.295-298) : c'est la variante « prédicat de signature » proposée par l'issue, qui ne couple pas l'organe au tag injected-parameters d'un outil tiers (papermill) — le bon arbitrage : l'empreinte (id retiré + id ajouté) d'une ré-exécution restera exempte pour tout outil, pas juste pour celui-ci.
  • L'argument de cohérence ordinal est exact, pas rhétorique — vérifié sur le code : _diff_signatures_ordinal (l.303-313) compare b=() vs h=head_sig[i] pour une cellule ajoutée ⇒ flaggée ssi signature non vide. La branche id-alignée fait désormais exactement la même chose par id. Les deux chemins d'alignement portent la même sémantique — c'était l'incohérence de fond de l'instance #17145.
  • Aucune dérive réelle ne peut être masquée (la question qui compte) : une dérive de repr float exige par construction une signature non vide (float_signatures l.138-162 n'émet que les matchs FLOAT_ARRAY_RE des outputs) — la classe dé-classée (« ajoutée sans AUCUNE sortie float ») ne peut pas contenir de dérive float. La classe « ajoutée avec floats » reste signalée, et c'est épinglé par le contrôle positif test_added_cell_with_float_output_still_flagged (attend ["b"]) — pas une promesse, une assertion.
  • Les 4 tests pin les vrais discriminants (lus en entier) : instance fondatrice avec id réel bec0f46c + cellule commune pour maintenir la branche id active (sinon fallback ordinal — le commentaire du test le sait) ; insertion d'une cellule vide AVANT une commune ne décale pas l'alignement (pin de régression #16466) ; côté retrait du couple papermill épinglé. Comptage re-mesuré : 14 → 18 fonctions test, conforme au body.
  • Périmètre tenu : diff_signatures a un seul appelant (_run, l.345, sémantique de retour inchangée — liste d'ids, moins les faux positifs) ; la zone de body_has_derive_exemption (~l.195-205, le hunk de #17257) est intacte au head — hunks disjoints confirmés côté fichier, les deux PR mergent dans n'importe quel ordre.
  • CI au head : Kernel drift guard (base vs PR) = success — l'organe corrigé passe sur sa propre PR (self-consistant, et la classe de faux positif #17145 est éliminée au passage) ; Exec-sequence ratchet, Always-on guards, Analyze (python) verts. Scripts Tests (CPU) et PR gate in_progress au moment de la review — le vert définitif se lira au merge.

Notes (mineures)

  1. La garde h_idx < len(head_sig) (l.297) est défensive morte : head_ids et head_sig sont bâtis du même notebook, même parcours code-only — l'index est toujours dans la plage. Sans conséquence, cohérente avec le style du fichier.
  2. Si un jour une cellule injected-parameters exécutait un affichage float (paramètre affiché en sortie), elle redeviendrait signalée — comportement correct (sortie float = signalable), à garder en tête si papermill change ses habitudes d'affichage.

Recommandation : correctif borné, cohérent entre les deux chemins d'alignement, auto-contrôlé par un contrôle positif, sur l'instance fondatrice réelle. Le vert de Scripts Tests (CPU) au head est la dernière pièce à attendre. Décision merge : Emerjesse.

@github-actions

Copy link
Copy Markdown
Contributor

Path-collision (organ #13359/#13615)

Cette PR #17309 (fix(notebook-tools,#17232): cellule ajoutee sans signature float n est pas une derive de kernel) touche au moins un chemin de fichier aussi modifie par d'autres PRs ouvertes. Risque de double-livraison (meme fichier livre deux fois, 2x le travail et 2x les runs CI). Advisory : parfois legitime (tranches coordonnees, partition paths: explicite, PRs empilees exclues) -- l'organe rend visible, il ne bloque pas.

@github-actions github-actions Bot added the pr-overlap Advisory: another open PR touches the same files (organ #13615) label Sep 21, 2026
@jsboige

jsboige commented Sep 21, 2026

Copy link
Copy Markdown
Owner Author

Imputation infra du rouge Scripts Tests (CPU) — run 35659048336, head d7832f6b (diff inchangé depuis).

5 attempts, même verdict, deux causes distinctes — aucune liée au code de cette PR :

Attempt Job Durée Verdict Nature
1 — — failure mort runner : steps figées Run tests=in_progress, aucune step en failure, log absent
2 — complète 1 failed / 14461 passed flake subprocess.CalledProcessError: git checkout-index exit 128 sur test_retroactive_control_sees_third_pair_pre_consolidation — test hors du diff de cette PR, vert au head en local, et le même workflow échoue sur main 2× à la même heure (21:45-22:25Z, fenêtre d'instabilité mesurée sur 3+ PRs : #17309, #17305, feat/17290, main 2×)
3 — — failure mort runner (même signature)
4 106542924549 22:32→22:45Z failure mort runner (même signature)
5 106552915893 23:17→23:33Z failure mort runner (même signature)

La signature « check-run failure + step Run tests figée in_progress + steps suivantes pending + --log-failed vide » signe la perte du runner (lost communication / OOM), pas un échec de tests : le check-run échoue au timeout de communication pendant que le job reste figé. L'attempt 2, seule exécution arrivée au bout de la suite, montre 14461 passed — la suite elle-même est saine.

À noter : d'autres workflows lourds passaient pendant la même période (87 checks verts sur #17305 à 22:51-23:06Z), donc ce n'est pas une panne globale du pool mais probablement son hétérogénéité — le job atterrit tantôt sur un runner qui tient la suite (attempt 2), tantôt sur un qui meurt au même step (attempts 1/3/4/5).

Suites : je ne multiplie pas les reruns aveugles au-delà de 5 attempts. Le rerun d'un job unique (gh run rerun 35659048336 --job <id>) reste la parade si un coordinateur veut relancer la jambe sur un runner plus large ; le test flake d'attempt 2 mériterait à terme une issue dédiée (il frappe aussi main).

lane myia-po-2027:CoursIA — détail des mesures sur le dashboard workspace.

@jsboige

jsboige commented Sep 21, 2026

Copy link
Copy Markdown
Owner Author

Fermée comme doublon de #17236 (mergée e8d7676, 2026-09-21T23:13Z).

Les deux PRs livrent le même correctif #17232 : la cellule ajoutée n'est signalée que si elle porte une signature float non vide (empreinte papermill injected-parameters = plomberie d'exécution, pas une dérive). Le diff restant entre cette branche et main est purement rédactionnel (commentaires) — la sémantique est couverte à l'identique par #17236.

Pourquoi cette PR n'est jamais passée verte : voir le commentaire d'imputation infra — 4 morts runner sur 5 attempts sur le check Scripts Tests (CPU), la seule exécution complète donnait 14461 passed. Le rouge n'a jamais été le code.

Leçon de livraison en double (L1356) : mon préflight de claim n'a pas vu #17236 — la recherche cross-lane au moment du [CLAIMED] doit passer --state all et les PRs récentes sur le chemin, pas seulement l'issue. Consigné côté lane.

lane myia-po-2027:CoursIA

@jsboige

jsboige commented Sep 21, 2026

Copy link
Copy Markdown
Owner Author

Doublon de #17236 (mergée) — voir le commentaire ci-dessus.

@jsboige jsboige closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-overlap Advisory: another open PR touches the same files (organ #13615) trivial-diff-advisory Diff trivial : grain META mecanique sans fournee ni exception ecrite (#15740)

Projects

None yet

2 participants