diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index a6a7a03293..9108cc7976 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -96,11 +96,13 @@ jobs: # One leg, on the dispatch's runner, or `ubuntu-26.04-arm` on the schedule, # which has no inputs; spelt as the other matrix jobs spell their runner. runs-on: ${{ matrix.os || 'ubuntu-26.04-arm' }} - # Unmeasured on a hosted runner; the first dispatch measures it. Locally a - # full matrix took about two hours (the README's "How long it takes"), - # nearly all of it a tshark per pyshark extraction, so this leaves room for - # a slower runner under the six-hour job limit. - timeout-minutes: 300 + # Run 40062653380 measured this job at about 38 minutes on `ubuntu-26.04-arm` + # with every version and engine, 04:36:11Z to 05:14:34Z, nearly all of it a + # tshark per pyshark extraction. `ubuntu-latest` took about 30% longer per + # packet in run 40047570595, so roughly 50 minutes by that ratio; 90 leaves + # room for that or a busy host, and stops a hung extraction in an hour and + # a half rather than at the six-hour job limit. + timeout-minutes: 90 strategy: matrix: os: [ "${{ inputs.runner || 'ubuntu-26.04-arm' }}" ] diff --git a/examples/benchmark/README.md b/examples/benchmark/README.md index 61f09022ae..81d9fbb199 100644 --- a/examples/benchmark/README.md +++ b/examples/benchmark/README.md @@ -1,7 +1,7 @@ # Engine Benchmark Suite A containerised benchmark that produces the engine speed table in the project docs, -`docs/source/index.rst` -- the whole table, every supported Python version, in one +`docs/source/engines.rst` -- the whole table, every supported Python version, in one run. One command, on any machine with Docker, and the output is two paste-ready reStructuredText snippets: @@ -85,7 +85,7 @@ includes any disagreement *between* interpreters, which is real uncertainty abou engine and is not something to average away; the per-version table is where it gets broken out. -One absolute figure survives inside the ratio snippet, as a footnote: the `default` +One absolute figure survives inside the ratio snippet, in a paragraph: the `default` engine's own milliseconds per packet. That is what lets a reader convert the ratios back into times on the machine the run happened on. @@ -95,13 +95,27 @@ one is `Test Results (Relative)` so that pasting both does not produce two secti with one name. The `Test Environment` block, which serves both, is in `table.rst`. +**Notes are footnotes.** A row or column that needs explaining carries a reST +auto-symbol footnote reference, `[*]_`, with the reason as the footnote body: an +engine with gaps in the per-version table, a version that was not measured or not in +full, a ratio row that lost a pass or was not measured. Rows whose ranges overlap all +point at one labelled footnote, `[#overlap]_`, since an auto-symbol body answers only +one reference. Each body follows its own table, in the order of its references, which +is how docutils pairs auto-symbols. docutils numbers footnotes page-wide, so the +snippets paste beside the page's own footnotes with nothing to renumber or convert. + +The per-version lead names the script that ran: `native.sh` passes +`--runner examples/benchmark/native.sh`, and on Actions `--workflow` with the +workflow's name; `run.sh` passes its own name to the reporting container. Run by hand, +`report.py` names `benchmark.py`, which measured every figure. + **The run-to-run spread is reported too.** A single pass cannot tell a real difference between two engines from the machine having been briefly busy, so the whole engine set is measured several times (`--repeats`, default 3) and every row carries the range -actually observed. Rows whose ranges overlap are marked `‡`: this run does not -establish which of them is faster, and printing their medians as though it did would -be inventing precision. The footnote states the median and the widest spread seen, so -a reader can judge any gap in the table for themselves. +actually observed. Rows whose ranges overlap share the `[#overlap]_` footnote: this +run does not establish which of them is faster, and printing their medians as though +it did would be inventing precision. A paragraph under the table states the median and +the widest spread seen, so a reader can judge any gap in the table for themselves. ## The property that matters most @@ -139,8 +153,8 @@ So failure is contained, and counted, at each level below: what would not be honest is discarding it silently, so the count and the first reason are reported. - **A failed pass** costs that engine that pass, not the run. The row is built from - the passes that did work, its `Passes` count shows fewer, and it is marked `¶` - with what was lost. If *every* pass failed, the engine becomes `*not measured*` + the passes that did work, its `Passes` count shows fewer, and it carries a footnote + naming what was lost. If *every* pass failed, the engine becomes `*not measured*` with the exception as its reason. Some things are never tolerated, because tolerating them would mean reporting a number @@ -456,10 +470,11 @@ No docker needed. These cover the parts of a benchmark that *can* be checked wit a stopwatch, which is more than it sounds: that a ratio is taken against the right baseline, that machine drift cancels, that repeats are paired one to one rather than averaged first, that the environments stitch together correctly, that an unmeasured -engine keeps its reason, that the overlap marking neither over- nor under-claims, and -that the emitted reStructuredText parses under **plain docutils** with no warnings and +engine keeps its reason, that the overlap marking neither over- nor under-claims, +that every footnote reference resolves to its own row's or column's body, beside the +page's own footnotes too, and that the emitted reStructuredText parses under **plain docutils** with no warnings and uses no Sphinx-only roles. That last one is a conservative choice rather than a -hard requirement: the snippets land in `docs/source/index.rst`, which Sphinx +hard requirement: the snippets land in `docs/source/engines.rst`, which Sphinx renders, so a `:mod:` role would survive there. Keeping them docutils-clean is what lets a table be pasted into any reStructuredText a plain docutils reader will see -- where a `:mod:` role comes out as a visible error block while looking perfectly fine diff --git a/examples/benchmark/entrypoint.sh b/examples/benchmark/entrypoint.sh index a25f86ce5e..13724cf073 100755 --- a/examples/benchmark/entrypoint.sh +++ b/examples/benchmark/entrypoint.sh @@ -28,6 +28,7 @@ ROUNDS="${BENCH_ROUNDS:-1000}" REPEATS="${BENCH_REPEATS:-3}" ENGINES="${BENCH_ENGINES:-default,dpkt,scapy,pypcap,pcap_ct,pypcapfile,pyshark}" EMULATED="${BENCH_EMULATED:-}" +RUNNER="${BENCH_RUNNER:-}" MODE="${BENCH_MODE:-measure}" OUT="${BENCH_OUT:-/out}" IN="${BENCH_IN:-/in}" @@ -105,6 +106,11 @@ done if [ -n "$EMULATED" ]; then report_args+=(--emulated "$EMULATED") fi +# The script driving this container, for the per-version table to name; `run.sh` sets +# it. report.py names benchmark.py, which measured every figure, when it is unset. +if [ -n "$RUNNER" ]; then + report_args+=(--runner "$RUNNER") +fi # `${report_args[@]+"${report_args[@]}"}` rather than a bare `"${report_args[@]}"`: # expanding an empty array under `set -u` is an error before bash 4.4, and a native run diff --git a/examples/benchmark/native.sh b/examples/benchmark/native.sh index 0771a47878..9e87443fb8 100755 --- a/examples/benchmark/native.sh +++ b/examples/benchmark/native.sh @@ -274,7 +274,12 @@ if [ "${#DOCUMENTS[@]}" -eq 0 ] || [ -z "${REPORTER}" ]; then exit 1 fi -REPORT_ARGS=() +# The per-version table names the script that ran, and on Actions the workflow that +# ran it, which is the one fact here report.py cannot read from the documents. +REPORT_ARGS=(--runner examples/benchmark/native.sh) +if [ -n "${GITHUB_WORKFLOW:-}" ]; then + REPORT_ARGS+=(--workflow "${GITHUB_WORKFLOW}") +fi for path in "${OUT_DIR}"/missing/*.txt; do if [ -e "${path}" ]; then REPORT_ARGS+=(--missing "$(basename "${path}" .txt)=$(tr '\n' ' ' < "${path}")") diff --git a/examples/benchmark/report.py b/examples/benchmark/report.py index a260a65abf..1b403926db 100644 --- a/examples/benchmark/report.py +++ b/examples/benchmark/report.py @@ -16,7 +16,7 @@ environment on the same repeat* divides the machine out. It is also what makes mutually exclusive environments joinable at all: ``pypcap`` and ``pcap_ct`` both provide the top-level :mod:`pcap` module and cannot coexist, and ``default`` is the -engine present in every environment. One absolute number survives, as a footnote: +engine present in every environment. One absolute number survives, below the table: the baseline's own milliseconds per packet, which is what a reader needs to convert the ratios back. @@ -36,14 +36,20 @@ **Why the spread is reported.** A single pass cannot distinguish a real difference between two engines from the machine having been briefly busy. So the whole set is measured several times and each row carries the range actually observed. Rows -whose ranges overlap are marked: this run does not establish which of them is +whose ranges overlap share a footnote: this run does not establish which of them is faster, and reporting their medians as though it did would be inventing precision. +**Why the notes are footnotes.** Every row or column that needs explaining carries a +reStructuredText footnote reference, ``[*]_`` for a note of its own and ``[#overlap]_`` +for the one note several rows share, and the bodies follow the table they annotate. +docutils numbers them page-wide, so a snippet pastes beside the page's own footnotes +without renumbering anything. + The emitted markup is plain reStructuredText for **docutils**. No Sphinx-only roles (``:mod:``, ``:func:``, ``:manpage:``) appear in it -- only double-backtick literals -- because docutils does not know them and renders them as errors. -The snippets land in :file:`docs/source/index.rst`, which Sphinx renders, so a +The snippets land in :file:`docs/source/engines.rst`, which Sphinx renders, so a Sphinx-only role would in fact survive there. Staying docutils-clean is kept deliberately anyway: it is what lets a table be pasted into any reStructuredText a plain docutils reader will see, the project README having been one such @@ -56,6 +62,7 @@ import math import statistics import sys +import textwrap from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -73,21 +80,41 @@ #: How the baseline is spelled in the report. BASELINE_LABEL = 'pcapkit' -#: Marks a row whose observed range overlaps another row's, i.e. a gap this run -#: does not resolve. A double dagger rather than a footnote reference, so the -#: emitted snippet cannot collide with footnote numbers already on the page it is -#: pasted into. +#: A reStructuredText auto-symbol footnote reference, which is how the emitted tables +#: mark a row or a column that carries a note: one reference per noted row, matched to +#: the ``.. [*]`` bodies emitted directly after the table, in the same order. +#: +#: These used to be literal symbols with a hand-written legend, on the theory that a +#: footnote reference could collide with footnote numbers already on the page the +#: snippet is pasted into. It cannot: docutils assigns auto-symbols and auto-numbers +#: page-wide, in document order, so a snippet's footnotes simply take the next free +#: symbols and numbers wherever they land -- and :file:`docs/source/engines.rst` used +#: footnotes for these tables before this module did. +FOOTNOTE = '[*]_' + +#: Label of the one footnote every overlapping row shares. Auto-numbered and labelled +#: rather than an auto-symbol, because an auto-symbol body answers exactly one +#: reference and this note is referenced from several rows. +OVERLAP_LABEL = 'overlap' + +#: Marks, in the plain-text report only, a row whose observed range overlaps another +#: row's, i.e. a gap this run does not resolve. The RST snippets use the footnote +#: :data:`OVERLAP_LABEL` instead. OVERLAP_MARK = '‡' -#: Marks a row that was not measured, and points at the note carrying the reason. -UNMEASURED_MARK = '†' - -#: Marks a measured row that lost a pass, or had extractions discarded. The number -#: is still reported -- what was collected is real -- but it rests on less evidence -#: than the rows beside it, and that has to be visible rather than inferable from -#: the pass count. +#: Marks, in the plain-text report only, a measured row that lost a pass, or had +#: extractions discarded. The number is still reported -- what was collected is real +#: -- but it rests on less evidence than the rows beside it, and that has to be visible +#: rather than inferable from the pass count. The RST snippets use a footnote. PARTIAL_MARK = '¶' +#: Width the emitted prose is wrapped to, footnote bodies included. +WIDTH = 79 + +#: What the per-version lead says measured the run when it is not told. Always true, +#: since every figure comes out of it; ``--runner`` names the script that drove it. +DEFAULT_RUNNER = 'examples/benchmark/benchmark.py' + #: Characters that begin reStructuredText inline markup, and therefore have to be #: neutralised in any text this module did not write itself. See :func:`_escape`. #: The backslash is first because the loop that applies these would otherwise escape @@ -115,7 +142,7 @@ def _escape(text: 'str') -> 'str': * anything ending ``::`` -- *literal block expected; none found*. Each of those turns the snippet this module promises can be pasted into - :file:`docs/source/index.rst` verbatim into a visible error block, and the failure + :file:`docs/source/engines.rst` verbatim into a visible error block, and the failure is invisible here because the harness's own fixtures are all well-behaved English. Args: @@ -138,6 +165,55 @@ def _escape(text: 'str') -> 'str': return text +def _wrap(text: 'str', initial: 'str' = '', subsequent: 'str' = '') -> 'list[str]': + """Wrap prose this module wrote to :data:`WIDTH`. + + Only on spaces: never inside a word, and never at a hyphen, which would split an + observed range such as ``0.0600-0.0702`` across two lines. + + Args: + text: Prose, on one line. + initial: Prefix of the first line. + subsequent: Prefix of every later line. + + Returns: + The wrapped lines. + + """ + return textwrap.wrap(text, WIDTH, initial_indent=initial, subsequent_indent=subsequent, + break_long_words=False, break_on_hyphens=False) + + +def _footnote(label: 'str', lead: 'str', tail: 'str' = '', + bullets: 'Sequence[str]' = ()) -> 'list[str]': + """Render one footnote body, ``.. [label]``, with its text indented under it. + + Text from outside this module is never wrapped, only this module's own prose. A + wrapped line is a fresh start for the parser, and an outside reason that happened + to put a run of ``=`` or a ``* `` at the start of one would turn the body into a + section title or a list: so outside text goes on the end of the last wrapped line, + as *tail*, or on a line of its own, as a bullet. + + Args: + label: ``*`` for an auto-symbol footnote, or ``#name`` for a labelled + auto-numbered one. + lead: This module's prose, wrapped. + tail: Escaped outside text, appended to the last line of *lead* as it is, + including any separator it needs. + bullets: Escaped outside text, one bullet each, after a blank line. + + Returns: + The footnote's lines, without a trailing blank. + + """ + lines = _wrap(lead, f'.. [{label}] ', ' ') + lines[-1] += tail + if bullets: + lines.append('') + lines.extend(f' * {bullet}' for bullet in bullets) + return lines + + class Row: """One engine's line in the table. @@ -290,8 +366,8 @@ def _fixed(value: 'float', decimals: 'int' = 4) -> 'str': down rather than one at a time, and significant figures give each row a different number of decimal places -- ``0.01700`` above ``14.74`` -- which is precisely the layout that makes a column impossible to scan. Fixed places line the decimal - points up, and they are also what the table this replaces in - :file:`docs/source/index.rst` has always used, so a regenerated table is a diff + points up, and they are also what the table on + :file:`docs/source/engines.rst` has always used, so a regenerated table is a diff of the numbers rather than a diff of the formatting. Args: @@ -882,10 +958,12 @@ def _package_table(documents: 'Sequence[dict[str, Any]]') -> 'list[str]': def render_versions_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', missing: 'Sequence[tuple[str, str]]' = (), - emulated: 'Optional[str]' = None) -> 'str': + emulated: 'Optional[str]' = None, + runner: 'Optional[str]' = None, + workflow: 'Optional[str]' = None) -> 'str': """Render the per-version table of absolute milliseconds per packet. - This is the snippet that replaces :file:`docs/source/index.rst`'s **Test Results** section + This is the snippet that replaces :file:`docs/source/engines.rst`'s **Test Results** section outright: engines down the side, Python versions across the top, ``--`` for a cell that could not be measured. It is self-contained -- heading, prose, table, notes -- because it is pasted as a unit, and a note explaining a gap is no use in a @@ -907,6 +985,10 @@ def render_versions_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, An that failed after writing some of its environments keeps the figures it managed and is labelled as incomplete rather than as unmeasured. emulated: Description of the emulation in play, or :data:`None`. + runner: The script that drove the run, as the lead names it; see + :data:`DEFAULT_RUNNER`. + workflow: The GitHub Actions workflow that ran *runner*, or :data:`None` for + a run from a shell. Returns: reStructuredText, ready to paste. @@ -929,13 +1011,19 @@ def render_versions_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, An if version in measured_series} lines = [] # type: list[str] - # Underlined with `-`, the level these sections take in docs/source/index.rst, so + # Underlined with `-`, the level these sections take in docs/source/engines.rst, so # the snippet pastes there verbatim; `render_rst` follows suit. lines.append('Test Results') lines.append('------------') lines.append('') - lines.append(f"Measured with ``examples/benchmark/run.sh``: {first['rounds']} timed") - lines.append('extractions of ``examples/captures/in.pcap`` per engine and Python version.') + # The script that actually ran, not a fixed one: `native.sh` and `run.sh` both end in + # this report, and naming the wrong one tells a reader to reproduce the figures a way + # they were not taken. + by = f'``{runner or DEFAULT_RUNNER}``' + if workflow: + by += f', which the {_escape(workflow)} workflow runs' + lines.extend(_wrap(f"Measured with {by}: {first['rounds']} timed extractions of " + f"``examples/captures/in.pcap`` per engine and Python version.")) lines.append('The first extraction is discarded as a warm-up. Values are milliseconds per') lines.append(f"packet, the median of {first['repeats']} passes.") lines.append('') @@ -945,63 +1033,72 @@ def render_versions_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, An lines.append('the host as much as of the engine.') lines.append('') + # Auto-symbol footnotes pair with their references in document order, so the bodies + # are collected in the order the references appear: the header row first, then the + # engines from the top. + notes = [] # type: list[list[str]] + + # Marked when there is a note about the column itself, whether that is "nothing was + # measured here" or "not everything was". A note nothing in the table points at is a + # note a reader has no reason to look for. + headers = ['Engine'] + for series in columns: + if series in absent: + notes.append(_footnote('*', 'Not measured at all, for the reason recorded at the ' + 'time:', bullets=[f'Python {series} -- ' + f'{_escape(absent[series])}'])) + elif series in partial: + notes.append(_footnote('*', 'Measured, but the run did not complete, so this column ' + 'may be missing engines that would otherwise have a ' + 'figure, for the reason recorded at the time:', + bullets=[f'Python {series} -- {_escape(partial[series])}'])) + else: + headers.append(series) + continue + headers.append(f'{series} {FOOTNOTE}') + body = [] # type: list[list[str]] for row in rows: by_series = grid.get(row.engine, {}) - gaps = [series for series in columns - if series not in by_series and series not in absent] # Marked for a gap in a column that produced something only. A blank under a # version whose image never built is explained by the column's own note, and # tagging the engine for it would blame the engine for someone else's failure. - name = f'``{row.label}``' + (f' {UNMEASURED_MARK}' if gaps else '') + gaps = [series for series in columns + if series not in by_series and series not in absent] + name = f'``{row.label}``' + if gaps: + name += f' {FOOTNOTE}' + # Every gap gets a line, including one no environment explained: a footnote + # reference with no body is a docutils error, not a blank. + by_reason = {} # type: dict[str, list[str]] + for series in gaps: + reason = reasons.get(row.engine, {}).get(series, 'no reason recorded') + by_reason.setdefault(reason, []).append(series) + # Grouped by reason rather than one bullet per cell: the usual case is one + # sentence true of several consecutive versions, and repeating it for each + # makes the notes longer than the table they annotate. + notes.append(_footnote('*', 'Not measured, for the reason recorded at the time:', + bullets=[f'on {", ".join(series_list)} -- {_escape(reason)}' + for reason, series_list in by_reason.items()])) cells = [name] for series in columns: values = by_series.get(series) cells.append(_fixed(statistics.median(values)) if values else '--') body.append(cells) - # Marked when there is a note about the column itself, whether that is "nothing was - # measured here" or "not everything was". A note nothing in the table points at is a - # note a reader has no reason to look for. - headers = ['Engine'] - for series in columns: - noted = series in absent or series in partial - headers.append(f'{series} {UNMEASURED_MARK}' if noted else series) lines.extend(_simple_table(headers, body, numeric=True)) lines.append('') # The owner's own sentence about the old hand-maintained table, kept word for word: # it is the right warning, and a regenerated table that rewords it would show up as - # a diff in the prose every time the numbers changed. + # a diff in the prose every time the numbers changed. It sits between the table and + # the footnotes, where docs/source/engines.rst has it. lines.append('The unavailable cells were attempted. They are not zeroes and must not be') lines.append('compared with a measured row.') lines.append('') - notes = [] # type: list[str] - for version, reason in sorted(absent.items(), key=lambda pair: _series_key(pair[0])): - notes.append(f'* Python {version} -- not measured at all: {_escape(reason)}') - for version, reason in sorted(partial.items(), key=lambda pair: _series_key(pair[0])): - notes.append(f'* Python {version} -- measured, but the run did not complete, so this ' - f'column may be missing engines that would otherwise have a figure: ' - f'{_escape(reason)}') - for row in rows: - by_reason = {} # type: dict[str, list[str]] - for series, reason in sorted(reasons.get(row.engine, {}).items(), key=lambda pair: - _series_key(pair[0])): - if series in absent: - continue - by_reason.setdefault(reason, []).append(series) - # Grouped by reason rather than one bullet per cell: the usual case is one - # sentence true of several consecutive versions, and repeating it for each - # makes the notes longer than the table they annotate. - for reason, series_list in by_reason.items(): - notes.append(f'* ``{row.label}`` on {", ".join(series_list)} -- {_escape(reason)}') - - if notes: - lines.append(f'``{UNMEASURED_MARK}`` not measured, or not measured in full, for the') - lines.append('reason recorded at the time:') - lines.append('') - lines.extend(notes) + for note in notes: + lines.extend(note) lines.append('') if emulated: @@ -1088,6 +1185,27 @@ def render_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', lines.append('about an engine; the per-version table is where that is broken out.') lines.append('') + # Computed before the table rather than after it, because the overlap note below + # the table depends on it: with no spread observed, "their ranges overlap" is not + # what happened, and the note says so instead. + spreads = [row.spread for row in rows if row.measured and len(row.ratios) > 1] + + # Auto-symbol bodies pair with their references in document order, so they are + # collected row by row. The overlap note is the one labelled body, since several rows + # point at it; its position does not matter, and it goes first. + notes = [] # type: list[list[str]] + # Only emitted when a row actually carries it: an explanation of a note nothing + # points at reads as though the run failed to separate rows it in fact separated + # cleanly -- and docutils would warn about an unreferenced footnote besides. + if marked: + if spreads: + overlap = ('This run does not separate these rows: their observed ranges overlap, ' + 'so the order their medians suggest is not established.') + else: + overlap = ('Measured on a single pass, so no run-to-run spread was observed and no ' + 'gap between rows is established. Re-run with more passes.') + notes.append(_footnote(f'#{OVERLAP_LABEL}', overlap)) + body = [] # type: list[list[str]] for row in rows: name = f'``{row.label}``' @@ -1099,15 +1217,34 @@ def render_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', observed = (_range(row.low, row.high) if len(row.ratios) > 1 else '*single pass*') if row.engine in marked: - name += f' {OVERLAP_MARK}' + name += f' [#{OVERLAP_LABEL}]_' if row.partial: - name += f' {PARTIAL_MARK}' + name += f' {FOOTNOTE}' + lead = ('Measured, but on less evidence than the rows beside it. What was ' + 'collected is real; what was lost is named here rather than left to be ' + 'inferred from the pass count:') + if row.discarded: + lead += (f' {row.discarded} individual extraction(s) failed and were ' + f'discarded') + failures = '; '.join(_escape(failure) for failure in row.failures) + if not failures: + tail = '.' + elif row.discarded: + tail = f'; {failures}' + else: + tail = f' {failures}' + notes.append(_footnote('*', lead, tail)) body.append([name, relative, observed, str(len(row.ratios))]) else: - body.append([f'{name} {UNMEASURED_MARK}', '*not measured*', '--', '0']) + body.append([f'{name} {FOOTNOTE}', '*not measured*', '--', '0']) + notes.append(_footnote('*', 'Not measured, for the reason the engine itself gives:', + f' {_escape(row.reason)}')) lines.extend(_simple_table(['Engine', 'Relative time', 'Observed range', 'Samples'], body)) lines.append('') + for note in notes: + lines.extend(note) + lines.append('') absolutes = baseline_absolutes(documents) if absolutes: @@ -1123,12 +1260,8 @@ def render_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', lines.append('ratio.') lines.append('') - # The spread is reported whether or not any row is marked -- it is what tells a - # reader which gaps in the table are large enough to mean anything. The marker's - # legend, by contrast, is only emitted when a row actually carries it: an - # explanation of a symbol that does not appear reads as though the run failed to - # separate rows it in fact separated cleanly. - spreads = [row.spread for row in rows if row.measured and len(row.ratios) > 1] + # The spread is reported whether or not any row carries the overlap note -- it is + # what tells a reader which gaps in the table are large enough to mean anything. if spreads: lines.append(f'Run-to-run spread: the median row varied by ' f'{_significant(statistics.median(spreads) * 100, 2)}% of its ratio across') @@ -1137,37 +1270,6 @@ def render_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', lines.append('smaller than that is noise, not a result.') lines.append('') - if marked: - if spreads: - lines.append(f'``{OVERLAP_MARK}`` this run does not separate these rows: their observed') - lines.append('ranges overlap, so the order their medians suggest is not established.') - else: - lines.append(f'``{OVERLAP_MARK}`` measured on a single pass, so no run-to-run spread was') - lines.append('observed and no gap between rows is established. Re-run with more passes.') - lines.append('') - - partial = [row for row in rows if row.measured and row.partial] - if partial: - lines.append(f'``{PARTIAL_MARK}`` measured, but on less evidence than the rows beside it.') - lines.append('What was collected is real; what was lost is named here rather than left to') - lines.append('be inferred from the pass count:') - lines.append('') - for row in partial: - detail = [] # type: list[str] - if row.discarded: - detail.append(f'{row.discarded} individual extraction(s) failed and were discarded') - detail.extend(_escape(failure) for failure in row.failures) - lines.append(f'* ``{row.label}`` -- ' + '; '.join(detail)) - lines.append('') - - unmeasured = [row for row in rows if not row.measured] - if unmeasured: - lines.append(f'``{UNMEASURED_MARK}`` not measured, for the reason the engine itself gives:') - lines.append('') - for row in unmeasured: - lines.append(f'* ``{row.label}`` -- {_escape(row.reason)}') - lines.append('') - if emulated: lines.append(f'**{emulated_rst}** Timings taken under emulation are not comparable ' f'with native') @@ -1333,6 +1435,11 @@ def main(argv: 'Optional[list[str]]' = None) -> 'int': help=('a Python version that produced no measurements, and why; ' 'repeatable. It becomes a column of "--" with the reason ' 'attached, rather than an absent column')) + parser.add_argument('--runner', default=None, metavar='SCRIPT', + help=('the script that drove the run, as the per-version table ' + f'names it (default: {DEFAULT_RUNNER})')) + parser.add_argument('--workflow', default=None, metavar='NAME', + help='the GitHub Actions workflow that ran --runner, if any') parser.add_argument('--rst-out', default=None, help='also write just the ratio RST snippet here') parser.add_argument('--versions-rst-out', default=None, @@ -1362,11 +1469,12 @@ def main(argv: 'Optional[list[str]]' = None) -> 'int': rows = collect(documents) print(render_text(rows, documents, args.image, args.emulated, missing)) - versions_snippet = render_versions_rst(rows, documents, missing, args.emulated) + versions_snippet = render_versions_rst(rows, documents, missing, args.emulated, + args.runner, args.workflow) snippet = render_rst(rows, documents, args.image, args.emulated, missing) print('-' * 78) - print('reStructuredText below, ready to paste into docs/source/index.rst') + print('reStructuredText below, ready to paste into docs/source/engines.rst') print('-' * 78) print() print(versions_snippet) diff --git a/examples/benchmark/run.sh b/examples/benchmark/run.sh index 0e6f5ef6d0..9fd02b9742 100755 --- a/examples/benchmark/run.sh +++ b/examples/benchmark/run.sh @@ -437,6 +437,7 @@ docker create \ --platform "${PLATFORM}" \ --env 'BENCH_MODE=report' \ --env "BENCH_EMULATED=${EMULATED}" \ + --env 'BENCH_RUNNER=examples/benchmark/run.sh' \ "${REPORT_IMAGE}" >/dev/null status=0 diff --git a/examples/benchmark/test_harness.py b/examples/benchmark/test_harness.py index 5e34f90a46..590371bab1 100644 --- a/examples/benchmark/test_harness.py +++ b/examples/benchmark/test_harness.py @@ -118,6 +118,28 @@ def document(environment, measurements, unmeasured=None, repeats=2, packages=Non } +def footnote_bodies(snippet): + """Every footnote body in *snippet*, each as its lines up to the next blank-led line. + + Args: + snippet: Emitted reStructuredText. + + Returns: + The bodies, in document order, ``.. [*]`` line first. + + """ + bodies = [] + lines = snippet.splitlines() + for index, line in enumerate(lines): + if not line.startswith('.. ['): + continue + end = index + 1 + while end < len(lines) and (not lines[end] or lines[end].startswith(' ')): + end += 1 + bodies.append('\n'.join(lines[index:end]).rstrip()) + return bodies + + class TestRatios: """The arithmetic that turns milliseconds into comparable numbers.""" @@ -455,7 +477,7 @@ def test_overlap_legend_only_appears_when_a_row_carries_it(self): 'dpkt': [0.0104, 0.0105], 'scapy': [0.0917, 0.0918]})] snippet = report.render_rst(report.collect(clean), clean) - assert report.OVERLAP_MARK not in snippet + assert f'[#{report.OVERLAP_LABEL}]' not in snippet assert 'does not separate these rows' not in snippet # ...but the spread is still reported, because it is what makes the gaps # above interpretable at all. @@ -465,7 +487,8 @@ def test_overlap_legend_only_appears_when_a_row_carries_it(self): 'dpkt': [0.020, 0.030], 'scapy': [0.025, 0.035]})] snippet = report.render_rst(report.collect(muddy), muddy) - assert report.OVERLAP_MARK in snippet + assert f'``dpkt`` [#{report.OVERLAP_LABEL}]_' in snippet + assert f'``scapy`` [#{report.OVERLAP_LABEL}]_' in snippet assert 'does not separate these rows' in snippet def test_provenance_is_present_and_host_free(self): @@ -611,7 +634,7 @@ def test_a_lost_pass_is_marked_and_explained(self): repeats=3, failures={'pyshark': ['pass 3: TSharkCrashException: retcode 255']})] snippet = report.render_rst(report.collect(docs), docs) - assert report.PARTIAL_MARK in snippet + assert f'``pyshark`` {report.FOOTNOTE}' in snippet assert 'TSharkCrashException' in snippet assert 'in pypcap, pass 3' in snippet @@ -621,7 +644,7 @@ def test_discarded_extractions_are_counted(self): discarded={'pyshark': 2})] rows = {row.engine: row for row in report.collect(docs)} assert rows['pyshark'].discarded == 4 # two per pass, two passes - snippet = report.render_rst(report.collect(docs), docs) + snippet = ' '.join(report.render_rst(report.collect(docs), docs).split()) assert '4 individual extraction(s) failed and were discarded' in snippet def test_a_clean_row_is_not_marked_partial(self): @@ -629,7 +652,7 @@ def test_a_clean_row_is_not_marked_partial(self): docs = [document('pypcap', {'default': [0.20, 0.20], 'dpkt': [0.020, 0.020]})] rows = {row.engine: row for row in report.collect(docs)} assert not rows['dpkt'].partial - assert report.PARTIAL_MARK not in report.render_rst(report.collect(docs), docs) + assert report.FOOTNOTE not in report.render_rst(report.collect(docs), docs) def test_every_pass_failing_makes_the_row_unmeasured(self): """An engine that never produced a number reports why, not a zero.""" @@ -898,7 +921,7 @@ def test_environments_disagreeing_about_one_cell_report_both(self): class TestVersionsMarkup: - """The per-version table as it will be pasted into docs/source/index.rst.""" + """The per-version table as it will be pasted into docs/source/engines.rst.""" def _snippet(self, missing=(), emulated=None, **kwargs): """Render the per-version snippet from a whole matrix run.""" @@ -990,13 +1013,13 @@ def test_every_engine_has_a_row_whatever_it_managed(self): def test_an_engine_with_a_gap_is_marked_and_explained(self): """The mark points at a reason, and the reason is the engine's own.""" snippet, _, _ = self._snippet() - assert f'``pypcapfile`` {report.UNMEASURED_MARK}' in snippet + assert f'``pypcapfile`` {report.FOOTNOTE}' in snippet assert 'pypcapfile does not support Python 3.12' in snippet def test_a_fully_measured_engine_is_not_marked(self): """No mark on a row with nothing to explain.""" snippet, _, _ = self._snippet() - assert f'``dpkt`` {report.UNMEASURED_MARK}' not in snippet + assert f'``dpkt`` {report.FOOTNOTE}' not in snippet def test_one_reason_covering_several_versions_is_stated_once(self): """Notes are grouped by reason, not one bullet per empty cell. @@ -1007,8 +1030,9 @@ def test_one_reason_covering_several_versions_is_stated_once(self): """ snippet, _, _ = self._snippet() - note = [line for line in snippet.splitlines() - if line.startswith('* ``pypcap``')] + body, = [body for body in footnote_bodies(snippet) + if 'pypcap is not installable' in body] + note = [line for line in body.splitlines() if line.startswith(' * on ')] assert len(note) == 1 assert '3.12, 3.13, 3.14' in note[0] @@ -1016,8 +1040,10 @@ def test_a_missing_version_is_a_marked_column_of_dashes(self): """A version that produced nothing says so in the table and in a note.""" snippet, _, _ = self._snippet( missing=[('3.15', 'the py3.15 image failed to build')]) - assert f'3.15 {report.UNMEASURED_MARK}' in snippet - assert '* Python 3.15 -- not measured at all: the py3.15 image failed to build' in snippet + assert f'3.15 {report.FOOTNOTE}' in snippet + body = footnote_bodies(snippet)[0] + assert body.startswith('.. [*] Not measured at all') + assert ' * Python 3.15 -- the py3.15 image failed to build' in body def test_a_missing_version_does_not_blame_the_engines(self): """No engine is marked for a column that never ran. @@ -1029,12 +1055,13 @@ def test_a_missing_version_does_not_blame_the_engines(self): """ snippet, _, rows = self._snippet(versions=('3.11',), missing=[('3.15', 'the image failed to build')]) - assert f'3.15 {report.UNMEASURED_MARK}' in snippet + assert f'3.15 {report.FOOTNOTE}' in snippet # Every engine, not a sample: on 3.11 every engine is measurable, so the # only mark in the whole table should be the one on the 3.15 column heading. for row in rows: - assert f'``{row.label}`` {report.UNMEASURED_MARK}' not in snippet - assert snippet.count(report.UNMEASURED_MARK) == 2 # the heading, and its legend + assert f'``{row.label}`` {report.FOOTNOTE}' not in snippet + assert snippet.count(report.FOOTNOTE) == 1 # the heading + assert len(footnote_bodies(snippet)) == 1 # and its note def test_a_partly_measured_version_is_not_called_unmeasured(self): """A version can have both figures and a failure note, and both are true. @@ -1055,7 +1082,7 @@ def test_a_partly_measured_version_is_not_called_unmeasured(self): dpkt_row = [line for line in snippet.splitlines() if line.startswith('``dpkt``')][0] assert dpkt_row.count('--') == 0 # ...and an engine with a real gap in it is still marked and still explained. - assert f'``pypcapfile`` {report.UNMEASURED_MARK}' in snippet + assert f'``pypcapfile`` {report.FOOTNOTE}' in snippet assert 'pypcapfile does not support Python 3.12' in snippet def test_emulation_warning_reaches_the_snippet(self): @@ -1310,7 +1337,7 @@ def _documents(self, reason): def test_both_snippets_still_parse(self, reason): """A gcc diagnostic in a reason must not break the pasted table. - The promise is that these snippets go into docs/source/index.rst verbatim, + The promise is that these snippets go into docs/source/engines.rst verbatim, and that they stay parseable by plain docutils -- where each of these strings produces a visible error block instead of the table. @@ -1399,6 +1426,202 @@ def test_the_emulation_note_is_escaped_too(self): assert emulated in report.render_text(rows, docs, emulated=emulated) +class TestFootnotes: + """Row and column notes are reStructuredText footnotes, as docs/source/engines.rst has them. + + The tables used to mark rows with literal symbols and explain them in a hand-written + legend, which had to be converted by hand before the snippet could go on the page. + Auto-symbol footnotes pair with their references by order alone, so the order the + bodies come out in is checked against what docutils actually resolves, not against + a count. + + """ + + #: The page's own footnotes, ahead of the snippets, as on docs/source/engines.rst. + PAGE = ('A sentence with a note [*]_ and a numbered one [#]_.\n\n' + '.. [*] A note that was on the page first.\n\n' + '.. [#] A numbered note that was on the page first.\n\n') + + def _snippets(self): + """Both snippets, from a run with absent and partial columns, gaps and a lost pass.""" + docs = matrix(versions=('3.11', '3.12', '3.13'), missing_pyshark=('3.13',)) + for entry in docs[0]['results']: + if entry['engine'] == 'pyshark': + entry['failures'] = ['pass 3: TSharkCrashException: retcode 255'] + missing = [('3.12', 'the 3.12-pypcap container exited non-zero'), + ('3.15', 'no Python 3.15 interpreter was found on this machine')] + rows = report.collect(docs) + return (report.render_versions_rst(rows, docs, missing), + report.render_rst(rows, docs, missing=missing)) + + @staticmethod + def _resolved(text): + """Each footnote reference's cell text, against the body docutils resolved it to.""" + docutils_core = pytest.importorskip('docutils.core') + from docutils import nodes # pylint: disable=import-outside-toplevel + + messages = [] + tree = docutils_core.publish_doctree( + text, settings_overrides={'halt_level': 2, 'report_level': 2, + 'warning_stream': messages, + 'input_encoding': 'unicode', + 'output_encoding': 'unicode'}) + assert not messages, f'docutils warned: {messages}\n\n{text}' + assert not list(tree.findall(nodes.problematic)), text + pairs = [] + for reference in tree.findall(nodes.footnote_reference): + assert reference.get('refid'), f'unresolved footnote reference in\n{text}' + footnote = tree.ids[reference['refid']] + note = '\n\n'.join(child.astext() for child in footnote.children + if not isinstance(child, nodes.label)) + pairs.append((reference.parent.astext(), note)) + return pairs + + def test_no_literal_markers(self): + """No symbol for a reader to look up in a legend that is not a footnote.""" + for snippet in self._snippets(): + for mark in '†‡¶': + assert mark not in snippet, snippet + assert 'not measured, or not measured in full, for the' not in snippet + + def test_every_reference_resolves_to_its_own_note(self): + """The note a row or column points at is that row's or column's, in both tables.""" + versions, ratio = self._snippets() + pairs = self._resolved(versions) + assert len(pairs) == versions.count(report.FOOTNOTE) + for cell, note in pairs: + if cell.startswith('3.'): + assert f'Python {cell.split()[0]} --' in note, (cell, note) + else: + assert (f'{cell.split()[0]} does not support' in note + or 'pypcap is not installable' in note), (cell, note) + notes = {cell.split()[0]: note for cell, note in pairs} + assert 'not measured at all' in notes['3.15'].lower() + assert 'the run did not complete' in notes['3.12'] + + notes = {cell.split()[0]: note for cell, note in self._resolved(ratio)} + assert 'TSharkCrashException' in notes['pyshark'] + assert 'less evidence' in notes['pyshark'] + + def test_they_paste_beside_the_page_footnotes(self): + """docutils numbers footnotes page-wide, so nothing on the page collides. + + This is the case the literal markers were chosen to avoid, and it does not + arise: the page's own notes take the first symbol and number, the snippets' + take the next, and every reference still reaches its own body. + + """ + versions, ratio = self._snippets() + pairs = self._resolved(self.PAGE + versions + '\n' + ratio) + assert pairs[0][1] == 'A note that was on the page first.' + assert pairs[1][1] == 'A numbered note that was on the page first.' + notes = {cell.split()[0]: note for cell, note in pairs[2:]} + assert 'Python 3.15' in notes['3.15'] + assert 'pypcapfile does not support' in notes['pypcapfile'] + assert 'TSharkCrashException' in notes['pyshark'] + + def test_overlapping_rows_share_one_labelled_note(self): + """An auto-symbol body answers one reference; this note answers several.""" + docs = [document('pypcap', {'default': [0.200, 0.201], 'dpkt': [0.020, 0.030], + 'scapy': [0.025, 0.035], 'pypcap': [0.028, 0.032]})] + snippet = report.render_rst(report.collect(docs), docs) + assert snippet.count(f'[#{report.OVERLAP_LABEL}]_') == 3 + assert snippet.count(f'.. [#{report.OVERLAP_LABEL}]') == 1 + notes = {note for _, note in self._resolved(snippet)} + assert len(notes) == 1 + assert 'does not separate these rows' in notes.pop() + + def test_a_single_pass_overlap_note_says_so(self): + """With no spread observed, "their ranges overlap" would not be what happened.""" + docs = [document('pypcap', {'default': [0.2], 'dpkt': [0.02]}, repeats=1)] + snippet = report.render_rst(report.collect(docs), docs) + assert f'.. [#{report.OVERLAP_LABEL}] Measured on a single pass' in snippet + assert self._resolved(snippet) + + def test_an_unmeasured_row_carries_its_reason_as_a_footnote(self): + """Not measured anywhere: the engine's own reason is the note.""" + docs = [document('pypcap', {'default': [0.20, 0.20]}, + {'pyshark': 'tshark is not installed'})] + snippet = report.render_rst(report.collect(docs), docs) + assert f'``pyshark`` {report.FOOTNOTE}' in snippet + (_, note), = self._resolved(snippet) + assert note == ('Not measured, for the reason the engine itself gives: ' + 'tshark is not installed') + + def test_the_bodies_follow_their_table(self): + """Each note sits under its own table, not gathered at the end of the page.""" + versions, ratio = self._snippets() + lines = ratio.splitlines() + last_rule = max(index for index, line in enumerate(lines) if line.startswith('====')) + assert lines[last_rule + 1] == '' + assert lines[last_rule + 2].startswith('.. [') + assert ratio.index('.. [') < ratio.index('The single absolute figure') + + lines = versions.splitlines() + last_rule = max(index for index, line in enumerate(lines) if line.startswith('====')) + # The owner's sentence stays between the table and its notes, where the page has it. + assert lines[last_rule + 2].startswith('The unavailable cells were attempted.') + assert lines[last_rule + 5].startswith('.. [') + + def test_a_gap_nothing_explained_still_has_a_body(self): + """An engine missing from a version's documents altogether is still a gap. + + A literal marker with no legend line was merely unhelpful; a footnote reference + with no body is a docutils error on the page. + + """ + docs = [document('3.11-pcap_ct', {'default': [0.2, 0.2], 'dpkt': [0.02, 0.02]}), + document('3.12-pcap_ct', {'default': [0.2, 0.2]}, python='3.12.12')] + snippet = report.render_versions_rst(report.collect(docs), docs) + assert f'``dpkt`` {report.FOOTNOTE}' in snippet + (_, note), = self._resolved(snippet) + assert 'on 3.12 -- no reason recorded' in note + + def test_the_lead_names_the_script_that_ran(self): + """native.sh and run.sh both end in this report; it names whichever ran.""" + docs = matrix(versions=('3.11',)) + rows = report.collect(docs) + + def lead(**kwargs): + text = report.render_versions_rst(rows, docs, **kwargs) + return ' '.join(text.split('All columns come')[0].split()) + + assert 'Measured with ``examples/benchmark/benchmark.py``: 1000 timed' in lead() + assert 'Measured with ``examples/benchmark/run.sh``: 1000 timed' in lead( + runner='examples/benchmark/run.sh') + assert ('Measured with ``examples/benchmark/native.sh``, which the Engine Benchmark ' + 'workflow runs: 1000 timed extractions of ``examples/captures/in.pcap`` per ' + 'engine and Python version.') in lead(runner='examples/benchmark/native.sh', + workflow='Engine Benchmark') + + def test_the_command_line_passes_the_runner_through(self, tmp_path, capsys): + """And points the reader at the page the snippets go on.""" + path = tmp_path / '3.11-pcap_ct.json' + path.write_text(json.dumps(document('3.11-pcap_ct', {'default': [0.2, 0.2]})), + encoding='utf-8') + out = tmp_path / 'versions.rst' + assert report.main([str(path), '--runner', 'examples/benchmark/native.sh', + '--workflow', 'Engine Benchmark', + '--versions-rst-out', str(out)]) == 0 + text = ' '.join(out.read_text(encoding='utf-8').split()) + assert '``examples/benchmark/native.sh``, which the Engine Benchmark workflow runs' in text + printed = capsys.readouterr().out + assert 'ready to paste into docs/source/engines.rst' in printed + assert 'index.rst' not in printed + + def test_each_script_names_itself(self): + """native.sh passes its own name, and run.sh its own through the report container.""" + here = Path(__file__).resolve().parent + native = (here / 'native.sh').read_text(encoding='utf-8') + assert 'REPORT_ARGS=(--runner examples/benchmark/native.sh)' in native + assert 'REPORT_ARGS+=(--workflow "${GITHUB_WORKFLOW}")' in native + run = (here / 'run.sh').read_text(encoding='utf-8') + assert "--env 'BENCH_RUNNER=examples/benchmark/run.sh'" in run + entrypoint = (here / 'entrypoint.sh').read_text(encoding='utf-8') + assert 'RUNNER="${BENCH_RUNNER:-}"' in entrypoint + assert 'report_args+=(--runner "$RUNNER")' in entrypoint + + class TestFixedFormatting: """Formatting for a column read downwards rather than a value read alone.""" diff --git a/tests/project/test_benchmark_workflow_2125.py b/tests/project/test_benchmark_workflow_2125.py index 79239a81ef..fcdca17a68 100644 --- a/tests/project/test_benchmark_workflow_2125.py +++ b/tests/project/test_benchmark_workflow_2125.py @@ -13,8 +13,9 @@ * :class:`TestPermissions`: read-only at the top, ``discussions: write`` on the ``discussion`` job alone, and no push. * :class:`TestTheJobs`: one leg on the chosen runner with the engine tiers - installed, running ``native.sh`` and never Docker; the tshark steps as - ``unit-tests.yml``'s; the post to ``: ``. + installed, running ``native.sh`` and never Docker, under a timeout sized from a + measured run; the tshark steps as ``unit-tests.yml``'s; the post to + ``: ``. * :class:`TestTheMeasureStep` and :class:`TestTheReportStep` run those steps' own ``run:`` scripts under bash, with a stand-in ``native.sh``. * :class:`TestNativeScript` runs :file:`examples/benchmark/native.sh` itself on @@ -242,6 +243,19 @@ def test_one_leg_on_the_chosen_runner(self) -> None: with self.subTest(job=job_id): self.assertEqual(job(WORKFLOW, job_id)['runs-on'], 'ubuntu-latest') + def test_the_timeout_is_sized_from_a_measured_run(self) -> None: + """Run 40062653380 took about 38 minutes on the default runner with everything. + + The placeholder was 300, which let a hung extraction hold a runner for five + hours. 90 leaves over twice the measured time, and the comment names the run. + + """ + self.assertEqual(job(WORKFLOW, 'benchmark')['timeout-minutes'], 90) + text = (WORKFLOWS / WORKFLOW).read_text(encoding='utf-8') + comment = text[text.index(' benchmark:\n'):text.index(' timeout-minutes: 90')] + self.assertIn('Run 40062653380 measured this job at about 38 minutes', comment) + self.assertNotIn('first dispatch measures it', text) + def test_the_engine_tiers_are_installed(self) -> None: setups = [item for item in steps(WORKFLOW, 'benchmark') if str(item.get('uses', '')).startswith('actions/setup-python@')]