promote, drift-check: a copy that caught up to an unrecorded canonical version, or in its own asset-path spelling, is refused as a collision and reported as drift

Aperta
#18 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
38/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
github, python
Ambito
testing, tooling

Direzione di ricerca

Start with the reproduction tests in tools/tests/test_promote_collisions.py, then read the stale checks in tools/promote and tools/drift-check (notably the referenced lines around 718–765 and 377–411). Run the tools/tests harness and use the two reproductions to verify both unrecorded canonical versions and asset-path-normalised copies are treated as stale rather than edited; the issue notes that version-lookup tests will need a fake.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

bug

Follow-up from the review of https://github.com/QuantEcon/lectures/pull/14. Line numbers are at #14's head as it merges, with #17 included.

What happens

Since 7128fb1 (in #14), a recorded source that no longer matches the canonical copy counts as stale, not edited, only if its raw digest equals one of two recorded digests: its own, or the canonical copy's at the last promotion (resolve_sources, tools/promote:718-726; check_lecture, tools/drift-check:377 and :394). Two kinds of copy that have only caught up fall outside those two digests:

  • (a) A copy at a canonical version that was never recorded. In the nightly cycle the ledger records a canonical version only when a refresh PR is merged, and drift/refresh is rebuilt from main on every run. If the canonical copy changes twice before that merge, or a version comes and goes between two nightly syncs, a series that ports the version in between holds a digest the ledger never saw.
  • (b) A normalised-equal source that caught up in its own asset-path spelling. Since #14, promote admits a source that equals the canonical copy once asset references are normalised, and records the source's own raw digest. When that copy catches up with the canonical version recorded at the last refresh, it keeps its own spelling, so its digest cannot equal the canonical copy's recorded digest.

In both cases, once the canonical copy has moved on:

  • promote --refresh refuses the entry as a collision, and the nightly run goes red. Its message (tools/promote:761-765) says to "name the canonical series in sync/canonical.yml"; doing that makes the entry interim, with the lagging copy recorded as a divergent lecture.
  • drift-check reports diverged and opens a drift issue saying the copy "has since been edited on its own", although it is only behind, not edited.

Both persist until the lagging copy reaches the canonical copy's current version. The same steps go through as stale-copy when the version was recorded (the v2 refresh merged before v3) or the spelling is identical; the latter is test_a_copy_that_caught_up_then_fell_behind_again_is_still_stale in tools/tests/test_promote_collisions.py.

What #17 changes

PR #17, which lands with #14, makes drift-check compare a moved source with the canonical copy's current text under promote's normalised_text (same_lecture, tools/drift-check:385). That covers the lockstep case in #15, but not the stale rule: tools/drift-check:394 still compares the copy's raw digest with the canonical copy's recorded raw digest, and promote is unchanged. With #17, the copy in (b) is reported as converged ... up to asset path spelling while the canonical copy stays at the version it caught up with, then as diverged once the canonical copy moves on. (a) is the same with or without #17.

Related to #15, which this extends: there, drift-check alone raised a false diverged that cleared once the refresh PR merged. Here promote refuses too, so no refresh PR clears it.

Before #14

On main, drift-check already reported (a) as diverged, since it had no caught-up rule, but the refresh went through (marking the entry interim) and the run stayed green. (b) cannot arise on main, which admitted only byte-identical sources.

Reproduction (with the tools/tests harness)

from conftest import lecture_text

FIG = "_static/lecture_specific/short_path/graph.png"


def test_a_copy_at_an_unrecorded_canonical_version_is_called_an_edit(make_repo):
    repo = make_repo(["programming", "intermediate"])
    for series in ("programming", "intermediate"):
        repo.lecture(series, "pandas_panel", lecture_text("Panel Data", "Version one."))
    assert repo.promote("pandas_panel").returncode == 0  # both recorded at v1

    # programming publishes v2 and then v3 before a refresh PR is merged, so
    # the ledger still records v1. intermediate ports v2, exactly.
    repo.lecture("programming", "pandas_panel", lecture_text("Panel Data", "Version three."))
    repo.move_pin("programming", 3)
    repo.lecture("intermediate", "pandas_panel", lecture_text("Panel Data", "Version two."))
    repo.move_pin("intermediate")

    assert "[diverged] lectures/pandas_panel.md" in repo.drift_check().stdout
    res = repo.promote("--refresh")
    assert res.returncode == 1
    assert "[pandas_panel] FAILED: pandas_panel.md differs across series" in res.stderr

    # Following the advice in that message makes the entry interim.
    repo.write_canonical_map({"pandas_panel": "programming"})
    assert repo.promote("--refresh").returncode == 0
    assert repo.entry("pandas_panel")["interim"] is True


def test_a_normalised_equal_copy_that_caught_up_is_called_an_edit(make_repo):
    repo = make_repo(["intro", "intermediate"])

    def publish(series, body):  # one lecture: intro writes /_static/..., intermediate _static/...
        repo.mirror_file(series, FIG, b"png")
        ref = "/" + FIG if series == "intro" else FIG
        repo.lecture(series, "short_path", lecture_text("Shortest Paths", body, refs=[ref]))

    publish("intro", "Version one.")
    publish("intermediate", "Version one.")
    assert repo.promote("short_path").returncode == 0
    publish("intro", "Version two.")
    repo.move_pin("intro", 2)
    assert repo.promote("--refresh").returncode == 0  # intermediate kept as a stale source
    publish("intermediate", "Version two.")  # caught up, in its own spelling
    repo.move_pin("intermediate", 2)
    publish("intro", "Version three.")  # intro moves on before a refresh records the catch-up
    repo.move_pin("intro", 3)

    assert "[diverged] lectures/short_path.md" in repo.drift_check().stdout
    res = repo.promote("--refresh")
    assert res.returncode == 1
    assert "[short_path] FAILED: short_path.md differs across series" in res.stderr

Both tests pass on #14's head, with #17 included (and at 7128fb1 before it), and the asserted steps print the same lines throughout:

[diverged] lectures/pandas_panel.md: the intermediate copy lectures/pandas_panel.md no longer matches its canonical home in programming
[pandas_panel] FAILED: pandas_panel.md differs across series (same: programming:91ab2fd94f58; different: intermediate:9edfc192d91e) — a same-named file must be one lecture: reconcile or rename it upstream, or name the canonical series in sync/canonical.yml
[diverged] lectures/short_path.md: the intermediate copy lectures/short_path.md no longer matches its canonical home in intro
[short_path] FAILED: short_path.md differs across series (same: intro:d9aec721be1c; different: intermediate:0deebc23b47c) — a same-named file must be one lecture: reconcile or rename it upstream, or name the canonical series in sync/canonical.yml

A drift-check run between intermediate's catch-up and intro's next edit reports [converged] lectures/short_path.md: the intermediate copy changed and now matches intro up to asset path spelling since #17; before #17 it reported [diverged] there too (the #15 case).

When it bites

Not today. Every non-canonical source in the committed ledger is a dp-test copy (25 entries), byte-identical to its canonical copy, and the sandbox is only edited deliberately, to exercise the tools. Apart from those, and two housekeeping pages that are not in the pool (status, zreferences), no lecture on the real mirror at the current pins is held identically by two canonical-eligible series, byte for byte or after normalisation.

(a) becomes possible with the two merges in QuantEcon/project-monorepo#76: pandas_panel (programming, which is canonical, and intermediate) and autodiff (programming and jax, once jax leaves PROMOTE_EXCLUDE) are to be held identically by two series and kept in step by hand. It then takes one port of a version that was never recorded: the canonical copy edited twice in one day, or edited again while the refresh PR for the first edit is still open.

(b) also needs the pair to spell an asset reference differently. Both copies of each pair spell theirs the same way today, but the acceptance criterion in QuantEcon/project-monorepo#76 only asks for the copies to be identical after the ledger's path normalisation. The first candidates that #15 names, short_path and ifp_egm, are not candidates: on the real mirror their jax copies are different lectures, which that item renames.

Suggested fix

Make "stale" mean "the same lecture as a version the canonical copy has had", with one rule shared by both tools, as #17 did for converged:

  1. For (b), compare in the form promotion uses. For example, on an entry with a normalised-equal source, record each source's normalised digest (sha256 of normalised_text) beside its raw digest, and apply the stale test to normalised digests; other entries, which today means all of them, re-dump unchanged. drift-check, which imports normalised_text since #17, applies the same test at :394.
  2. For (a), also accept a copy that equals, up to normalisation, any version the canonical file had between its recorded promoted_at pin and the current pin. drift-check already fetches the canonical copy by pin (fetch_at_pin), and GitHub's commits API filtered by path lists the versions in between. promote reads only the mirror today, so it would need the same best-effort lookup.
  3. Until 2 lands, reword the refusal (tools/promote:761-765) and the diverged detail (tools/drift-check:406-411): when the canonical copy has moved since promotion, a copy at no recorded version may be a port that is behind rather than an edit, and a sync/canonical.yml line is the wrong remedy for it.

The two tests above, with their assertions inverted, cover the change; for (a) the harness needs a fake of the version lookup, since it cannot reach GitHub.

Lingua principale
Python
Stelle
1
Fork
0
Merge medio
1g 3h
PR unite (30g)
17

Preparare l'ambiente

Questo progetto non fornisce container di sviluppo, Dockerfile né guida per i contributori, quindi l'ambiente è a tuo carico: parti dal suo README e consulta la nostra guida al primo contributo per i passaggi generali.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di QuantEcon/lectures

Tutte le issue di QuantEcon/lectures

Issue simili

Altre issue su Python

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.